Upgradability Repo Structure

[minghinmatthewlam]

Context

Currently in teleporter and avalanche-interchain-token-transfer we have non-upgradeable contracts. However, we plan on adding upgradability to the Avalanche ICTT, this means contract changes required to make them upgrade compatible.

Upgrade Contract Changes

• Cannot use constructor, instead uses an initialize function that can only be called once by proxy.
  • Immutable variables cant be set in constructor
  • Contracts in inheritance hierarchy also need to be upgradeable and not use constructor
  • If multiple contracts in inheritance derive off same contract, need to make sure only call initialize function once for each contract
• Call _disableInitiailizer in logic contract constructor for security
• Storage collision of state variables, use gap variables or namespace storage.
  • New variables need to be added at end, ordered
  • No removing old variables
  • Does not apply to functions/events
• No selfdestruct delegatecall in logic contract

Repo Structure

Option 1: Non-upgradeable Extension

The core of the contracts would be upgrade compatible, and the non-upgradeable versions of the contracts would simply inherit from the upgradeable version and call their initialize function.

contract NonUpgradeableExample is UpgradeableExample {
	constructor() UpgradeExample() {
		initialize();	
}
}

Pros:

• Reduce duplicate code
• Easier to maintain for code changes and probably testing

Cons:

• Will need _disableInitiailizers in the constructor of the upgradeable contract for security purposes. To work around this for the non-upgradeable contract extensions, we can take an additional constructor parameter for upgradeable versions like bool disableInitializers.
• To avoid extra _gap variables for non-upgradeable versions, will likely want to use namespace storage instead.
• One option is to use OZ v5 which requires upgrading our contracts to solidity 0.8.20, would need to check the changes required on that.
  • Another option is to stay with v0.8.18 and use namespace storage anyways, which might seem strange since OZ v4 will use _gap variables
  • Final option is just use _gap variables all across, extra costs for non-upgradeable, which is less reasonable when considering the Teleporter utility contracts, and having any Teleporter dApp that inherits TeleporterUpgradeable incur extra costs unnecessarily.
• Need to double check if runtime gas costs are affected too.

Option 2 Separate Non-upgradeable vs Upgradeable Contracts

Pros:

• Both contracts can be optimized (less gas waste for deployment, possibly runtime) for their purpose. This isn’t as much an issue for Avalanche ICTT, but since TeleporterUpgradeable is a utility contract for building Teleporter dApp, not optimal to provide a non-upgradeable version that costs higher gas than needed.
• Easily display and link the associated audit commits

Cons:

• Harder to maintain, new code changes will be needed to be reflected in both contract versions. Including testing.
  More code duplication

Proposal

Proposal is for Avalanche ICTT to use the upgradeable extension in Option 1 for non-upgradeable versions. The primary reason is that any contract change would need to be done in both contracts in option 2, and would need to make sure it’s done correctly. On top of that, since they are completely separate contracts, we should have unit test coverage for both contracts. Whereas for Option 1 with inheritance, code changes automatically apply to both contracts, and test coverage is more likely to cover both contracts.

Also this approach makes more sense if we’d like to push the upgradeable version as the standard, while still providing support for non-upgradeable versions.

The initial workaround for disableInitializer can be to take in an extra parameter in the constructor to specify to disable or not.

Next question is whether we upgrade to OZ v5 and Solidity ^0.8.20 with namespaced storage, use namespaced storage with OZ v4, or just use gap state variables with OZ v4. Proposal option is OZv5 and Solidity ^0.8.20, with the assumption that the audit timeline is August. Reasons being:

• Upgrading dependencies of OZv5 and Solidity versions with possible new features
• No extra _gap variables for higher deployment casts of non-upgradeable versions
• Can have the same convention throughout inherited contracts, and not namespace storage for some contracts, while using gap variables for others.

Teleporter utility contracts had more considerations for splitting the contracts, but at this point don’t feel it’s worth the tradeoff in terms of managing both non-upgradeable and upgradeable versions, with possible higher priority tasks.

Next steps:

• Cut a new release for current non-upgradeable contracts that’s marked as production ready
• Split up the upgradability changes into more modular issues
• Merge PR as initial state of proven upgradability and corresponding e2e test
• Changing Avalanche ICTT contracts to use namespaced storage
• Investigating Solidity 0.8.20 changes needed and OZ v5
• Creating the non-upgradeable contracts as extensions of upgradeable contracts
• All the above applied to Teleporter utility contracts like TeleporterUpgradeable

[ARR4N]

Small comment on bool disableInitializers; an enum like this will be clearer (and hence safer):

enum Initializable {
  Allowed,
  Disabled
}

I'm still thinking through everything else, but wanted to write this down before I forgot.

[cam-schultz]

Would we need _disableInitializers if we went with option 2? Or was the con you noted under option 1 pointing out that we'd only want _disableInitializers in the upgradeable case?

[minghinmatthewlam]

we'd still need _disableInitializers for option 2 for the upgradeable contracts. Yup that's the con mentioned for option 1, but there can be some simpler work arounds.

[ARR4N]

since they are completely separate contracts, we should have unit test coverage for both contracts. Whereas for Option 1 with inheritance, code changes automatically apply to both contracts, and test coverage is more likely to cover both contracts.

Assuming, under Option 2, that both the upgradeable and non-upgradeable versions satisfy the same interface, a single test suite could be used for both.

[minghinmatthewlam]

Yup that's true a majority of their test suites should be the same, but still would lean towards option 1 due to reducing code duplication and easier maintainability for changes.