Handle different contract versions #130
Conversation
| @@ -175,23 +146,23 @@ class CPK { | |||
| } | |||
|
|
|||
| get contract(): Contract | undefined { | |||
| return this.#contract | |||
| return this.#contractManager?.contract | |||
There was a problem hiding this comment.
Way to wrap the accessor earlier so that this can be done without a change to the interface!
| if (!this.#fallbackHandlerAddress) { | ||
| throw new Error('CPK fallbackHandlerAddress uninitialized') | ||
| if (!this.#contractManager) { | ||
| throw new Error('CPK contractManager uninitialized') |
There was a problem hiding this comment.
This possibly indicates a change in behavior. Whereas before, contract methods are lazily evaluated, now it may be that if the contract isn't there at init time of the CPK, it fails then, instead of inside of execTransactions.
src/config/networks.ts
Outdated
| } | ||
|
|
||
| export interface NetworkConfigEntry { | ||
| masterCopyAddress: Address |
There was a problem hiding this comment.
I think this be an optional field. Widening the type here still respects semantic versioning, and this way, somebody using the new typing could just pass in masterCopyAddressVersions instead of masterCopyAddress. Also see my remarks in the normalization logic and in the tests where this requirement exists.
| proxyFactoryAddress: networks[networkId].proxyFactoryAddress, | ||
| multiSendAddress: networks[networkId].multiSendAddress, | ||
| fallbackHandlerAddress: networks[networkId].fallbackHandlerAddress | ||
| } |
There was a problem hiding this comment.
It seems that if the user specifies the masterCopyAddressVersions field in networks, this method will ignore the user's settings. That doesn't seem right to me.
Maybe instead use something like:
if (networks[networkId].masterCopyAddress) {
mcVersions = [
{
version: masterCopyAddressVersions[0].version,
address: networks[networkId].masterCopyAddress
}
]
} else if (networks[networkId].masterCopyAddressVersions) {
mcVersions = networks[networkId].masterCopyAddressVersions
}Also, looking at the code, it's clear that mcVersions must be nonempty in order for the CPK to function for a particular network, so a check to validate that after this may be a good thing.
src/contractManagers/index.ts
Outdated
| const codeAtAddress = await ethLibAdapter.getCode(proxyAddress) | ||
| const isDeployed = codeAtAddress !== '0x' | ||
| if (isDeployed) { | ||
| properVersion = masterCopyVersion.version |
There was a problem hiding this comment.
Maybe it would be better to set properVersion = await temporaryContract.call('VERSION', []) here? That way, if a user has upgraded the proxy, this method will switch to the more efficient module manager for the upgraded version.
There was a problem hiding this comment.
I would like to talk about this in Zoom :)
src/contractManagers/index.ts
Outdated
| return proxyAddress | ||
| } | ||
|
|
||
| get contractVersionManager(): CommonContractManager | undefined { |
test/ethers/shouldWorkWithEthers.ts
Outdated
| address: gnosisSafe2.address, | ||
| version: '1.1.1' | ||
| } | ||
| ], | ||
| masterCopyAddress: gnosisSafe.address, |
There was a problem hiding this comment.
I'd remove masterCopyAddress here because it actually overrides the masterCopyAddressVersions from what I can tell from the code. Of course, that would require making the masterCopyAddress field optional in the entry type.
germartinez
force-pushed
the
handle-contract-versions
branch
from
962fde0 to
0ffdb2d
Compare
germartinez
force-pushed
the
handle-contract-versions
branch
from
0ffdb2d to
06a2562
Compare
No description provided.