Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We鈥檒l occasionally send you account related emails.

Already on GitHub? Sign in to your account

RFC: Standard for maintainable and backwards-compatible initializer override #194

Open
brunocangs opened this issue Mar 27, 2023 · 0 comments

Comments

@brunocangs
Copy link

馃 Motivation

There is currently no standard or well documented best practices for a use case where upgradeable contracts require chaining initializers as the contract develops. When a contract gets to it's third of fourth initializers it requires multiple calls to each upgrade step as well as being difficult to mark a method to require the latest version.

This proposal aims to provide an interface that can clearly signal when a new major version is deployed as well as center the upgrade path into a single upgrade() function, that will initialize the contract to the latest version wherever it is down the chain.

This pattern becomes increasingly frustrating when using BeaconProxies for user-deployed content, as it would have you require multiple transactions for the user's logic to be brought up-to-date.

馃摑 Details

The proposed contract should:

  • Provide an interface that, when followed, ensures the best upgradability practices.
  • Have text-editor and compile-time hints to requiring a new override to the upgrade function
  • Contain a modifier that prevents a certain function from executing when it is below the required version, even when the underlying implementation contract has been upgraded
  • Be able to upgrade to it's latest version with a single transaction

What I came up with is the following contract, with given rules:

Contract

contract BaseVersion is Initializable {
    function _version() internal view virtual returns (uint8) {
        return 1;
    }

    function version() public view returns (uint8) {
        return _version();
    }

    function upgrade() public virtual {
        _upgradeFrom(_getInitializedVersion());
    }

    function _upgradeFrom(uint8 version_) internal virtual {
        // Silencing warning
        version_;
        revert("BaseVersion: No upgrade function defined");
    }

    modifier requireVersion(uint8 _minimum) {
        require(
            _getInitializedVersion() >= _minimum,
            string(
                abi.encodePacked(
                    "BeaconVersion: Your contract is out of date - needed: ",
                    _minimum,
                    " currently: ",
                    _getInitializedVersion()
                )
            )
        );
        _;
    }
}

contract MajorVersion is BaseVersion {
    function _upgradeFrom(uint8 version_) internal virtual override {
        // Silencing warning
        super._upgradeFrom(version_);
    }
}

At first sight the MajorVersion contract might seem redundant and pointless, but it's only there because it was the way I managed to get this compile-time behaviour when declaring new MajorVersions

image

By following this pattern of declaring versions, the compiler will instruct you correctly.

Here's an extended example, using initializer and reinitializer to define a way to handle upgrades in the proposed way

Implementation
// SPDX-License-Identifier: MIT
import "@openzeppelin/contracts-upgradeable/proxy/utils/Initializable.sol";

pragma solidity ^0.8.0;

contract BaseVersion is Initializable {
    function _version() internal view virtual returns (uint8) {
        return 1;
    }

    function version() public view returns (uint8) {
        return _version();
    }

    function upgrade() public virtual {
        _upgradeFrom(_getInitializedVersion());
    }

    function _upgradeFrom(uint8 version_) internal virtual {
        // Silencing warning
        version_;
        revert("BaseVersion: No upgrade function defined");
    }

    modifier requireVersion(uint8 _minimum) {
        require(
            _getInitializedVersion() >= _minimum,e
            string(
                abi.encodePacked(
                    "BasVersion: Your contract is out of date - needed: ",
                    _minimum,
                    " currently: ",
                    _getInitializedVersion()
                )
            )
        );
        _;
    }
}

contract MajorVersion is BaseVersion {
    function _upgradeFrom(uint8 version_) internal virtual override {
        // Silencing warning
        super._upgradeFrom(version_);
    }
}

// Is base version so doesnt need to implement upgrade methods, should correclty revert
contract ContractV1 is BaseVersion {
    string private someString;

    function __ContractV1_initialize() internal onlyInitializing {
        someString = "Some value";
    }

    function initialize() public virtual initializer {
        __ContractV1_initialize();
    }

    function echoString() public view virtual returns (string memory) {
        return someString;
    }
}

contract ContractV2 is MajorVersion, ContractV1 {
    uint8 private constant v2 = 2;

    function _version() internal view virtual override returns (uint8) {
        return v2;
    }

    function _upgradeFrom(
        uint8 version_
    ) internal virtual override(BaseVersion, MajorVersion) {
        // If is more out-of-date than previous version
        // would never happen in this case cause it would be version 1
        // but placed it here for consistency
        if (version_ < v2 - 1) {
            super._upgradeFrom(version_);
        }
        initializeV2();
    }

    uint256 private someInt;

    function __ContractV2_initialize_unchained() internal onlyInitializing {
        someInt = 11;
    }

    function __ContractV2_initialize() internal onlyInitializing {
        super.__ContractV1_initialize();
        __ContractV2_initialize_unchained();
    }

    function initialize() public virtual override reinitializer(2) {
        __ContractV2_initialize();
    }

    function initializeV2() public reinitializer(v2) {
        __ContractV2_initialize_unchained();
    }

    function echoInt() public view requireVersion(v2) returns (uint256) {
        return someInt;
    }
}

contract ContractV3 is MajorVersion, ContractV2 {
    uint8 private constant v3 = 3;

    function _version()
        internal
        view
        virtual
        override(BaseVersion, ContractV2)
        returns (uint8)
    {
        return v3;
    }

    function _upgradeFrom(
        uint8 version_
    ) internal virtual override(MajorVersion, ContractV2) {
        // If is more out-of-date than previous version
        if (version_ < v3 - 1) {
            ContractV2._upgradeFrom(version_);
        }
        initializeV3();
    }

    string private v3string;

    function __ContractV3_initialize_unchained() internal onlyInitializing {
        v3string = "v3 is here";
    }

    function __ContractV3_initialize() internal onlyInitializing {
        __ContractV2_initialize();
        __ContractV3_initialize_unchained();
    }

    function initialize() public override reinitializer(v3) {
        __ContractV3_initialize();
    }

    function initializeV3() public reinitializer(v3) {
        __ContractV3_initialize_unchained();
    }

    function echoString()
        public
        view
        virtual
        override
        requireVersion(v3)
        returns (string memory)
    {
        return v3string;
    }
}

I've implemented this with three contract version and uploaded them to this repo for reference. It includes unit tests that deploy and upgrades a few BeaconProxies

Conclusion

I'm not attached to the name or implementation, just really wanted to raise this discussion and see the community's thoughts. Would love to hear what everyone thinks.

I don't quite like the amount of boilerplate required, including having to use a constant version declaration on every new contract implementation to avoid bad comparisons on the modifier and upgrade functions

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

1 participant