Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

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

Merged
merged 2 commits into from
May 21, 2024

Conversation

SidestreamColdMelon
Copy link
Contributor

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
Copy link
Contributor Author

SidestreamColdMelon commented Feb 5, 2024

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
Copy link
Contributor

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.

mattsse pushed a commit to foundry-rs/compilers that referenced this pull request Feb 13, 2024
This PR implements two small flattener features mentioned in
makerdao/spells-mainnet#391 (comment)

1. Combine pragmas from different sources into one pragma so that
version restrictions are kept the same before and after flattening
2. Comments specifying original source file of the flattened code (`//
src/File.sol`)
@SidestreamColdMelon
Copy link
Contributor Author

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
Copy link
Contributor

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
Copy link
Contributor

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
Copy link
Contributor Author

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 SidestreamColdMelon marked this pull request as ready for review May 6, 2024 12:26
@SidestreamColdMelon
Copy link
Contributor Author

SidestreamColdMelon commented May 6, 2024

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
Copy link
Contributor

amusingaxl commented May 8, 2024

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.

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I couldn't find any other differences than already reported ones hence approve

@SidestreamColdMelon SidestreamColdMelon merged commit 7d51a28 into master May 21, 2024
3 checks passed
@SidestreamColdMelon SidestreamColdMelon deleted the use-forge-for-flattening branch May 21, 2024 08:30
@SidestreamColdMelon SidestreamColdMelon changed the title Replace dapp tools with foundry for spell flattening Refactor: replace dapp-tools with foundry for spell flattening May 21, 2024
@SidestreamColdMelon SidestreamColdMelon mentioned this pull request May 30, 2024
15 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants