search

makerdao / spells-mainnet

Staging repo for MakerDAO weekly executive spells
GNU Affero General Public License v3.0
122 stars 50 forks source link

Refactor: replace dapp-tools with foundry for spell flattening #391

Closed SidestreamColdMelon closed 6 months ago

SidestreamColdMelon commented 9 months ago

Description

This PR implements minimal changes required to remove dependency on dapp.tools and replace it with foundry commands (ie hevm with forge, seth with cast).

In order to try out this PR, one have to install a foundry build containing the fix with rewritten implementation of the flattening logic, eg:

foundryup --version nightly-2cb875799419c907cc3709e586ece2559e6b340e

Or a later version that also includes this major fix.

SidestreamColdMelon commented 9 months ago

Current state of the flattening: running make verify inside this PR (with nightly-2cb8757 build of the foundry) and comparing it to the code of the previously deployed spell would produce the following diff: https://www.diffchecker.com/mhQp4BXR/

Spotted differences:

  1. pragma solidity version is different
    • The difference:
      • pragma solidity =0.8.16 >=0.5.12 >=0.8.16 <0.9.0; is produced by the hevm flatten
      • pragma solidity 0.8.16; is produced by the forge flatten
    • Might be ok for us, since the actual compiler_version sent to etherscan for verification is actually loaded from the out/dapp.sol.json (file produced during the compilation), and is currently set to v0.8.16+commit.07a7930e
  2. Comments specifying original source file of the flattened code are missing (eg // lib/dss-exec-lib/src/CollateralOpts.sol)
    • Not a blocker, but nice to have for better readability of the flattened file
  3. Imports code is missing when forge flatten, while commented in when hevm flatten
    • Not a blocker, but nice to have for better readability
  4. Interfaces are not renamed by default to OriginalInterfaceName_1 (eg see ProxyLike)
    • Not a problem, just a design decision from the foundry team
  5. Order of the imported code is different sometimes
    • The difference:
      • When hevm flatten, the order is:
        1. lib/dss-exec-lib/src/CollateralOpts.sol
        2. lib/dss-exec-lib/src/DssExecLib.sol
        3. lib/dss-exec-lib/src/DssAction.sol
        4. lib/dss-exec-lib/src/DssExec.sol
        5. lib/dss-test/lib/dss-interfaces/src/ERC/GemAbstract.sol
        6. lib/dss-test/lib/dss-interfaces/src/dss/VatAbstract.sol
        7. src/DssSpell.sol
      • When forge flatten, the order is:
        1. lib/dss-exec-lib/src/CollateralOpts.sol
        2. lib/dss-exec-lib/src/DssExec.sol
        3. lib/dss-test/lib/dss-interfaces/src/ERC/GemAbstract.sol
        4. lib/dss-test/lib/dss-interfaces/src/dss/VatAbstract.sol
        5. lib/dss-exec-lib/src/DssExecLib.sol
        6. lib/dss-exec-lib/src/DssAction.sol
        7. src/DssSpell.sol
    • Should not be a problem, but I wonder if it affects the order of the opcodes in the binary
amusingaxl commented 9 months ago

Should not be a problem, but I wonder if it affects the order of the opcodes in the binary

It doesn't matter. Each contract or library bytecode is a separate "entity". What the verifier does is to check against all bytecodes generated by the compilation and match it against the contract under verification.

SidestreamColdMelon commented 9 months ago

What the verifier does is to check against all bytecodes generated by the compilation

Do you refer to some known implementation of the verifier that does it this way?

amusingaxl commented 9 months ago

To be honest, I don't know about the implementation details of any verifier tools, but I'm inferring based on how smart contracts are deployed on-chain.

Each contract/library is a separate entity in the state trie, with their own nonce, balance and bytecode.

I'm guessing that what any verifier tool does is:

  1. Compile every contract/library with the provided configuration (this will generate $n$ different bytecodes)
  2. For each generated bytecode, match it against the bytecode from the contract under verification. a. If any of the compiled bytecodes match the one on-chain, we have a winner. b. If all fail, then the process reverts.

The change in the order of declaration of the contracts could potentially just impact the order under each compiled bytecode is compared against the one on-chain, but not the final result.

amusingaxl commented 9 months ago

I'm not fluent in Rust, but Blockscout's verifier – that does roughly what I'm trying to describe – can be found here:

https://github.com/blockscout/blockscout-rs/blob/main/smart-contract-verifier/smart-contract-verifier/src/verifier/all_metadata_extracting_verifier.rs#L58

SidestreamColdMelon commented 7 months ago

Since goerli PR can't proceed (as goerli is not mining any blocks and was officially deprecated), we can try to use sepolia to validate flattening correctness.

SidestreamColdMelon commented 6 months ago

While temporary changing hardcoded network ids in various scripts in order to deploy the spell to Sepolia, I've realised that we first need to deploy ExecLib in order for the deployment command to work, while ExecLib depends on the chainlog to be deployed, which blows up the scope of this PR and doesn't actually help others to review it.

To merge this sooner, my suggestion is to test code flattening functionality locally (i.e. flatten a spell in the PR vs master and compare flattened code or the compiled binaries).

amusingaxl commented 6 months ago

Other than the order of the contracts being different after flattening, the only differences I could find in the latest spell were:

  1. The version declaration is slightly different, however still compatible. version Left: Dapp Tools / Right: Foundry
  2. Interface renaming on naming collision follows a different strategy, apparently: interfaces Left: Dapp Tools / Right: Foundry

Neither of those seems like a blocker, so I'm gonna approve it.