Add redeployImplementation option

[ericglau]

Adds the following option: redeployImplementation: ("always" | "never" | "onchange") Determines whether the implementation contract will be redeployed. Defaults to "onchange".

• If set to "always", the implementation contract is always redeployed even if it was previously deployed with the same bytecode. This can be used with the salt option when deploying a proxy through OpenZeppelin Platform to ensure that the implementation contract is deployed with the same salt as the proxy.
• If set to "never", the implementation contract is never redeployed. If the implementation contract was not previously deployed or is not found in the network file, an error will be thrown.
• If set to "onchange", the implementation contract is redeployed only if the bytecode has changed from previous deployments.

Deprecates useDeployedImplementation which is equivalent to redeployImplementation = "never"

[frangio]

I'm confused by a couple of things here.

1. Why would the default change?
2. Was the default really true? I thought our default was always the onchange behavior.

See my other comment. I think we should be returning an options object that is self-consistent. Can we return an object without the useDeployedImplementation field?

[frangio]

I see now that if the user set useDeployedImplementation in their config we want to refer to that in the error message. This makes sense.

Perhaps the way it's implemented currently is the best option, let me know what you think. My comment was because I would expect the deprecated option to be handled in this location instead of in the other places.

[ericglau]

Re: original default being true - that was a bug but had no consequence because we never actually used the default value from withDefaults.

In deployImpl, we use the opts param instead of deployData.fullOpts, where only deployData.fullOpts was populated by withDefaults.

I agree we could remove useDeployedImplementation from withDefaults, but this function's return signature is currently Required<UpgradeOptions> so we would need to get around that.

Technically the default of redeployImplementation = onchange seems kind of pointless as well -- we could equally dictate that undefined means "on change" and not allow an explicit onchange value. It does help to make it clearer though. But (similar to useDeployedImplementation above) is there any benefit in setting redeployImplementation = onchange in withDefaults if we never use this default value?

[ericglau]

Leaving the defaults as-is but improved the way we consume fullOpts (coming from withDefaults) and opts

[frangio]

I think we need to be handling this differently... Possibly in the withDefaults function. Here we should be able to assume that options have "integrity".

[ericglau]

Consider the following:
User specifies useDeployedImplementation = true but does not specify redeployImplementation in their script, so it is undefined.
If we use withDefaults to treat redeployImplementation as being onchange, then that will ALWAYS conflict with useDeployedImplementation = true.
So useDeployedImplementation and redeployImplementation need to be mutually exclusive and we should detect this as above.

The only benefit of allowing these to have integrity would be to allow useDeployedImplementation = true and redeployImplementation = never at the same time, but there is no need for this.

[frangio]

Ideally, if the user specifies salt, we only redeploy if the existing implementation was deployed with another salt. But I understand that we may not want to implement this right now. What do you think about forcing the user to use redeploy: 'always' when they specify a salt? If we do this, we can change the default behavior in the future.

[ericglau]

What do you think about forcing the user to use redeploy: 'always' when they specify a salt

I don't think we should do this. Users may want to run proposeUpgrade with a salt. If they run it again with the same salt (for example, they want to propose an upgrade for another proxy), forcing redeploy: 'always' would result in a deployment error for the implementation because the resulting address would be the same.