OpenZeppelin / openzeppelin-upgrades

Plugins for Hardhat and Foundry to deploy and manage upgradeable contracts on Ethereum.
MIT License
627 stars 267 forks source link

Validations fail when importing artifact from external package #402

Open PaulRBerg opened 3 years ago

PaulRBerg commented 3 years ago

Description

When I import a Hardhat artifact from an external package, the OpenZeppelin upgrades plugin does not make the deployment because it fails to validate the cache. But there is no cache to validate because I am not compiling that contract locally.

The solution would be to let us disable the cache validation via a flag, so that we can use @openzeppelin/hardhat-upgrades with external Hardhat artifacts.

Error

ValidationsCacheNotFound [Error]: Validations cache not found. Recompile with `hardhat compile --force`
    at Object.readValidations (/Users/paulrberg/workspace/hifi/deployers/node_modules/@openzeppelin/hardhat-upgrades/src/utils/validations.ts:42:13)
    at Object.deployImpl (/Users/paulrberg/workspace/hifi/deployers/node_modules/@openzeppelin/hardhat-upgrades/src/utils/deploy-impl.ts:27:23)
    at Proxy.deployProxy (/Users/paulrberg/workspace/hifi/deployers/node_modules/@openzeppelin/hardhat-upgrades/src/deploy-proxy.ts:57:18)

Environment

See my deployer script. The Hardhat artifact is imported from an external package @hifi/protocol.

frangio commented 3 years ago

Thanks for the issue. This cache is actually important because that's where the result of validations (i.e. safety checks) are stored. This is what ensures the safety of the deployment and subsequent upgrades.

These checks run on the source code, so we need it to be part of compilation. The plugin currently will not work with contracts imported via ABI and bytecode as in your script. You will need to import the contracts in your local Solidity files.

Do you want to completely ignore all safety checks?

PaulRBerg commented 3 years ago

Yes, I would like to be able to disable the safety checks for testing purposes, or at least when deploying the logic contract for the first time (i.e. when not upgrading anything). I.e. when calling deployProxy, but not when upgradeProxy. What do you think?

frangio commented 3 years ago

at least when deploying the logic contract for the first time (i.e. when not upgrading anything)

The issue here is that if you want safety checks in future upgrades, we also need to look at the source code (i.e. we need the cache) on the first deploy, because that's when we extract storage layout. We have plans to be able to extract storage layout later, if the source code is still available, but I wouldn't recommend relying on that generally because it will be easy to introduce small changes that would make the contract no longer a match (e.g. changing compiler version, or in your case the hifi/protocol dependency).

Opting out of safety checks in the first deploy would generally imply opting out of safety checks in future upgrades as well.

For testing purposes it would be ok.

Just as a reminder, I believe a good workaround is to import the contract in your project Solidity files. Does this work for you?

PaulRBerg commented 3 years ago

I believe a good workaround is to import the contract in your project Solidity files.

Not really, because the hifi-deployers package is meant to be a standalone repo for deploying contracts.

Anyways, I understand your rationale here, and the security implications of disabling the safety checks. I will proceed with deploying our upgradeable contracts from the core repository. Thanks for your help!

PaulRBerg commented 3 years ago

An idea: what if we point the OZ Hardhat plugin to the source code of the contract in an npm package, and also provide a Solidity settings object as defined by Hardhat? Would this, in theory, be a good enough workaround for the lack of the cache folder?

frangio commented 3 years ago

In theory it would work, but would be too difficult given the current architecture of the project. I'll take note as something that we'll probably need to solve in the future.