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 71.70% 71.28% -0.43%
==========================================
Files 228 229 +1
Lines 8277 8326 +49
Branches 2661 2673 +12
==========================================
Hits 5935 5935
- Misses 2113 2162 +49
Partials 229 229
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}
contracts/script/SafePropose.s.sol
Outdated
| 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 on line 16 is explanatory prose documenting the source of the MULTISEND address. AGENTS.md explicitly states: "this codebase does not use comments. the only exception is static analysis annotations (@ts-expect-error, eslint-disable, slither-disable, solhint-disable, cspell:ignore) and TODO/HACK/FIXME markers. everything else—jsdoc, explanatory prose, region markers, inline labels—is noise that masks unclear code." No other script in contracts/script/ has non-annotation // comments (confirmed via grep). This comment should either be removed or, if the address provenance must be tracked, placed in a commit message instead.
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.
2 participants
Summary by CodeRabbit
New Features
Chores