Non upgradeable extensions #206
Conversation
minghinmatthewlam
requested review from
iansuvak,
feuGeneA,
geoff-vball,
bernard-avalabs,
michaelkaplan13 and
cam-schultz
| * that will be rewarded to sender of the report. | ||
| */ | ||
| function initialize( | ||
| contract NativeTokenRemote is NativeTokenRemoteUpgradeable { |
There was a problem hiding this comment.
Can we add a comment explaining that inheriting from the Upgradeable version and adding a constructor that calls initialize creates a non-upgradeable instance of these contracts?
There was a problem hiding this comment.
added comments to each non-upgradeable
|
For fellow reviewers: Excluding the first commit that renames the files from the diff makes the code changes much easier to follow: https://github.com/ava-labs/avalanche-interchain-token-transfer/pull/206/files/74ac6c13b673fcb977e15975ec80be2b7f94b375..5e638c75af68f352e6e2d2efffa97d4c59bdeb97#diff-78821aeaa7f01fc3ef181c83469473e5510a5dbf474e0fe850952415d1ac05d4 |
There was a problem hiding this comment.
Can we pick a different name for this file and enum to avoid confusion with Open Zeppelin's Initializable, especially since they're both in the inheritance graph? It's a bit wordy, but perhaps ContractInitializable to make that distinction.
There was a problem hiding this comment.
renamed to ICTTInitializable
minghinmatthewlam
requested review from
geoff-vball,
michaelkaplan13 and
cam-schultz
| * keccak256(abi.encode(uint256(keccak256("avalanche-ictt.storage.ERC20TokenHome")) - 1)) & ~bytes32(uint256(0xff)); | ||
| */ | ||
| bytes32 public constant ERC20_TOKEN_HOME_STORAGE_LOCATION = | ||
| 0x914a9547f6c3ddce1d5efbd9e687708f0d1d408ce129e8e1a88bce4f40e29500; |
There was a problem hiding this comment.
Just a thought - we should consider how to verify these addresses, possibly in CI.
There was a problem hiding this comment.
there is a StorageSlotTests that checks for storage slots
There was a problem hiding this comment.
Still going through the diff a bit more closely, but could you add some comments explaining that the non-upgradeable versions are implemented by calling the one time initializer of the upgradeable versions such that they can never be upgraded? Could definitely see future engineers being confused about the pattern of NonUpgradeableApp is UpgradeableApp
| | src/utils/SafeWrappedNativeTokenDeposit.sol | 100.00% (5/5) | 100.00% (8/8) | 100.00% (0/0) | 100.00% (1/1) | | ||
| | src/utils/SendReentrancyGuard.sol | 100.00% (8/8) | 100.00% (10/10) | 100.00% (0/0) | 100.00% (4/4) | | ||
| | src/utils/TokenScalingUtils.sol | 100.00% (8/8) | 100.00% (14/14) | 100.00% (2/2) | 100.00% (4/4) | | ||
| | Total | 100.00% (478/478) | 100.00% (599/599) | 100.00% (72/72) | 100.00% (125/125) | |
| token.safeTransfer(message.fallbackRecipient, remainingAllowance); | ||
| } | ||
| ) ERC20TokenHomeUpgradeable(Initializable.Allowed) { | ||
| initialize(teleporterRegistryAddress, teleporterManager, tokenAddress_, tokenDecimals_); |
There was a problem hiding this comment.
Nit: Don't think we need the trailing underscores here anymore since they don't conflict with any variables in this contract itself.
I think this applies in a number of places now since all of the namespaced variable names have leading underscores.
Why this should be merged
Fixes #187
How this works
Creates non-upgradeable extensions of the four upgradeable token transferrer contracts.
The non-upgradeable extensions inherit from their corresponding upgradeable version, and pass into the parent constructor to allow initialization, then call
initialize.How this was tested
CI
adding unit tests for initialization
How is this documented
TODO, update readmes