Add support for constructor in logic holding contracts.

[Amxx]

When deploying the logic contract for a proxy, you are expected to replace the constructor with an initialize function, for the part that is instance-specific (let's say the name and symbol of an ERC20). This is verified by the plugin, which will refuse to work with implementations that have non-trivial constructors

However, it can still make sense to have a constructor that sets an immutable variable (such as the address of a forwarder for the ERC2771Context). That way the logic contract can "hardcode" the forwarder in an immutable variable, and then all instances use the same forwarder out of the box, without having to initialize it.

Therefore, I think it would be useful to have a way to bypass this check and provide constructor arguments for the logic contract.

[frangio]

There are two components to this:

1. Bypassing the check and allowing constructors through. Ideally we'd do this automatically in an intelligent way, possibly by checking that the constructor doesn't write to storage, but this complicates the feature and we can start by offering an override flag for people to manually check this themselves. See Add a manual override for every check #280.
2. Providing arguments for implementation constructors. There is no place in the current API where people could provide constructors for deploying the implementation.

[frangio]

Bypassing the check is now implemented, and basic immutable variables should work with the appropriate overrides:

/// @custom:oz-upgrades-unsafe-allow state-variable-immutable
address immutable self = address(this);

Empty constructors should also be possible:

/// @custom:oz-upgrades-unsafe-allow constructor
constructor() {
    // set immutable vars here?
}

But it's still not possible to provide arguments to the constructor.

[lbeder]

We also encountered this issue when we need to provide constructor arguments to initialize immutable state variables. For example, would it be possible to add an optional ctorArgs argument to the deployProxy method to support it?

I also tried to hook ContractFactory's deploy method with a method supplying it. Still, since the framework treats the factory as a singleton - I can't initialize it with different values multiple times (e.g., during tests, when there's a deployProxy call in a beforeEach callback).

[frangio]

@lbeder Yes this is possible, but I don't know when we'll be able to work on it.

[lbeder]

I totally understand. If I'd create such a PR (converging both truffle and hardhat, UUPS and TransparentProxy, and full tests) - will this be something you'd consider merging?

[Amxx]

@lbeder provided that it follows the style and logic of the rest of the code, we would definitely consider it !

[lbeder]

Thanks :)

[lbeder]

@Amxx I've made good progress, but I'm currently stuck on some the last stage and contemplating how to proceed.

Everything is hunky-dory, other than the fact that the calculation of the "version" of the logic in getVersion(unlinkedBytecode, ImplFactory.bytecode) doesn't take into account that immutable state variables are going to be backpatched in the bytecode, thus it caches the first deployed version and keeps returning it for subsequent initialization with different init values.

For example, given the following test contract:

contract ImmutableStateVariable {
  uint public immutable x;

  constructor(uint _x) public {
    x = _x;
  }
}

And the following test (please ignore the new arguments spec for a moment):

const test = require('ava');

const { ethers, upgrades } = require('hardhat');

test.before(async t => {
  t.context.ImmutableStateVariable = await ethers.getContractFactory('ImmutableStateVariable');
});

test('initialize immutable state variables', async t => {
  const { ImmutableStateVariable } = t.context;
  const instance = await upgrades.deployProxy(ImmutableStateVariable, [], {
    kind: 'transparent',
    ctorArgs: [42],
    unsafeAllow: ['constructor', 'state-variable-immutable'],
  });
  t.is((await instance.x()).toString(), '42');

  const instance2 = await upgrades.deployProxy(ImmutableStateVariable, [], {
    kind: 'transparent',
    ctorArgs: [100],
    unsafeAllow: ['constructor', 'state-variable-immutable'],
  });
  t.is((await instance2.x()).toString(), '100');
});

The last t.is((await instance2.x()).toString(), '100') check fill fail, as it's going to reuse the cached version from the first run.

Can you think of a quick way to take immutable state variables into account without rewriting version.ts?

[lbeder]

You can find it here: https://github.com/OpenZeppelin/openzeppelin-upgrades/compare/master...lbeder:ctor-args?expand=1 and other than the issue with the caching of the logic contract (which causes the tests to fail) - it's good for review.

[frangio]

Thanks for tackling this! Making this work with immutable variables is going to be a lot more effort. Can you add a test using upgradeProxy to see if it works?

This is just a rough idea but I think we should store the ABI-encoded constructor arguments for an impl deployment in the network manifest. When we look up an existing deployment to see if we can reuse it, we should also consider the constructor arguments and if they don't match, we should deploy a new one.

[lbeder]

Oh, I missed that. I'll definitely add a test using upgradeProxy 😅 .

I'll try the ABI-encoded constructor arguments key approach. Good idea!