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 @@
## redeployer #883 +/- ##
==============================================
- Coverage 71.80% 71.24% -0.57%
==============================================
Files 227 228 +1
Lines 8290 8287 -3
Branches 2660 2651 -9
==============================================
- Hits 5953 5904 -49
- Misses 2109 2156 +47
+ Partials 228 227 -1
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.
| "mload", | ||
| "modelcontextprotocol", | ||
| "moti", | ||
| "MULTISEND", |
There was a problem hiding this comment.
🔴 One-off identifiers added to global cspell dictionary instead of using inline cspell:ignore
MULTISEND (a Solidity constant name) and oeth (a chain prefix string literal) each appear only in contracts/script/SafePropose.s.sol and nowhere else in the codebase. Per AGENTS.md: "only add words to cspell.json when the term is a real project-relevant word that appears broadly (e.g., a protocol name, a library, a domain term)... one-off occurrences (variable names, company names, urls, hashes, identifiers) stay as inline cspell:ignore — never pollute the global dictionary with them." Solidity supports // comments, so both can use inline cspell:ignore annotations on the lines where they appear.
Was this helpful? React with 👍 or 👎 to provide feedback.
| "nfmelendez", | ||
| "nomiclabs", | ||
| "nystrom", | ||
| "oeth", |
There was a problem hiding this comment.
🟡 One-off chain prefix "oeth" added to global cspell dictionary instead of inline cspell:ignore
"oeth" is a Safe Transaction Service chain prefix string that only appears in contracts/script/SafePropose.s.sol:115. AGENTS.md states: "one-off occurrences (variable names, company names, urls, hashes, identifiers) stay as inline cspell:ignore — never pollute the global dictionary with them." This is a one-off identifier (Optimism's chain prefix abbreviation), not a broadly-used project domain term. An inline // cspell:ignore oeth annotation on contracts/script/SafePropose.s.sol:115 is the correct approach.
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 👍 / 👎.
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: ff60f1ba-3a43-4ab2-8368-acf5b09938a0
📒 Files selected for processing (2)
contracts/script/SafePropose.s.solcspell.json
| "MULTISEND", | ||
| "mysten", | ||
| "natspec", | ||
| "nfmelendez", | ||
| "nomiclabs", | ||
| "nystrom", | ||
| "oeth", |
There was a problem hiding this comment.
Add "opbnb" to the words list for consistency.
Static analysis flags opbnb (opBNB chain prefix used in SafePropose.s.sol line 119) as an unknown word. Since oeth was added as a chain prefix, opbnb should be included as well for consistency.
🔧 Proposed fix
"oeth",
"offramp",
+ "opbnb",
"oneline",
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
449ada9 to
50f2b29
Compare
2 participants
Summary by CodeRabbit
New Features
Chores