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

Type error when using verifyUpgrade with constructorArgs options #647

Open
cygnusv opened this issue Aug 29, 2022 · 6 comments
Open

Type error when using verifyUpgrade with constructorArgs options #647

cygnusv opened this issue Aug 29, 2022 · 6 comments

Comments

@cygnusv
Copy link
Contributor

cygnusv commented Aug 29, 2022

I'm trying to upgrade a contract that was initially deployed with a constructor on the implementation, and particularly, with arguments via the constructorArgs option. The new implementation will use the same pattern, but we're facing now that upgrades.verifyUpgrade() doesn't allow us to use constructorArgs as an option:

  Overload 1 of 2, '(origImplFactory: ContractFactory, newImplFactory: ContractFactory, opts?: ValidationOptions): Promise<void>', gave the following error.
    Argument of type 'string' is not assignable to parameter of type 'ContractFactory'.
  Overload 2 of 2, '(proxyOrBeaconAddress: ContractAddressOrInstance, newImplFactory: ContractFactory, opts?: ValidationOptions): Promise<...>', gave the following error.
    Argument of type '{ constructorArgs: string[]; }' is not assignable to parameter of type 'ValidationOptions'.
      Object literal may only specify known properties, and 'constructorArgs' does not exist in type 'ValidationOptions'.

Note that disabling the TypeScript check with // @ts-ignore: works, but I'd like to avoid this.

@frangio
Copy link
Contributor

frangio commented Aug 30, 2022

We don't have a function called verifyUpgrade. Do you mean validateUpgrade?

@cygnusv
Copy link
Contributor Author

cygnusv commented Aug 30, 2022

🤦‍♂️Sorry, I meant validateUpgrade

@frangio
Copy link
Contributor

frangio commented Aug 30, 2022

Are you sure that it is necessary to pass constructorArgs for validateUpgrade to work correctly?

@cygnusv
Copy link
Contributor Author

cygnusv commented Aug 31, 2022

If I don't pass them, it fails with the following error:

Error: types/values length mismatch (count={"types":6,"values":0}, value={"types":[ ... ],"values":[]}, code=INVALID_ARGUMENT, version=abi/5.7.0)

    at Logger.makeError (/Users/david/dev/t/solidity-contracts/node_modules/@ethersproject/logger/src.ts/index.ts:269:28)
    at Logger.throwError (/Users/david/dev/t/solidity-contracts/node_modules/@ethersproject/logger/src.ts/index.ts:281:20)
    at AbiCoder.encode (/Users/david/dev/t/solidity-contracts/node_modules/@ethersproject/abi/src.ts/abi-coder.ts:101:20)
    at Interface._encodeParams (/Users/david/dev/t/solidity-contracts/node_modules/@ethersproject/abi/src.ts/interface.ts:323:31)
    at Interface.encodeDeploy (/Users/david/dev/t/solidity-contracts/node_modules/@ethersproject/abi/src.ts/interface.ts:327:21)
    at getDeployData (/Users/david/dev/t/solidity-contracts/node_modules/@openzeppelin/hardhat-upgrades/src/utils/deploy-impl.ts:49:45)
    at async Proxy.validateUpgrade (/Users/david/dev/t/solidity-contracts/node_modules/@openzeppelin/hardhat-upgrades/src/validate-upgrade.ts:49:26)
    at DeploymentsManager.executeDeployScripts (/Users/david/dev/t/solidity-contracts/node_modules/hardhat-deploy/src/DeploymentsManager.ts:1223:19)
    at async DeploymentsManager.runDeploy (/Users/david/dev/t/solidity-contracts/node_modules/hardhat-deploy/src/DeploymentsManager.ts:1053:5)
    at async SimpleTaskDefinition.action (/Users/david/dev/t/solidity-contracts/node_modules/hardhat-deploy/src/index.ts:409:5)
    at async Environment._runTaskDefinition (/Users/david/dev/t/solidity-contracts/node_modules/hardhat/src/internal/core/runtime-environment.ts:219:14)
    at async Environment.run (/Users/david/dev/t/solidity-contracts/node_modules/hardhat/src/internal/core/runtime-environment.ts:131:14)
    at async SimpleTaskDefinition.action (/Users/david/dev/t/solidity-contracts/node_modules/hardhat-deploy/src/index.ts:555:32)
    at async Environment._runTaskDefinition (/Users/david/dev/t/solidity-contracts/node_modules/hardhat/src/internal/core/runtime-environment.ts:219:14)
    at async Environment.run (/Users/david/dev/t/solidity-contracts/node_modules/hardhat/src/internal/core/runtime-environment.ts:131:14)
    at async SimpleTaskDefinition.action (/Users/david/dev/t/solidity-contracts/node_modules/hardhat-deploy/src/index.ts:640:5)
    at async Environment._runTaskDefinition (/Users/david/dev/t/solidity-contracts/node_modules/hardhat/src/internal/core/runtime-environment.ts:219:14)

@frangio frangio changed the title TSError when using verifyUpgrade with constructorArgs options Type error when using verifyUpgrade with constructorArgs options Aug 31, 2022
@sujantkumarkv
Copy link
Contributor

sujantkumarkv commented Jan 30, 2023

Hey there!

  • Can you please give me more details about the contract code, and
  • How to reproduce the above error when not using constructorArgs?
    @cygnusv @frangio @ericglau
  • Also, if I read the docs correctly, it specifically talks of constructors, not being allowed in upgradeable contracts. It uses proxy and delegates all calls to the implementation contract so the constructor is basically absent for the proxy.
  • The solution by openzeppelin is using an initialize() function with an initalizer modifier. So @cygnusv , I suppose maybe the contract code needs modifications? The docs also talks of intializers & contracts with multiple inheritances, here.

@ericglau
Copy link
Member

@sujantkumarkv
Here is an example that can reproduce this:

  1. Go to https://wizard.openzeppelin.com/, enable Upgradeability and choose UUPS.
  2. Click Download -> Development Package (Hardhat)
  3. Unzip the downloaded file, and run npm install in the unzipped folder.
  4. Change the contract's constructor to have an argument, e.g.
    /// @custom:oz-upgrades-unsafe-allow constructor
    constructor(uint256 a) {
        _disableInitializers();
    }
  1. Change test/test.ts in the unzipped folder to just
import { ethers, upgrades } from "hardhat";

describe("MyToken", function () {
  it("Test contract", async function () {
    const ContractFactory = await ethers.getContractFactory("MyToken");

    await upgrades.validateImplementation(ContractFactory);
  });
});
  1. Run npm test

Initializers are generally used instead of constructors for upgradeable contracts. However, constructors could still be included which run only for the implementation contract itself, and this can be used to set immutable variables in some cases. We also recommend having a constructor with _disableInitializers(); to disable initialization of the implementation contract itself.

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

No branches or pull requests

4 participants