feat: Verifiable Deployment Calldata script #1415
Conversation
… sharing functions
…erification in a foundry test
pakim249CAL
changed the title
.github/workflows/test-contracts.yml
Outdated
| @@ -47,11 +47,11 @@ jobs: | |||
| working-directory: ./contracts | |||
|
|
|||
| - name: Run tests | |||
| run: forge test -vvv | |||
| run: forge test --no-match-path script/**/*.sol -vvv | |||
There was a problem hiding this comment.
should we also capture this exclusion regex in the package.json's test command definition? also what's the rationale for not running the script tests as part of CI?
There was a problem hiding this comment.
The test I wrote to test the deployment is meant to be ran after a deployment by a verifier (which may not be the deployer!), ingesting environment variables input by the verifier. This would obviously fail in CI without contracts already deployed. And yes, I guess it would need to be excluded in package.json too.
Maybe there is a better way to do this, but I think in general there should be no need to put tests that are in the script/ folder into CI. If we put tests there meant for CI, they should be in the test/ folder.
There was a problem hiding this comment.
I guess by default, I could have the test run after running the deployment script using mock values. But then the test would not fail if the correct environment variables are not supplied, which seems like a way that the verifier might make a mistake.
There was a problem hiding this comment.
Actually, maybe an even better solution is just to make the BeforeInitializaiton test be done in a script instead of a test. Then it won't be flagged as a test by forge test. I think I'll just do that.
There was a problem hiding this comment.
+1 to having it as a part of a script and not a test
| CalldataInitParams memory params = CalldataInitParamsLib.getCalldataInitParams(cfg); | ||
| function _logs() internal virtual { | ||
| console2.log("Deployment addresses:"); | ||
| console2.log("Deployment Initializer: ", address(deploymentInitializer)); |
There was a problem hiding this comment.
nit - inconsistent formatting with other log stmts
| ); | ||
| } | ||
|
|
||
| function _doTests() internal { |
There was a problem hiding this comment.
is this not a redundant simulation with just running the script with --broadcast enabled?
There was a problem hiding this comment.
How so? I don't think I understand this question. Do you mean that this whole fn is redundant? Basically this function is put into the script to run the same tests that the verifier would run to verify the deployment. That way, the deployer has some assurance that the verifier can run the BeforeVerifiableDeploymentInitialization test on the latest block on the deployed chain and have it pass.
There was a problem hiding this comment.
ah I see that makes sense - any chance we could capture this role differentiation in a smol comment? At first glance the behavior is a bit hard to decipher
| vm.startPrank(initialOwner); | ||
| CalldataInitParams memory params = CalldataInitParamsLib.getCalldataInitParams(cfg); | ||
| function _logs() internal virtual { | ||
| console2.log("Deployment addresses:"); |
There was a problem hiding this comment.
just to learn - how does console2 differ from just regular console?
There was a problem hiding this comment.
From the foundry doc,
"console2.sol contains patches to console.sol that allow Forge to decode traces for calls to the console, but it is not compatible with Hardhat."
Why are these changes needed?
Adds a script to get calldata that would be submitted to a multisig, and adds supporting refactors to minimize code duplication and vectors for misconfiguration. Also adds tests for what is testable on-chain, but there is still some verification that will need to be done offchain such as verifying the contracts on etherscan, making sure proxies have a clean history, etc.
The refactors mainly are:
Checks