Upgradeability for TeleporterUpgradeable and TeleporterOwnerUpgradeable

[minghinmatthewlam]

Why this should be merged

Fixes #420 #414

How this works

Makes it so the utility contracts are upgradeable, and can have easy non-upgradeable extensions that derive off these contracts in future issue.

How this was tested

CI

How is this documented

[openzeppelin-code]

Upgradeability POC

Generated at commit: a322c6a3cb9475a59e831c3153e830a978fdc145

🚨 Report Summary

	Severity Level	Results
Contracts	Critical High Medium Low Note Total	0 0 0 2 14 16
Dependencies	Critical High Medium Low Note Total	0 0 0 0 0 0

---

For more details view the full report in OpenZeppelin Code Inspector

[michaelkaplan13]

Any idea why this ABI binding changed despite the contract being untouched?

[michaelkaplan13]

Looks like it's just the IPFS hash of the deployed byte code that changed, but still seems odd since nothing else was modified as far as I can tell.

[michaelkaplan13]

Just for my own understanding, do you know why ContextUpgradeable doesn't have a __ContextUpgradeable_init function to be called here?

[minghinmatthewlam]

ContextUPgradeable has no state to initialize with so the init functions are empty

    function __Context_init() internal onlyInitializing {
    }

    function __Context_init_unchained() internal onlyInitializing {
    }

[michaelkaplan13]

Gotcha, should we include calls to that here for completeness? In the event that state is added to those contracts in a future version I image we'd want to call it.

[minghinmatthewlam]

still need to follow up this. If you include the Context init call, for new versions we'd need to check through the inheritance hierarchy to make sure there's no duplicates of initialize calls anyways. Likely if called, we can call the init_unchained call. Also was looking at OZ's OwnableUpgradeable which also inherits ContextUpgradeable but they do not call the Context init function, so might ask OZ about that.

[minghinmatthewlam]

following convention in #397 (comment) to call init instead of unchained when not needed, went forward to call Context_init as well for completeness, and no duplicate initializations.

[minghinmatthewlam]

Related OZ issue with their upgradeable contract transpiler not always calling the init functions of parent contracts OpenZeppelin/openzeppelin-contracts#4690

[michaelkaplan13]

Bumping this comment that may have fallen through the cracks: #397 (comment)

[feuGeneA]

Do we need to call _disableInitializers() somewhere? https://docs.openzeppelin.com/upgrades-plugins/1.x/writing-upgradeable#initializing_the_implementation_contract

Do we plan to integrate OZ's Upgrades Plugins? In particular, the "upgrade safety validations" sound helpful.

[geoff-vball]

LGTM once Gene's comments are addressed

[minghinmatthewlam]

Do we need to call _disableInitializers() somewhere? https://docs.openzeppelin.com/upgrades-plugins/1.x/writing-upgradeable#initializing_the_implementation_contract

Do we plan to integrate OZ's Upgrades Plugins? In particular, the "upgrade safety validations" sound helpful.

I deferred the _disableInitializer to the non-upgradeable extensions Issue/PR, but yes they should be added to constructors of upgradeable contracts.

Also I agree that now we have upgradeable contracts, integrating the plugin will be beneficial especially after moving to mono-repo structure. Can you create a separate issue for it @feuGeneA