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
|
WalkthroughAdds a new Solidity script contract Changes
Sequence Diagram(s)sequenceDiagram
actor User
participant File as JSON File
participant Script as SafePropose
participant Safe as ISafe
participant API as Safe Tx Service
participant Multi as IMultiSend
User->>Script: run(filePath)
Script->>File: read & parse JSON -> Call[]
Script->>Script: validate single sender
alt single transaction
Script->>Safe: nonce()
Safe-->>Script: nonce
Script->>Safe: getTransactionHash(to,value,data,op,...,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 batch
Script->>Script: encode multisend payload (calls -> bytes)
Script->>Multi: multiSend payload (encoded)
Script->>Safe: nonce()
Safe-->>Script: nonce
Script->>Safe: getTransactionHash(MULTISEND,0,multisendPayload,op,...,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)
📝 Coding Plan
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.
|
✅ All tests passed. |
![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 the no-comments rule from AGENTS.md
The comment // github.com/safe-global/safe-deployments v1.4.1 on the MULTISEND constant is explanatory prose, not a static analysis annotation or a TODO/HACK/FIXME marker. 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." All other // comments across contracts/src/ and contracts/script/ are strictly static analysis annotations (slither-disable, solhint-disable, forge-lint), confirming this is a violation. The comment could be replaced with a cspell:ignore annotation if needed for the spell checker.
| IMultiSend internal constant MULTISEND = IMultiSend(0x38869bf66a61cF6bDB996A6aE40D5853Fd43B526); // github.com/safe-global/safe-deployments v1.4.1 | |
| IMultiSend internal constant MULTISEND = IMultiSend(0x38869bf66a61cF6bDB996A6aE40D5853Fd43B526); |
Was this helpful? React with 👍 or 👎 to provide feedback.
| "mload", | ||
| "modelcontextprotocol", | ||
| "moti", | ||
| "MULTISEND", |
There was a problem hiding this comment.
🟡 One-off constant name "MULTISEND" added to global cspell dictionary instead of inline cspell:ignore
"MULTISEND" only appears in the single file contracts/script/SafePropose.s.sol as a constant identifier. AGENTS.md states: "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)" and "one-off occurrences (variable names, company names, urls, hashes, identifiers) stay as inline cspell:ignore — never pollute the global dictionary with them." Since Solidity supports // comments, an inline // cspell:ignore MULTISEND on contracts/script/SafePropose.s.sol:16 is the correct approach.
Prompt for agents
Remove "MULTISEND" from cspell.json line 109. Instead, add an inline cspell:ignore annotation on the line where the constant is declared in contracts/script/SafePropose.s.sol line 16. The line should become:
IMultiSend internal constant MULTISEND = IMultiSend(0x38869bf66a61cF6bDB996A6aE40D5853Fd43B526); // cspell:ignore MULTISEND
Note: this also addresses the separate issue of the explanatory comment on that same line, which should be removed per the no-comments rule.
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.
Prompt for agents
Remove "oeth" from cspell.json line 115. Instead, add an inline cspell:ignore annotation on the line where the string appears in contracts/script/SafePropose.s.sol line 115. The line should become:
if (block.chainid == 10) return "oeth"; // cspell:ignore oeth
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); | |
| } |
1 participant
Summary by CodeRabbit