openzeppelin-foundry-upgrades icon indicating copy to clipboard operation
openzeppelin-foundry-upgrades copied to clipboard

How to Upgrade without Changing Name or Creating New Contract ?

Open obumnwabude opened this issue 1 year ago • 7 comments

All the examples make it look like to upgrade a contract, you are changing the proxy to a new implementation address whose Contract name is different from the old implementation. (BoxV2 against Box).

But how do you make changes to the old implementation without changing its name and successfully upgrade the proxy?

For example, you fix a simple logic in one line of code in Box and then you want the proxy to reflect this. But then following the provided steps don't work. Even after setting unsafeSkipAllChecks to false in opts.

So how do you upgrade without changing name?

obumnwabude avatar Jun 14 '24 23:06 obumnwabude

You could use the same contract name in a differently named Solidity file, then refer to that contract using its fully qualified contract name in the format FILE_PATH:CONTRACT_NAME. For example, "contracts/BoxV2.sol:Box"

The purpose of requiring both versions of the contract is to allow storage layout validations to be run.

ericglau avatar Jun 18 '24 17:06 ericglau

Would it be possible to download the source from etherscan/polygonscan/etc and use that automagically as a comparison? Similar to what the truffle debugger did with the --fetch-external flag. I mean, would something like that somehow be considered as an addition to a roadmap?

tomw1808 avatar Jul 11 '24 06:07 tomw1808

@tomw1808 I think that makes sense as an enhancement and we will consider that as part of this issue -- we have a similar enhancement in mind for Hardhat in https://github.com/OpenZeppelin/openzeppelin-upgrades/issues/598

ericglau avatar Jul 11 '24 14:07 ericglau

ohhh so basically right now we need both contracts.

i was expecting it to run a comparison with the previous implementation, but it does make sense since if the contract has never been deployed there would be nothing to check against, same with using the same contract.

i think a cache/artifact system eill be a good idea to store previous versions to check against for upgrades where it stores the previous versions that oz foundry would then use to compare the current iteration (this artifact MUST be included in the commit since it is needed in future upgrades), doing it like this will make it easier to have fewer files while also allowing the oz checks to work.

now the problem here would be the changes mid development if something changed and created a conflict with the old builds (uncommited local)

ex.

  • commited build
  • changes made 1
  • conflicting change made for change 1

if so there should be a command to revert the changes for the artifacts to the commited build to clear the conflicts (which should be doable with git)

yeah now that i think about this enough it would be complicated to implement although it would be easier for the development side since there would be fewer files

what i was doing was just adding a public constant var for the contract version instead of a new implementationV2 and hoping the check would be smart enough to compare the changes from the previous contract state 😂which is foolish hahaha

Metaxona avatar Jul 11 '24 15:07 Metaxona

So, I know that is very specific to our own project, but I made a sort of prototype now for a release pipeline. Not that we have so many releases, but I still like to have a functioning reassurance that I'm not overwriting anything in production that shouldn't be overwritten. I also know that doesn't apply to every project, ours is a bit bigger and dates back before hardhat even existed, so there was a lot of porting going on over the years. Anyways I ended up with something like this:

  1. downloading the existing contracts from polygon explorer
  2. opening anvil with a polygon mainchain fork and overwrite some storage slots so I don't need the hardware wallet to deploy new versions of a proxy
  3. verifying the new contracts against the previous version
  4. deploying them to the forked chain and running some unit tests
  5. deploy everything using frame wallet with the ledger

So that would be what I will end up with. The main scripts are actually really not a lot, however its finicky and of course if would be better if there was some sort of toolchain that supported this somehow with a standard workflow...

Here's the script to download the polygon contracts (mind our own folder structure comes from truffle before): https://github.com/Morpher-io/MorpherProtocol/blob/account-abstraction/helpers/chainfork/dl_source.js

and here's the script that runs everything, the script, then anvil and then overwrites the storage and then runs forge script to validate the contracts (and then run the unit tests in the future - work in progress): https://github.com/Morpher-io/MorpherProtocol/blob/account-abstraction/helpers/chainfork/start.sh

May it help someone out there...

tomw1808 avatar Jul 17 '24 06:07 tomw1808

@tomw1808 I think that makes sense as an enhancement and we will consider that as part of this issue -- we have a similar enhancement in mind for Hardhat in https://github.com/OpenZeppelin/openzeppelin-upgrades/issues/598

if it's going to be fetched from an external source i think adding a custom natspec that indicates the chain, address and or name of the reference contract implementation would be good since not only will the deployed next version implementation would have a reference pointing to that contract itself meaning people will be able to see previous versions of the contract with clear reference, it would also make it easier on the development side since developers would just need to add a natspec for it without adding more contracts for new implementation versions.

the problem i am seeing with this is which would be used to check the code?

address().code (i don't know if it is possible to use this for storage layout verification)

or

block explorer api for fetching the source code (this would cost money since it is usually under pro tier feature)

maybe there are other ways to do it too

Metaxona avatar Jul 17 '24 11:07 Metaxona

@Metaxona

i think a cache/artifact system eill be a good idea to store previous versions to check against for upgrades

For Hardhat, we do exactly that. Hardhat Upgrades uses network files to save the storage layout of deployed contracts, which are used for comparison during later upgrades. We decided not to use this approach for Foundry for the following reasons:

  1. Unlike Hardhat, Foundry scripts first run as a local simulation on a local EVM. A later phase of forge script can optionally broadcast some of the transactions that were simulated. From the Foundry script, I am not aware of a way to properly determine if the current part of the code is being broadcast (and even if it is, the script may still fail at a later step). Therefore, we cannot conclusively know when a deployment has occurred to a real network before saving the storage layout.
  2. The maintenance of network files has been an issue for some Hardhat users in the past, since we recommend committing them to source control but users may sometimes forget to do so or lose certain versions of those files accidentally. We determined that network files should not be used in the Foundry plugin's design to avoid these kinds of issues that users may face.

if it's going to be fetched from an external source i think adding a custom natspec that indicates the chain, address and or name of the reference contract implementation would be good

Supposedly the chain and address can already be determined from how the user is calling the upgrade function (via the connected network and the address parameter), but perhaps which specific explorer to get source code from needs to be provided somewhere.

@tomw1808

I made a sort of prototype now for a release pipeline

Thanks for sharing. Another possible future approach may be to keep previous releases in a different branch, and when we support comparisons with other build info dirs, the storage layout from the build info of the previous branch can be used for reference.

ericglau avatar Jul 17 '24 15:07 ericglau

Thanks everyone for the support.

But Please how exactly do you reference the old built contract (with same name) in the oz-upgrade-from comment from build-info?

The build info in foundry are JSON files (with hex names) created per every "broadcast"/run. In the contract's comment, how exactly does OpenZeppelin expect the build-info? All the examples only show a "build-info-v1" and there is nothing like that in these generations.

Yes, in foundry, the build-info is at out/build-info. Does OpenZeppelin want something like:

/// custom:oz-upgrades-from out/build-info/ff2233dd99ee3333dd00111:ContractName

Or should ".json" come after the build-info directory and before the colon character (:) ?

Either way, have tried upgrading by doing just the above and I'm getting the following error:

revert: Failed to run upgrade safety validation: /home/dev/.npm/_npx/e9c2fe9985ed1095/node_modules/@openzeppelin/upgrades-core/dist/cli/validate/build-info-file.js:127
            throw new error_1.ValidateCommandError(`Build info file ${buildInfoFilePath} is not from a full compilation.`, () => PARTIAL_COMPILE_HELP);
                  ^

ValidateCommandError: Build info file out/build-info/149c17b8b72c52ffd0422189ea532559.json is not from a full compilation.

Recompile all contracts with one of the following commands and try again:
If using Hardhat: npx hardhat clean && npx hardhat compile
If using Foundry: forge clean && forge build
    at /home/dev/.npm/_npx/e9c2fe9985ed1095/node_modules/@openzeppelin/upgrades-core/dist/cli/validate/build-info-file.js:127:19

I am also noticing that the linked out/build-info/149c17b8b....json build is the newly generated one for that forge script call. Why is forge saying that this its most recent build-info is not a full compilation?

Furthermore, doing the recommended forge clean && forge build is highly misleading because those commands actually deletes the out/build-info directory. I suppose we shouldn't be deleting those directories now that we need them for reference of upgrades. Maybe that should be a new issue, that build-infos shouldn't be deleted when forge clean is run. I may be wrong here.

Was thinking of opening a new issue, but wanted to be sure if I'm not doing the referencing wrongly first. Because so far, I'm still not able to upgrade without changing contract name or without relisting the same contract but in a different file. Didn't try the referenceBuildInfoDir because I also don't know how it expects its value.

obumnwabude avatar Jan 04 '25 22:01 obumnwabude

@obumnwabude After you have built the first version of your contract, make a copy of the entire out/build-info directory (including all of its JSON files) and store it in a new directory somewhere else. Then pass in the path to that new directory using opts.referenceBuildInfoDir.

For example,

  1. Build the project containing the first version of your contract.

  2. Copy everything from out/build-info to a new directory called build-info-v1 in the root of your project.

  3. Write the new version of your contract and set the following annotation in the new version:

/// @custom:oz-upgrades-from build-info-v1:MyContract
contract MyContract {
  ...
}
  1. Set the referenceBuildInfoDir option in your script for the upgrade or validate function:
        Options memory opts;
        opts.referenceBuildInfoDir = "build-info-v1";

        Upgrades.validateUpgrade(
            "MyContract.sol",
            opts
        );
  1. Run forge script --force ... -- this will actually delete the out/build-info directory and will rebuild everything for the new version of your contract. This is expected, since you have the old version in build-info-v1.

You can find some examples in this test script which uses these test projects.

ericglau avatar Jan 07 '25 16:01 ericglau

Oh Wow, thanks so much for the detailed explanations. Will try it and I hope it works. Step 2 above is what is in missing in the docs. Please also add it to the docs that out/build-info should be copied out inorder to help others. Thanks again.

obumnwabude avatar Jan 11 '25 21:01 obumnwabude