Foundry verify-deployed#11540
Conversation
* WIP. * Consitution test that fail with anvil. * Identify part of migration script that causes tests to fail. * Wip: fixing migration script. * Do not rely on ffi mode. * Fix constitution values in anvil migration. * Port old governance tests from Truffle. * Finish porting old governance integration tests to Foundry. * Fix unit tests. * Fix CI. * Fix remappings for CI. * Fix TS deps. * Update comment. Co-authored-by: pahor167 <47992132+pahor167@users.noreply.github.com> * CR. * Combine proxy selectors with contract selectors. * CR. * Isolate common part. * Enabled detailed anvil logging in CI by default. * Fix tests. * CR fixes. * Cleanup merging. --------- Co-authored-by: pahor167 <47992132+pahor167@users.noreply.github.com>
…ns on top of CR14 (#11485) * Update caching version and add Foundry release scripts * prettify * Refactor argument names in make-release scripts for consistency * Refactor getDefaultValueForSolidityType to handle fixed arrays and improve readability * prettify
…1488) * Update README and Foundry configuration for contract verification - Added Foundry verification instructions alongside Truffle in README.md - Updated foundry.toml to include a profile for Truffle compatibility with Solidity 0.5.13 - Clarified parameter descriptions in SortedOracles contract * revert of foundry.toml * API key * Update .gitignore and foundry.toml for improved build configuration and compatibility * Update openzeppelin-contracts8 submodule to v4.9.0 * Refactor foundry.toml: reorganize profiles and update no_match_path and fs_permissions for clarity
* Clean up dead code and refresh README (#11473) * refactor: remove celotool references and related configurations * refactor: remove deprecated configuration files and ignore unnecessary packages * docs: update contributing guide and README links for clarity * refactor: simplify CODEOWNERS and remove metadata-crawler Dockerfile and workflows * refactor: remove obsolete environment configuration files for oracle and staging * refactor: update README for clarity and remove obsolete SETUP.md * refactor: update CODEOWNERS to correct default owner and improve clarity * fix: correct typo in "Resources" heading in README.md * fix: wrap logo section in a div for center alignment * fix: typo in parameter name: newMininumReports → newMinimumReports (#11483) * chore: fix typo in function's name * chore: fix typo in function's name * chore: fix typo in function's name * chore: fix typo in function's name * fix(deps): update dependency mathjs to v7 [security] (#10841) Signed-off-by: renovate[bot] <29139614+renovate[bot]@users.noreply.github.com> Co-authored-by: renovate[bot] <29139614+renovate[bot]@users.noreply.github.com> * Fix typos in governance contracts documentation (#11370) * Update Election.sol * Update ReleaseGold.sol * Update Governance.sol * chore: fix typo in comment (#11498) * fix: typo in release script console messages (#11500) * Update make-release-6-changes.ts * Update make-release-3-changes.ts * chore: fix typo in comments (#11499) * Update IFeeHandler.sol * Update TypedMemView.sol * Update packages/protocol/contracts/common/libraries/TypedMemView.sol --------- Co-authored-by: Mc01.eth <phenom.home@gmail.com> * fix(deps): update bip39 digest to a7ecbfe (#10848) Signed-off-by: renovate[bot] <29139614+renovate[bot]@users.noreply.github.com> Co-authored-by: renovate[bot] <29139614+renovate[bot]@users.noreply.github.com> * chore: fix typos (#11503) * Update EpochManager.sol * Update EpochManager.sol --------- Co-authored-by: Mc01.eth <phenom.home@gmail.com> * fix: typos and improve log clarity in protocol library files (#11502) * Update bytecode.ts * Update proxy-utils.ts * Update registry-utils.ts * Update web3-utils.ts * Fix typos in test contracts and comments for better readability (#11504) * Update recoverFunds.ts * Update utils.sol * Update EpochManager.t.sol * Update EpochManager.t.sol --------- Co-authored-by: Mc01.eth <phenom.home@gmail.com> * chore: fix some typos (#11511) * Update Accounts.sol * Update FeeHandler.sol * Update MultiSig.sol * fix typos (#11510) * Update verify-bytecode.ts * Update deploy_release_contracts.ts * Fix typos in specs for accounts and governance (#11509) * Update accounts.spec * Update goverance_with_dequeue.spec * chore: fixed broken links (#11513) * fixed broken and redirect links in developer_key_publishing.md * fixed broken link in packages/protocol/scripts/devchain.ts * fixed dead link in packages/protocol/README.md * fixed broken link in artifacts/Proxy/Readme.md * Apply suggestions from code review Co-authored-by: Mc01.eth <phenom.home@gmail.com> --------- Co-authored-by: Mc01.eth <phenom.home@gmail.com> * Fix log message in `Migration.s.sol` (#11515) * Update Migration.s.sol * Update Fixidity.t.sol * Update GovernanceSlasher.t.sol * Fix typos and improve NatSpec comments in EpochManager and SortedLinkedList (#11516) * Update SortedLinkedList.sol * Update EpochManager.sol * chore(protocol): fix broken link (#11525) * Fix unit tests on master. (#11512) * Fix unit tests on master. * Revert fixidity typo fix. Bump version of FeeHandler. --------- Co-authored-by: Martín Volpe <volpe@clabs.co> --------- Signed-off-by: renovate[bot] <29139614+renovate[bot]@users.noreply.github.com> Co-authored-by: Victoria <4222953+lvpeschke@users.noreply.github.com> Co-authored-by: Maximilian Hubert <64627729+gap-editor@users.noreply.github.com> Co-authored-by: renovate[bot] <29139614+renovate[bot]@users.noreply.github.com> Co-authored-by: leopardracer <136604165+leopardracer@users.noreply.github.com> Co-authored-by: Afounso Souza <drewsmpk@gmail.com> Co-authored-by: sashass1315 <sashass1315@gmail.com> Co-authored-by: Forostovec <ilonaforostovec22@gmail.com> Co-authored-by: MozirDmitriy <dmitriymozir@gmail.com> Co-authored-by: radik878 <radikpadik76@gmail.com> Co-authored-by: Snezhkko <snezhkodaria38@gmail.com> Co-authored-by: phrwlk <phrwlk7@gmail.com> Co-authored-by: viktorking7 <140458814+viktorking7@users.noreply.github.com> Co-authored-by: Fibonacci747 <albertofibonacci12@gmail.com> Co-authored-by: Cypher Pepe <125112044+cypherpepe@users.noreply.github.com> Co-authored-by: anim001k <140460766+anim001k@users.noreply.github.com> Co-authored-by: GarmashAlex <garmasholeksii@gmail.com> Co-authored-by: Galoretka <galoretochka@gmail.com> Co-authored-by: Martín Volpe <volpe@clabs.co>
* Add tooling for L2 to L1 withdrawal. * CR. * Cursor CR.
* Add verification steps for Foundry & Truffle. * CR.
* Verify contracts for new chain deployment. * Add OpStack verification scripts. * Change hardcoded config to input params. * Cursor CR. * Cleanup default values in comments. * CR.
- Introduced a new bytecode verification system for Foundry, maintaining compatibility with the existing Truffle implementation. - Created utilities for handling Foundry's library linking format and artifact loading. - Implemented a verification script that mirrors the Truffle interface, supporting command-line arguments for build artifacts and network configurations. - Added bash wrapper for executing the verification process. - Updated package dependencies to support new functionalities. This update allows for a seamless transition between Truffle and Foundry verification processes, facilitating gradual migration.
- Added functionality to back up and restore the foundry.toml file during the verification process to preserve profiles. - Cleaned up the script by removing unnecessary code related to Mento core contracts. - Updated the bytecode verification script to write library addresses in a simplified format for better clarity. These changes improve the reliability and maintainability of the deployment verification process.
- Added a debug parameter to various functions to enable detailed logging during bytecode collection and verification processes. - Updated the `collect` method in `LibraryAddressesFoundry` to conditionally log information based on the debug flag. - Enhanced logging in `getLibraryLinkReferences` and `verifyBytecodesFoundry` to provide insights into the verification steps when debugging is enabled. - Modified the verification script to accept a debug option, improving the usability of the verification process. These changes improve the traceability and debugging capabilities of the bytecode verification workflow.
There was a problem hiding this comment.
This PR is being reviewed by Cursor Bugbot
Details
Your team is on the Bugbot Free tier. On this plan, Bugbot will review limited PRs each billing cycle for each member of your team.
To receive Bugbot reviews on all of your PRs, visit the Cursor dashboard to activate Pro and start your 14-day free trial.
| const bytePositions = positions.map(pos => { | ||
| // Remove "0x" prefix (2 chars) then convert byte position to char position | ||
| return pos.start * 2 + 2 | ||
| }) |
There was a problem hiding this comment.
Bug: Bytecode Address Extraction Error
The registerLibraryFromLinkReferences method miscalculates library placeholder character positions. It adds + 2 for an assumed '0x' prefix, but the bytecode string used for address extraction lacks this prefix. This results in incorrect library address extraction, causing linking and verification failures.
Mc01
left a comment
There was a problem hiding this comment.
Overall quite amazing result based on the given context of creation 😂
I would recommend to extensively test such utilities or maybe even generate tests for them 😉
Left 2 cents from my side 💸
| { access = "read", path = "./migrations_sol/"}, | ||
| { access = "read", path = "./governanceConstitution.json"}, | ||
| { access = "read", path = "./artifacts/"}, | ||
| { access = "read", path = "./.tmp/selectors" } |
There was a problem hiding this comment.
Why removal of .tmp/selectors?
There was a problem hiding this comment.
I would rename this file to something like BYTECODE.md or VERIFY.md or anything similar 😉
| build_artifacts?: string | ||
| proposal?: string | ||
| initialize_data?: string | ||
| network?: string | ||
| rpc_url: string | ||
| librariesFile?: string |
There was a problem hiding this comment.
It looks strange to mix underscores (build_artifacts, rpc_url) & camelCase (librariesFile)
| build_artifacts?: string | |
| proposal?: string | |
| initialize_data?: string | |
| network?: string | |
| rpc_url: string | |
| librariesFile?: string | |
| build_artifacts?: string | |
| proposal?: string | |
| initialize_data?: string | |
| network?: string | |
| rpc_url: string | |
| libraries_file?: string |
| alfajores) | ||
| RPC_URL="${RPC_URL:-https://alfajores-forno.celo-testnet.org}" | ||
| ;; | ||
| baklava) | ||
| RPC_URL="${RPC_URL:-https://baklava-forno.celo-testnet.org}" | ||
| ;; |
There was a problem hiding this comment.
Doesn't make sense to include references to alfajores or baklava anymore -> should be replaced with Celo Sepolia
There was a problem hiding this comment.
Too much files are named similarly:
verify-bytecode-foundrybytecode-foundryverify-deployed-foundryverify-bytecode
It makes it quite hard to understand what lies where on first glance 😅
There was a problem hiding this comment.
This might get clearer once Truffle is removed and it makes sense to rename these without the -foundry suffix. verify-bytecode is for verifying bytecode equality, bytecode is a helper library for dealing with bytecodes in general.
| let ignoredContracts = [ | ||
| // This contract is not proxied | ||
| 'TransferWhitelist', | ||
|
|
||
| // These contracts are not in the Registry (before release 1) | ||
| 'ReserveSpenderMultiSig', | ||
| 'GovernanceApproverMultiSig', | ||
|
|
||
| // These contracts live in monorepo but are not part of the core protocol | ||
| 'CeloFeeCurrencyAdapterOwnable', | ||
| 'FeeCurrencyAdapter', | ||
| 'FeeCurrencyAdapterOwnable', | ||
| ] |
There was a problem hiding this comment.
Should contracts like:
- Attestations
- DoubleSigningSlasher
- DowntimeSlasher
- etc...
be included to ignore here?
| const ContractNameExtractorRegex = new RegExp(/(.*)Proxy/) | ||
| const ZERO_ADDRESS = '0x0000000000000000000000000000000000000000' | ||
|
|
||
| // Checks if the given transaction is a repointing of the Proxy for the given contract | ||
| const isProxyRepointTransaction = (tx: ProposalTx) => | ||
| tx.contract.endsWith('Proxy') && | ||
| (tx.function === '_setImplementation' || tx.function === '_setAndInitializeImplementation') | ||
|
|
||
| export const isProxyRepointAndInitializeTransaction = (tx: ProposalTx) => | ||
| tx.contract.endsWith('Proxy') && tx.function === '_setAndInitializeImplementation' | ||
|
|
||
| export const isProxyRepointAndInitForIdTransaction = (tx: ProposalTx, registryId: string) => | ||
| tx.contract === registryId && isProxyRepointAndInitializeTransaction(tx) | ||
|
|
||
| const isProxyRepointForIdTransaction = (tx: ProposalTx, contract: string) => | ||
| tx.contract === `${contract}Proxy` && isProxyRepointTransaction(tx) | ||
|
|
||
| const isImplementationChanged = (contract: string, proposal: ProposalTx[]): boolean => | ||
| proposal.some((tx: ProposalTx) => isProxyRepointForIdTransaction(tx, contract)) | ||
|
|
||
| const getProposedImplementationAddress = (contract: string, proposal: ProposalTx[]) => | ||
| proposal.find((tx: ProposalTx) => isProxyRepointForIdTransaction(tx, contract)).args[0] |
There was a problem hiding this comment.
Good idea might be to ask AI to simplify & extract / isolate somewhere such expressions into utilities to hide bloat... or maybe to avoid extracting them into functions used once 🙃
| */ | ||
| export const computeFoundryLibraryHash = (sourcePath: string, libraryName: string): string => { | ||
| const stringToHash = `${sourcePath}:${libraryName}` | ||
| const hashed = keccak256(toHex(new TextEncoder().encode(stringToHash))) |
There was a problem hiding this comment.
Potential cleanup: this works, but should also work without the TextEncoder
|
|
||
| /** | ||
| * Class for tracking positions of library placeholders in Foundry bytecode. | ||
| * Uses linkReferences from Foundry artifacts directly instead of regex parsing. |
There was a problem hiding this comment.
| * Uses linkReferences from Foundry artifacts directly instead of regex parsing. | |
| * Uses linkReferences from Foundry artifacts. |
This is useful context in this PR, but not really a necessary comment in the future.
| const bytePositions = positions.map(pos => { | ||
| // Remove "0x" prefix (2 chars) then convert byte position to char position | ||
| return pos.start * 2 + 2 | ||
| }) |
| governanceAddress: string | ||
| proposal: ProposalTx[] | ||
| proxyABI: any[] | ||
| web3: Web3 |
There was a problem hiding this comment.
Could replace this with Viem, but could be further work, not as important as removing Truffle.
| contract: string, | ||
| artifacts: FoundryBuildArtifacts[], | ||
| debug: boolean | ||
| ): Map<string, { sourcePath: string; positions: Array<{ start: number; length: number }> }> => { |
There was a problem hiding this comment.
Could abstract away some of the verbose types for cleanliness.
|
|
||
| // Extract creation bytecode | ||
| let bytecode = '' | ||
| if (typeof rawArtifact.bytecode === 'string') { |
| FORNO="" | ||
| LOG_FILE="/dev/stdout" | ||
|
|
||
| while getopts 'b:n:fl:' flag; do |
There was a problem hiding this comment.
Could add a -d debug flag to activate the optional debugging.
| echo "Build completed. Output directories: $BUILD_DIR_05 and $BUILD_DIR_08" | ||
|
|
||
| # Determine RPC URL based on network | ||
| case "$NETWORK" in |
There was a problem hiding this comment.
The logic here + below feels a bit superfluous. If --forno is set, just use one of the hardcoded URLs. Otherwise we could either require a set RPC_URL, or use the default localhost:8545.
| const buildArtifacts = getFoundryBuildArtifacts(artifactsDirectory, debug) | ||
| const artifacts08 = getFoundryBuildArtifacts(artifacts08Directory, debug) |
| const buildArtifacts = getFoundryBuildArtifacts(artifactsDirectory, debug) | ||
| const artifacts08 = getFoundryBuildArtifacts(artifacts08Directory, debug) | ||
|
|
||
| if (debug) { |
There was a problem hiding this comment.
As in some other places, this debugging seems superfluous.
553b28c to
e24acf1
Compare
Description
A few sentences describing the overall effects and goals of the pull request's commits.
What is the current behavior, and what is the updated/expected behavior with this PR?
Other changes
Describe any minor or "drive-by" changes here.
Tested
An explanation of how the changes were tested or an explanation as to why they don't need to be.
Related issues
Backwards compatibility
Brief explanation of why these changes are/are not backwards compatible.
Documentation
The set of community facing docs that have been added/modified because of this change