Non upgradeable TeleporterRegistryApps #458
Conversation
minghinmatthewlam
requested review from
iansuvak,
feuGeneA,
geoff-vball,
bernard-avalabs,
michaelkaplan13 and
cam-schultz
There was a problem hiding this comment.
To aid with review, what commit are the non-upgradeable variants sourced from?
There was a problem hiding this comment.
https://github.com/ava-labs/teleporter/blob/c6c6117c99ef0a426595c8ae2c536dc89eeea735/contracts/src/teleporter/registry/TeleporterUpgradeable.sol main before the upgradability PR was merged
| * | ||
| * @custom:security-contact https://github.com/ava-labs/teleporter/blob/main/SECURITY.md | ||
| */ | ||
| abstract contract TeleporterRegistryApp is Context, ITeleporterReceiver, ReentrancyGuard { |
There was a problem hiding this comment.
Would it make sense to add a ITeleporterRegistryApp interface that both the upgradeable and non-upgradeable versions implement?
There was a problem hiding this comment.
To aid in review, I'd suggest we defer this to a separate PR.
There was a problem hiding this comment.
This may require more changes. Right now most of the functions are public, and for interfaces the visibility would need to be made external. The functions were made public so that inherited contracts can still call these functions internally without doing this.func. To still provide the ability to call internally, we could go through the external calling internal logic pattern.
| function _erc7201StorageSlot(bytes memory storageName) private pure returns (bytes32) { | ||
| return keccak256( | ||
| abi.encode( | ||
| uint256(keccak256(abi.encodePacked("avalanche-ictt.storage.", storageName))) - 1 |
There was a problem hiding this comment.
Should this be "teleporter.storage"?
There was a problem hiding this comment.
nice catch, wasn't sure if this should be icm but will stick with teleporter for now
There was a problem hiding this comment.
nvm in the contract already uses teleporter used the wrong name for test
Co-authored-by: Michael Kaplan <[email protected]> Signed-off-by: minghinmatthewlam <[email protected]>
| * | ||
| * Checks that the caller is the owner of the contract for upgrade access. | ||
| */ | ||
| function _checkTeleporterUpgradeAccess() internal view virtual override { |
There was a problem hiding this comment.
Should we rename this function to something like _checkTeleporterRegistryAccess? Don't want to overload the term "upgrade" in a non-upgradeable contract.
There was a problem hiding this comment.
changed to _changeTeleporterRegistryAppAccess
Why this should be merged
Fixes #416 and #421
How this works
How this was tested
Restructured unit tests so that there are base tests to cover both non-upgradeable and upgradeable versions of the TeleporterRegistryApp contracts.
How is this documented
update README to include upgradeable contracts