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’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Unclear error: revert: Failed to deploy contract using constructor data "" #72

Open
yvesbou opened this issue Sep 2, 2024 · 1 comment

Comments

@yvesbou
Copy link

yvesbou commented Sep 2, 2024

I’m using Upgrades.upgradeProxy() to upgrade proxy contracts pointing to new implementations. A vanilla test case works. But I tried to test revoke ownership and then upgrade which should fail, but it fails with an error that I assume is not correct.

I expect OwnableUnauthorizedAccount and I get this error (instead of passing, I assume)

[FAIL. Reason: revert: Failed to deploy contract MyTokenV2.sol:MyTokenV2 using constructor data ""] testRevokingOwnership() (gas: 8237465)

if I don’t expect OwnableUnauthorizedAccount I get this error

[FAIL. Reason: OwnableUnauthorizedAccount(0x7E5F4552091A69125d5DfCb7b8C2659029395Bdf)] testRevokingOwnership() (gas: 8242052)

which is what I expect, if I don't expect the OwnableUnauthorizedAccount error

So why do I get revert: Failed to deploy contract MyTokenV2.sol:MyTokenV2 using constructor data ""] - this doesn't compute with my current understanding

Here are necessary parts of the contracts and tests 👇

contract

I just have one argument in the initializer

contract MyToken is
    Initializable,
    ERC20Upgradeable,
    OwnableUpgradeable,
    ERC20PermitUpgradeable,
    ERC20VotesUpgradeable,
    ERC20BurnableUpgradeable,
    UUPSUpgradeable
{

		...
		
    function initialize(address initialOwner) public initializer {
        __ERC20_init("MyToken", "MTK");
        __Ownable_init(initialOwner);
        __ERC20Permit_init("MyToken");
        __ERC20Burnable_init();
        __ERC20Votes_init();
        __UUPSUpgradeable_init();

        _mint(msg.sender, 1000000 * 10 ** decimals());
    }
    
    ...
   
}

V2 of token

it's reinitialisable, thus I want to call it with an initialiser argument, like the first init

contract MyTokenV2 is
    Initializable,
    ERC20Upgradeable,
    OwnableUpgradeable,
    ERC20PermitUpgradeable,
    ERC20VotesUpgradeable,
    ERC20BurnableUpgradeable,
    UUPSUpgradeable
{
    /// @custom:oz-upgrades-unsafe-allow constructor
    constructor() {
        _disableInitializers();
    }

    function initialize(address initialOwner) public reinitializer(2) {
        __ERC20_init("MyTokenV2", "MTKV2");
        __Ownable_init(initialOwner);
        __ERC20Permit_init("MyTokenV2");
        __ERC20Burnable_init();
        __ERC20Votes_init();
        __UUPSUpgradeable_init();

        _mint(msg.sender, 1000000 * 10 ** decimals());
    }
    
}

setup

function setUp() public {
        // Deploy the token implementation
        MyToken implementation = new MyToken();
        // Define the owner address
        owner = vm.addr(1);

        vm.startPrank(owner);
        // Deploy the proxy and initialize the contract through the proxy
        proxy = new ERC1967Proxy(address(implementation), abi.encodeCall(implementation.initialize, owner));
        vm.stopPrank();

        // remember: Call your contract's functions as normal, but remember to always use the proxy address:
        // Attach the MyToken interface to the deployed proxy
        myToken = MyToken(address(proxy));
        // Emit the owner address for debugging purposes
        emit log_address(owner);
    }

test case

// upgrade should revert, since no ownership
function testRevokingOwnership() public {
        // revoke ownership
        vm.startPrank(owner);
        myToken.renounceOwnership();

        bytes memory data = abi.encodeCall(MyTokenV2.initialize, owner);

        bytes memory expectedRevertData =
            abi.encodeWithSelector(OwnableUpgradeable.OwnableUnauthorizedAccount.selector, owner);

        vm.expectRevert(expectedRevertData); // reverts with another reason (unclear if goal achieved)
        Upgrades.upgradeProxy(address(proxy), "MyTokenV2.sol:MyTokenV2", data, owner);

        // no mint possible (as expected)
        vm.expectRevert(expectedRevertData);
        myToken.mint(owner, 100e18);

        vm.stopPrank();
    }
@yvesbou yvesbou changed the title Not matching error Unclear error: revert: Failed to deploy contract MyTokenV2.sol:MyTokenV2 using constructor data "" Sep 2, 2024
@yvesbou yvesbou changed the title Unclear error: revert: Failed to deploy contract MyTokenV2.sol:MyTokenV2 using constructor data "" Unclear error: revert: Failed to deploy contract using constructor data "" Sep 2, 2024
@ericglau
Copy link
Member

ericglau commented Sep 5, 2024

Upgrades.upgradeProxy consists of two separate calls: one to deploy the new implementation, and another to upgrade the proxy to use that new implementation.

It looks like the error is occurring on the call to deploy the new implementation. Specifically, it might be related to the "Gotcha: Usage with low-level calls" section of https://book.getfoundry.sh/cheatcodes/expect-revert

Since what you want to test is the revert reason during the upgrade, you can break this down into two separate steps: use Upgrade.prepareUpgrade to validate and deploy the new implementation itself, then use the vm.expectRevert before calling the proxy's upgradeToAndCall function.

For example:

    Options memory opts;
    address v2implementation = Upgrades.prepareUpgrade("MyTokenV2.sol:MyTokenV2", opts);

    vm.expectRevert(expectedRevertData);
    proxy.upgradeToAndCall(v2implementation, data);

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

2 participants