Repository navigation
🔨 deploy: sign safe proposals as proposer - #826
cruzdanilo wants to merge 2 commits into
Conversation
|
WalkthroughThe deployment script replaces Safe SDK operations with direct Safe Transaction Service requests and ethers typed-data signing. It checks supported chains, Safe nonce values, and pending proposals before submitting a signed transaction. The package removes the Safe SDK development dependencies. ChangesSafe multisig proposal flow
Priority: ⬇️ Low Estimated code review effort: 3 (Moderate) | ~20 minutes Change: Feature Sequence Diagram(s)sequenceDiagram
participant ProposalScript
participant SafeTransactionService
participant ConfiguredSender
ProposalScript->>SafeTransactionService: Fetch Safe nonce and pending transactions
SafeTransactionService-->>ProposalScript: Return nonce and pending transactions
ProposalScript->>ConfiguredSender: Sign Safe transaction typed data
ConfiguredSender-->>ProposalScript: Return signature
ProposalScript->>SafeTransactionService: Submit signed proposal
Suggested reviewers: Merge Risk: 🟡 Moderate · up to
|
| Check name | Status | Explanation |
|---|---|---|
| Description Check |  |
Check skipped - CodeRabbit’s high-level summary is enabled. |
| Title check | |
The title clearly describes the main change: deploy scripts now sign Safe proposals as the proposer. |
| Linked Issues check | |
Check skipped because no linked issues were found for this pull request. |
| Out of Scope Changes check | |
Check skipped because no linked issues were found for this pull request. |
✨ Finishing Touches
-
📝 Generate docstrings
-
- Commit to this branch
- Create a new PR
🧪 Generate unit tests (beta)
-
- Commit to this branch
- Create a new PR
✨ Simplify code
-
- Commit to this branch
- Create a new PR
-
- Autofix · Keep fixing CodeRabbit findings and required CI, and resolving merge conflicts
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 @coderabbitai help to get the list of available commands.
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #826 +/- ##
========================================
Coverage 94.82% 94.82%
========================================
Files 32 32
Lines 2742 2742
Branches 366 469 +103
========================================
Hits 2600 2600
Misses 141 141
Partials 1 1 ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
Codex Review SummaryThis comment shows the latest Codex review activity on this pull request.
ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
Codex reacts with 👀 while any review is running, comments if it has suggestions, and reacts with 👍 once all reviews finish with no findings. |
bfdad42 to
a5169e0
Compare
![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: bfdad42712
ℹ️ About Codex in GitHub
Your team has set up Codex to 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 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| const { results } = await api<{ results: { to: string; data: string | null; nonce: number }[] }>( | ||
| `multisig-transactions/?executed=false&nonce__gte=${nonce}`, | ||
| ); |
There was a problem hiding this comment.
When this Safe has more pending transactions than the transaction service returns on its default page, results can omit the transaction with the highest nonce. The Math.max calculation then reuses an existing nonce and submits a competing proposal, rather than appending after all pending proposals as the previous getNextNonce call did. Request the highest nonce explicitly (for example, descending nonce ordering with a one-item limit) or follow pagination before calculating the new nonce.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
This isn't a regression. The previous api-kit@2.4.6 getNextNonce also read only the first page of getPendingTransactions (no limit, so the service default of 100), and its duplicate check did the same. Hitting this would take more than 100 unexecuted proposals queued on the multisig at once. For scale, the optimism multisig has 117 transactions in its entire history, and its pending queue is empty right now. Handling it properly would mean paginating both the duplicate check and the nonce scan, so I'm leaving this as is unless the maintainer wants it.
Generated by Claude Code
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
ℹ️ Review info
⚙️ Run configuration
- Configuration used: Organization UI
- Review profile: ASSERTIVE
- Plan: Advanced
- Run ID:
314f4bcf-eefa-4f25-bd3c-b6715d503fe9
⛔ Files ignored due to path filters: 1
pnpm-lock.yamlis excluded by!**/pnpm-lock.yaml
📒 Files selected for processing: 2
deploy/.utils/multisigPropose.tspackage.json
💤 Files with no reviewable changes: 1
- package.json
Included review availability: This review used your included allowance. Your plan provides up to 1 included review per hour; 0 remain after this review.
| gasPrice: 0, | ||
| gasToken: ZeroAddress, | ||
| refundReceiver: ZeroAddress, | ||
| nonce: Math.max(Number(nonce), ...results.map((tx) => Number(tx.nonce) + 1)), |
There was a problem hiding this comment.
🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick win
🔎 Supported by static analysis
🏁 Script executed:
sed -n '40,85p' deploy/.utils/multisigPropose.ts
rg -n 'multisigPropose\(|already queued|pending|results.some' deploy/.utils deploy testRepository: exactly/protocol
Length of output: 2342
Fetch the full pending queue before both checks.
results contains only the current page. A matching queued call on a later page can bypass the duplicate check and be proposed again. A higher nonce on a later page can also cause the calculation to select an occupied nonce.
Fetch every page at the shared response boundary, then use the aggregated results for both the duplicate check and nonce calculation. ordering=-nonce is not sufficient because it does not inspect later pages for duplicate calls.
There was a problem hiding this comment.
This is the same point as the Codex thread above. Both checks read only the first page (100 pending transactions), and that's also how the api-kit getPendingTransactions/getNextNonce calls this replaces behaved. For it to matter, the multisig would need more than 100 unexecuted proposals queued at once. Its whole history has 117 transactions, and the queue is empty right now. I'm leaving it as is unless the maintainer wants pagination.
Generated by Claude Code
There was a problem hiding this comment.
@cruzdanilo Thanks for clarifying. Given the existing first-page behavior, I overstated this as a major issue introduced by this PR. Pagination remains an edge-case limitation, but I accept leaving it unchanged unless the maintainer requests it.
✏️ Learnings added
Learnt from: cruzdanilo
Repo: exactly/protocol PR: 826
File: deploy/.utils/multisigPropose.ts:68-68
Timestamp: 2026-10-20T01:58:06.562Z
Learning: In exactly/protocol, deploy/.utils/multisigPropose.ts retains the prior Safe api-kit first-page behavior for pending transaction checks and nonce selection. The author considers full pagination optional unless the maintainer requests it. Do not describe this inherited limitation as a regression introduced by the direct Safe Transaction Service integration.
Note: Learnings are effective only in the context of similar code segments. To apply general coding guidelines, please configure review instructions. You can manage existing learnings in the app.
You are interacting with an AI system.
Requested by danilo · project thread
Before:
multisigProposewent through the Safe SDK (protocol-kit+api-kit). It only proposed when the deployer was a Safe owner, and otherwise printed the calldata for a manual proposal.After: the deployer is a proposer (a delegate on the Safe Transaction Service), not an owner. The deploy script signs the
SafeTxEIP-712 hash with the deployer key and posts it straight to the transaction service. The Safe SDK packages are gone.How:
multisigProposereads the Safe's nonce and pending queue fromapi.safe.global/tx-service/<chain>. It skips calls that are already queued and builds a defaultSafeTxat the next free nonce. Then it signs withethers.getSigner(deployer).signTypedDataand posts it withsenderset to the deployer. IfSAFE_API_KEYis set, it's sent as a bearer token. The fallback behavior hasn't changed: unsupported chains, or a sender equal to the safe, still throw, sotimelockProposeexecutes directly as before. Any other failure, such as the sender not being a proposer, logs the service's error and prints the manual proposal.Checks:
safeTxHashof the latest real transactions on the optimism multisig (Safe 1.3.0) and the base multisig (Safe 1.4.1).pnpm lintandpnpm test:hardhat(674 passing).Generated by Claude Code
Summary by CodeRabbit