Conversation
|
Summary of ChangesHello, I'm Gemini Code Assist1! I'm currently reviewing this pull request and will post my feedback shortly. In the meantime, here's a summary to help you and other reviewers quickly get up to speed! This pull request introduces a new utility script designed to streamline the process of proposing transactions to Gnosis Safes. By reading transaction details from a JSON file, the script automates the creation and submission of single or batched transactions to the Safe Transaction Service API, enhancing efficiency and reducing manual effort for Safe operations across various EVM chains. Highlights
Changelog
Using Gemini Code AssistThe full guide for Gemini Code Assist can be found on our documentation page, here are some quick tips. Invoking Gemini You can request assistance from Gemini at any point by creating a comment using either
Customization To customize Gemini Code Assist for GitHub experience, repository maintainers can create a configuration file and/or provide a custom code review style guide (such as PEP-8 for Python) by creating and adding files to a Limitations & Feedback Gemini Code Assist may make mistakes. Please leave feedback on any instances where its feedback is incorrect or counter productive. You can react with 👍 and 👎 on @gemini-code-assist comments. If you're interested in giving your feedback about your experience with Gemini Code Assist for GitHub and other Google products, sign up here. You can also get AI-powered code generation, chat, as well as code reviews directly in the IDE at no cost with the Gemini Code Assist IDE Extension. Footnotes
|
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
WalkthroughAdds SafePropose, a Solidity script that parses Safe deployment JSON into calls, enforces a single Safe sender and CALL types, builds single or multisend proposals, signs Safe transaction hashes, and POSTs proposals to the Safe Transaction Service; also updates spellcheck words. Changes
Sequence Diagram(s)sequenceDiagram
actor User
participant File as JSON File
participant Script as SafePropose
participant Safe as ISafe
participant Multi as IMultiSend
participant API as Safe Tx Service
User->>Script: run(filePath)
Script->>File: read & parse JSON -> Call[]
Script->>Script: validate single sender & types
alt single call
Script->>Safe: nonce()
Safe-->>Script: nonce
Script->>Safe: getTransactionHash(to,value,data,op=0,...,nonce)
Safe-->>Script: safeTxHash
Script->>Script: vm.sign(safeTxHash) -> signature
Script->>API: POST proposal (body with safeTxHash, signature, nonce, sender)
API-->>Script: 201 / error
else multisend
Script->>Script: encode multisend payload (calls -> bytes)
Script->>Multi: prepare multisend calldata
Script->>Safe: nonce()
Safe-->>Script: nonce
Script->>Safe: getTransactionHash(MULTISEND,0,payload,op=1,...,nonce)
Safe-->>Script: safeTxHash
Script->>Script: vm.sign(safeTxHash) -> signature
Script->>API: POST proposal (body with safeTxHash, signature, nonce, sender)
API-->>Script: 201 / error
end
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes 🚥 Pre-merge checks | ✅ 2✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
This comment was marked as resolved.
This comment was marked as resolved.
Sorry, something went wrong.
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #883 +/- ##
==========================================
- Coverage 72.02% 71.59% -0.43%
==========================================
Files 229 230 +1
Lines 8358 8408 +50
Branches 2692 2705 +13
==========================================
Hits 6020 6020
- Misses 2106 2156 +50
Partials 232 232
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
This comment was marked as resolved.
This comment was marked as resolved.
Sorry, something went wrong.
This comment was marked as resolved.
This comment was marked as resolved.
Sorry, something went wrong.
![devin-ai-integration[bot]](https://avatars.githubusercontent.com/in/811515?s=60&v=4){kind=small}
| using stdJson for string; | ||
| using Surl for string; | ||
|
|
||
| IMultiSend internal constant MULTISEND = IMultiSend(0x38869bf66a61cF6bDB996A6aE40D5853Fd43B526); // github.com/safe-global/safe-deployments v1.4.1 |
There was a problem hiding this comment.
🟡 Explanatory comment violates AGENTS.md no-comments rule
The inline comment // github.com/safe-global/safe-deployments v1.4.1 is an explanatory/attribution comment. Per AGENTS.md, the codebase does not use comments — the only exceptions are static analysis annotations (solhint-disable, cspell:ignore, forge-lint, etc.) and TODO/HACK/FIXME markers. This comment is neither. Every other comment in the contracts/script/ directory follows this convention (all are lint suppressions or TODO markers).
Was this helpful? React with 👍 or 👎 to provide feedback.
| function _chainPrefix() internal view returns (string memory) { | ||
| if (block.chainid == 1) return "eth"; | ||
| if (block.chainid == 10) return "oeth"; | ||
| if (block.chainid == 137) return "pol"; | ||
| if (block.chainid == 8453) return "base"; | ||
| if (block.chainid == 42_161) return "arb1"; | ||
| revert UnsupportedChain(); | ||
| } |
There was a problem hiding this comment.
🚩 Safe API v2 URL format and chain prefixes should be verified
The URL pattern https://api.safe.global/tx-service/{prefix}/api/v2/safes/{safe}/multisig-transactions/ and the chain prefix mapping (eth=1, oeth=10, pol=137, base=8453, arb1=42161) at contracts/script/SafePropose.s.sol:113-120 correspond to the Safe Transaction Service's newer API format. These prefixes are not easily verifiable from the codebase alone. If Safe changes their API URL structure or chain prefixes, this would silently fail with a non-201 status (caught by the ProposalFailed revert). Worth verifying against current Safe API documentation.
Was this helpful? React with 👍 or 👎 to provide feedback.
There was a problem hiding this comment.
Actionable comments posted: 1
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro
Run ID: 66bc0413-5e67-4e27-bd51-08fba140d16c
📒 Files selected for processing (2)
contracts/script/SafePropose.s.solcspell.json
| { | ||
| (uint8 v, bytes32 r, bytes32 s) = vm.sign(safeTxHash); | ||
| sender = ecrecover(safeTxHash, v, r, s); | ||
| signature = abi.encodePacked(r, s, v); | ||
| } |
There was a problem hiding this comment.
🧹 Nitpick | 🔵 Trivial
Consider validating ecrecover result.
ecrecover returns address(0) if the signature or hash is malformed. While unlikely in normal operation, adding a guard would provide a clearer error message than a downstream API failure.
🛡️ Proposed fix
{
(uint8 v, bytes32 r, bytes32 s) = vm.sign(safeTxHash);
sender = ecrecover(safeTxHash, v, r, s);
+ require(sender != address(0), "Invalid signature");
signature = abi.encodePacked(r, s, v);
}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| { | |
| (uint8 v, bytes32 r, bytes32 s) = vm.sign(safeTxHash); | |
| sender = ecrecover(safeTxHash, v, r, s); | |
| signature = abi.encodePacked(r, s, v); | |
| } | |
| { | |
| (uint8 v, bytes32 r, bytes32 s) = vm.sign(safeTxHash); | |
| sender = ecrecover(safeTxHash, v, r, s); | |
| require(sender != address(0), "Invalid signature"); | |
| signature = abi.encodePacked(r, s, v); | |
| } |
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 463722fb9a
ℹ️ About Codex in GitHub
Codex has been enabled to automatically review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback".
| uint256 nonce = safe.nonce(); | ||
| bytes32 safeTxHash = | ||
| safe.getTransactionHash(to, value, data, operation, 0, 0, 0, address(0), payable(address(0)), nonce); |
There was a problem hiding this comment.
safe.nonce()
When the target Safe already has a pending multisig transaction, safe.nonce() is still the last executed nonce, so this script will always propose a competing transaction for the current nonce instead of queueing the new action after the existing one. In that common multisig workflow, the generated contractTransactionHash is for the wrong slot and the proposal cannot be used as the next step in the queue.
Useful? React with 👍 / 👎.
| function _chainPrefix() internal view returns (string memory) { | ||
| if (block.chainid == 1) return "eth"; | ||
| if (block.chainid == 10) return "oeth"; | ||
| if (block.chainid == 137) return "pol"; | ||
| if (block.chainid == 8453) return "base"; | ||
| if (block.chainid == 42_161) return "arb1"; | ||
| if (block.chainid == 204) return "opbnb"; | ||
| revert UnsupportedChain(); |
There was a problem hiding this comment.
_chainPrefix() only recognizes mainnet chain IDs, but this repository already ships contract configs and broadcast artifacts for Base Sepolia (84532) and OP Sepolia (11155420) in contracts/deploy.json and contracts/broadcast/**. Running the new script on either staging network will always revert with UnsupportedChain(), which makes the tool unusable for the existing non-production deployment flow.
Useful? React with 👍 / 👎.
This comment was marked as resolved.
This comment was marked as resolved.
Sorry, something went wrong.
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: bbf0f44b4c
ℹ️ About Codex in GitHub
Codex has been enabled to automatically review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback".
| string[] memory headers = new string[](1); | ||
| headers[0] = "Content-Type: application/json"; |
There was a problem hiding this comment.
When this posts to the hosted https://api.safe.global/tx-service/... endpoint, the request only includes Content-Type. Safe's current docs for the default Transaction Service say hosted requests should be authenticated with Authorization: Bearer ..., and their API-key guide notes that missing keys are rejected with 401 Unauthorized. In other words, on the official service this script will always revert with ProposalFailed instead of creating the proposal, because there is no way here to send the API key or point at a custom/self-hosted tx-service.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: f20de20fd5
ℹ️ About Codex in GitHub
Codex has been enabled to automatically review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback".
| string memory url = string.concat( | ||
| "https://api.safe.global/tx-service/", _chainPrefix(), "/api/v2/safes/", hexSafe, "/multisig-transactions/" | ||
| ); |
There was a problem hiding this comment.
run parses transactions from a broadcast file but never verifies that their .transaction.chainId matches the chain the script is currently connected to; propose always derives the endpoint from _chainPrefix() (current block.chainid). If someone accidentally feeds a run-latest.json from another network (easy in this repo because artifacts are stored per-chain), this can submit a proposal to the wrong Safe Transaction Service/network instead of failing fast. Add an explicit chain-id check before proposing.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: cccdae6879
ℹ️ About Codex in GitHub
Codex has been enabled to automatically review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback".
| address from = json.readAddress(string.concat(prefix, ".from")); | ||
| if (i == 0) safe = from; | ||
| else if (from != safe) revert SenderMismatch(); |
There was a problem hiding this comment.
run currently treats .transactions[i].transaction.from as the Safe address, but in standard Forge broadcast artifacts that field is the broadcasting EOA (for example, existing contracts/broadcast/**/run-latest.json files use a deployer EOA in from). In that common workflow safe is set to an EOA, so subsequent safe.nonce() / getTransactionHash(...) calls in propose revert or build an invalid proposal, making the script unusable for normal broadcast outputs.
Useful? React with 👍 / 👎.
itofarina
force-pushed
the
redeployer
branch
3 times, most recently
from
50f2b29 to
26eefc4
Compare
This comment was marked as resolved.
This comment was marked as resolved.
Sorry, something went wrong.
![sentry[bot]](https://avatars.githubusercontent.com/in/12637?s=60&v=4){kind=small}
| revert NotACall(); | ||
| } | ||
| prefix = string.concat(prefix, ".transaction"); | ||
| address from = json.readAddress(string.concat(prefix, ".from")); |
There was a problem hiding this comment.
Bug: The script assumes the from address in the input JSON is a Safe contract. It will fail if an EOA address is provided, as there's no validation.
Severity: MEDIUM
Suggested Fix
Add a check to validate that the safe address is a contract before attempting to call methods on it. This can be done by checking the address's code size, for example: if (safe.code.length == 0) revert NotASafeContract();. This will provide a clear error message to the user instead of a cryptic low-level failure.
Prompt for AI Agent
Review the code at the location below. A potential bug has been identified by an AI
agent.
Verify if this is a real issue. If it is, propose a fix; if not, explain why it's not
valid.
Location: contracts/script/SafePropose.s.sol#L31
Potential issue: The `SafePropose.s.sol` script reads a `from` address from a broadcast
JSON file and assigns it to the `safe` variable. It then proceeds to call
contract-specific methods like `safe.nonce()` on this address. The script lacks any
validation to ensure the provided address is a contract and not an Externally Owned
Account (EOA). If a user mistakenly provides a standard broadcast file generated from an
EOA, the script will fail with a low-level error when it attempts to execute code on an
address that has none. This makes the script fragile and hard to debug for users.
Did we get this right? 👍 / 👎 to inform future reviews.
| using stdJson for string; | ||
| using Surl for string; | ||
|
|
||
| IMultiSendCallOnly internal constant MULTI_SEND = IMultiSendCallOnly(0x9641d764fc13c8B624c04430C7356C1C7C8102e2); // github.com/safe-global/safe-deployments v1.4.1 |
There was a problem hiding this comment.
🚩 Hardcoded MultiSendCallOnly address assumes deployment on all supported chains
The constant MULTI_SEND at line 16 uses address 0x9641d764fc13c8B624c04430C7356C1C7C8102e2 (Safe v1.4.1 MultiSendCallOnly). This is a deterministically deployed Safe contract that should exist on standard EVM chains. However, _chainPrefix() at line 119 includes opBNB (chain 204), which is a less common chain. If Safe's v1.4.1 MultiSendCallOnly is not deployed on opBNB, batch proposals (proposeBatch) would delegate-call to a non-existent contract. Single-call proposals would still work since they bypass MultiSend. Worth verifying the deployment exists on all six supported chains.
Was this helpful? React with 👍 or 👎 to provide feedback.
| string memory hexSafe = address(safe).toHexStringChecksummed(); | ||
| string memory url = string.concat( | ||
| "https://api.safe.global/tx-service/", _chainPrefix(), "/api/v2/safes/", hexSafe, "/multisig-transactions/" | ||
| ); |
There was a problem hiding this comment.
🟡 Single-use hexSafe variable violates the inline extraction rule
hexSafe is consumed exactly once (line 55) yet is extracted into a named local variable on line 53. AGENTS.md's extraction rules state: "single-use = inline: a value consumed once stays at the point of consumption. … no exceptions for 'clarity' — the call site is already clear." The expression address(safe).toHexStringChecksummed() can be placed directly inside the string.concat call on line 55 without any technical issue.
| string memory hexSafe = address(safe).toHexStringChecksummed(); | |
| string memory url = string.concat( | |
| "https://api.safe.global/tx-service/", _chainPrefix(), "/api/v2/safes/", hexSafe, "/multisig-transactions/" | |
| ); | |
| string memory url = string.concat( | |
| "https://api.safe.global/tx-service/", _chainPrefix(), "/api/v2/safes/", address(safe).toHexStringChecksummed(), "/multisig-transactions/" | |
| ); |
Was this helpful? React with 👍 or 👎 to provide feedback.
This comment was marked as resolved.
This comment was marked as resolved.
Sorry, something went wrong.
This comment was marked as resolved.
This comment was marked as resolved.
Sorry, something went wrong.
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 4e12746953
ℹ️ About Codex in GitHub
Codex has been enabled to automatically review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback".
| if (block.chainid == 42_161) return "arb1"; | ||
| if (block.chainid == 204) return "opbnb"; // cspell:ignore opbnb | ||
| revert UnsupportedChain(); |
There was a problem hiding this comment.
SafePropose now has companion config for BNB (bnb_smart_chain in contracts/foundry.toml and chain "56" data in contracts/deploy.json), but _chainPrefix() never handles block.chainid == 56, so the script always reverts with UnsupportedChain() on BNB instead of proposing to Safe’s tx-service. This makes the new BNB wiring unusable in practice whenever operators run the script against that network.
Useful? React with 👍 / 👎.
| using stdJson for string; | ||
| using Surl for string; | ||
|
|
||
| IMultiSendCallOnly internal constant MULTI_SEND = IMultiSendCallOnly(0x9641d764fc13c8B624c04430C7356C1C7C8102e2); // github.com/safe-global/safe-deployments v1.4.1 |
There was a problem hiding this comment.
🚩 Explanatory comment on hardcoded address may violate no-comments convention
Line 16 has // github.com/safe-global/safe-deployments v1.4.1 documenting the provenance of the hardcoded MULTI_SEND address. The AGENTS.md rule states that the only allowed comments are static analysis annotations and TODO/HACK/FIXME markers. While this technically violates the rule, documenting the source of a hardcoded contract address is a reasonable safety practice — it allows future developers to verify the address. The existing codebase uses the // TODO remove after https://github.com/... pattern in contracts/test/Fork.t.sol:23-25 which is allowed because it uses the TODO prefix. A possible compromise: convert this to a TODO-style comment or encode the provenance into the variable name.
Was this helpful? React with 👍 or 👎 to provide feedback.
| if (block.chainid == 137) return "pol"; | ||
| if (block.chainid == 204) return "opbnb"; // cspell:ignore opbnb | ||
| if (block.chainid == 8453) return "base"; | ||
| if (block.chainid == 42_161) return "arb1"; |
There was a problem hiding this comment.
Bug: The script uses the wrong Safe Transaction Service endpoint for opBNB. It points to api.safe.global instead of the correct bnbchain.org domain, causing proposals to fail.
Severity: HIGH
Suggested Fix
Update the script to use the correct endpoint for opBNB. Instead of constructing the URL with the api.safe.global domain for chain ID 204, it should use https://safe-transaction-opbnb-mainnet.bnbchain.org/. This may require special handling for the opBNB chain ID to use a different base URL.
Prompt for AI Agent
Review the code at the location below. A potential bug has been identified by an AI
agent. Verify if this is a real issue. If it is, propose a fix; if not, explain why it's
not valid.
Location: contracts/script/SafePropose.s.sol#L120
Potential issue: The `SafePropose` script adds support for opBNB (chain ID 204) by
constructing a URL to the Safe Transaction Service. However, it incorrectly uses the
generic `api.safe.global` domain. opBNB has its own dedicated Safe Transaction Service
infrastructure hosted at `safe-transaction-opbnb-mainnet.bnbchain.org`. Consequently,
any attempt to propose a transaction on opBNB will target a non-existent endpoint,
receive a non-201 HTTP status, and cause the script to revert with a `ProposalFailed`
error. This renders the opBNB proposal feature non-functional.
| "1": { | ||
| "0xA0b86991c6218b36c1d19D4a2e9Eb0cE3606eB48": "USDC", | ||
| "0xC02aaA39b223FE8D0A0e5C4F27eAD9083C756Cc2": "WETH", | ||
| "0x6B175474E89094C44Da98b954EedeAC495271d0F": "DAI", | ||
| "0x2260FAC5E5542a773Aa44fBCfeDf7C193bc2C599": "WBTC", | ||
| "0x7f39C581F595B53c5cb19bD0b3f8dA6c935E2Ca0": "wstETH", | ||
| "0xdAC17F958D2ee523a2206206994597C13D831ec7": "USDT" | ||
| }, |
There was a problem hiding this comment.
🟡 Unsorted addresses in deploy.json for new chain entries break established pattern
All pre-existing chain entries in the allowlist (chains 10, 137, 8453, 42161) have addresses sorted in ascending hex order. The three newly added chains (1, 56, 204) do NOT follow this convention. For example, chain "1" has order 0xA0b8, 0xC02a, 0x6B17, 0x2260, 0x7f39, 0xdAC1 instead of the correct 0x2260, 0x6B17, 0x7f39, 0xA0b8, 0xC02a, 0xdAC1. This violates AGENTS.md rules on consistency ("consistency is more important than personal preference. adhere to the established patterns in the codebase") and diff-friendliness ("add in the middle or sorted position").
| "1": { | |
| "0xA0b86991c6218b36c1d19D4a2e9Eb0cE3606eB48": "USDC", | |
| "0xC02aaA39b223FE8D0A0e5C4F27eAD9083C756Cc2": "WETH", | |
| "0x6B175474E89094C44Da98b954EedeAC495271d0F": "DAI", | |
| "0x2260FAC5E5542a773Aa44fBCfeDf7C193bc2C599": "WBTC", | |
| "0x7f39C581F595B53c5cb19bD0b3f8dA6c935E2Ca0": "wstETH", | |
| "0xdAC17F958D2ee523a2206206994597C13D831ec7": "USDT" | |
| }, | |
| "1": { | |
| "0x2260FAC5E5542a773Aa44fBCfeDf7C193bc2C599": "WBTC", | |
| "0x6B175474E89094C44Da98b954EedeAC495271d0F": "DAI", | |
| "0x7f39C581F595B53c5cb19bD0b3f8dA6c935E2Ca0": "wstETH", | |
| "0xA0b86991c6218b36c1d19D4a2e9Eb0cE3606eB48": "USDC", | |
| "0xC02aaA39b223FE8D0A0e5C4F27eAD9083C756Cc2": "WETH", | |
| "0xdAC17F958D2ee523a2206206994597C13D831ec7": "USDT" | |
| }, |
Was this helpful? React with 👍 or 👎 to provide feedback.
| "56": { | ||
| "0x8AC76a51cc950d9822D68b83fE1Ad97B32Cd580d": "USDC", | ||
| "0x4DB5a66E937A9F4473fA95b1cAF1d1E1D62E29EA": "WETH", | ||
| "0x0555E30da8f98308EdB960aa94C0Db47230d2B9c": "WBTC", | ||
| "0x55d398326f99059fF775485246999027B3197955": "USDT" | ||
| }, |
There was a problem hiding this comment.
🟡 Unsorted addresses in deploy.json for BNB Smart Chain (chain 56)
Chain "56" allowlist addresses are in order 0x8AC7, 0x4DB5, 0x0555, 0x55d3 instead of ascending hex order 0x0555, 0x4DB5, 0x55d3, 0x8AC7. This breaks the established pattern used by all pre-existing chain entries (10, 137, 8453, 42161) and violates AGENTS.md consistency and diff-friendliness rules.
| "56": { | |
| "0x8AC76a51cc950d9822D68b83fE1Ad97B32Cd580d": "USDC", | |
| "0x4DB5a66E937A9F4473fA95b1cAF1d1E1D62E29EA": "WETH", | |
| "0x0555E30da8f98308EdB960aa94C0Db47230d2B9c": "WBTC", | |
| "0x55d398326f99059fF775485246999027B3197955": "USDT" | |
| }, | |
| "56": { | |
| "0x0555E30da8f98308EdB960aa94C0Db47230d2B9c": "WBTC", | |
| "0x4DB5a66E937A9F4473fA95b1cAF1d1E1D62E29EA": "WETH", | |
| "0x55d398326f99059fF775485246999027B3197955": "USDT", | |
| "0x8AC76a51cc950d9822D68b83fE1Ad97B32Cd580d": "USDC" | |
| }, |
Was this helpful? React with 👍 or 👎 to provide feedback.
| "204": { | ||
| "0xE7798f023fC62146e8Aa1b36Da45fb70855a77Ea": "WETH", | ||
| "0x9e5AAC1Ba1a2e6aEd6b32689DFcF62A509Ca96f3": "USDT" | ||
| }, |
There was a problem hiding this comment.
🟡 Unsorted addresses in deploy.json for opBNB (chain 204)
Chain "204" allowlist addresses are in order 0xE779, 0x9e5A instead of ascending hex order 0x9e5A, 0xE779. This breaks the established pattern used by all pre-existing chain entries and violates AGENTS.md consistency and diff-friendliness rules.
| "204": { | |
| "0xE7798f023fC62146e8Aa1b36Da45fb70855a77Ea": "WETH", | |
| "0x9e5AAC1Ba1a2e6aEd6b32689DFcF62A509Ca96f3": "USDT" | |
| }, | |
| "204": { | |
| "0x9e5AAC1Ba1a2e6aEd6b32689DFcF62A509Ca96f3": "USDT", | |
| "0xE7798f023fC62146e8Aa1b36Da45fb70855a77Ea": "WETH" | |
| }, |
Was this helpful? React with 👍 or 👎 to provide feedback.
| mainnet = "${ETHEREUM_RPC_URL}" | ||
| optimism = "${OPTIMISM_RPC_URL}" | ||
| optimism_sepolia = "${OPTIMISM_SEPOLIA_RPC_URL}" | ||
| op_bnb = "${OP_BNB_RPC_URL}" |
There was a problem hiding this comment.
🟡 op_bnb placed out of alphabetical order in foundry.toml [rpc_endpoints]
op_bnb is placed after optimism_sepolia but alphabetically "op_b" < "opt", so it should come before optimism. The other new entries (arbitrum, bnb_smart_chain) were correctly placed in alphabetical order. The pre-existing entries (base, base_sepolia, mainnet, optimism, optimism_sepolia, polygon) were also in alphabetical order. This violates the AGENTS.md diff-friendliness rule: "add in the middle or sorted position." The same issue exists in the [etherscan] section at contracts/foundry.toml:60.
Prompt for agents
The `op_bnb` entry in foundry.toml is placed after `optimism_sepolia` in both `[rpc_endpoints]` (line 48) and `[etherscan]` (line 60) sections, but alphabetically it should come before `optimism`. Move `op_bnb` to be between `mainnet` and `optimism` in both sections to maintain alphabetical ordering consistent with how the other new entries (`arbitrum`, `bnb_smart_chain`) were placed.
Was this helpful? React with 👍 or 👎 to provide feedback.
cruzdanilo
force-pushed
the
safe-propose
branch
from
684590a to
cd62f9d
Compare
2 participants
Summary by CodeRabbit
New Features
Chores