Suggestions

[Ramarti]

Hi all,

Thanks a lot for bringing OZ storage layout checks to foundry. However, the current way this library works makes hard to actually use Upgrades helper in tests, due to the need to add --ffi and that it often requires a clean full compilation, it hurts dev x.

My suggestion is that there is no need to map 1:1 the behavior from the hardhat version.

It feels unnecessary to pass the contract name in a string (and pass constructor arguments inside Options struct), have the upgrades helper load the info, then run storage checks through --ffi only in that contract, every contract.

In foundry, it's no big drama to just deploy the implementation contract and pass the address, something like

(TestProxyHelper just deploys an ERC1967)

address impl = address(new RoyaltyModule());
royaltyModule = RoyaltyModule(
    TestProxyHelper.deployUUPSProxy(
        impl,
        abi.encodeCall(
            RoyaltyModule.initialize, (
                address(governance)
            )
        )
    )
);

For the storage check, I'm going to have a base script that runs npx @openzeppelin/upgrades-core validate <build-info-dir> --requireReference

Anyway, hope this feedback is useful, thanks again!

[ericglau]

Hi @Ramarti, thanks for the feedback. I agree with your points, but would like to share why the current design was chosen.

The current behavior was designed to allow upgrade safety validations to run by default, to minimize the likelihood of unsafe deployments. Passing in the contract name is needed so that we can programmatically identify the requested implementation contract, and use that identifier to 1) run validations, and 2) get the contract bytecode using vm.getCode for deployment via either a create operation or through Defender.

Similarly, even for the proxy contracts themselves, the internal functions of the library also retrieve their bytecode by contract name, so that they could be deployed through Defender if needed. This would allow users to use a salt with Defender deployments in order to get deterministic addresses on different chains.

For tests, is automation (i.e. ease of deploying proxies) the only scenario that you are interested in? I see it could be useful to add additional functions such as what you proposed, to help automate proxy deployments with an existing implementation address. It would be up to the user to ensure that they are using an implementation that is upgrade safe, and this would need to be documented clearly to avoid accidental misuse.