feat: support custom Safe nonce and simulate batches through the Safe - #53
Open
PierrickGT wants to merge 1 commit into
Open
feat: support custom Safe nonce and simulate batches through the Safe#53PierrickGT wants to merge 1 commit into
PierrickGT wants to merge 1 commit into
Conversation
MultiSigBatchBase previously always proposed at the Safe's on-chain nonce, which only advances on execution and therefore collides with proposals that are already queued. - `_proposeBatch(safe, sender)` now defaults to the `SAFE_NONCE` environment variable, falling back to the on-chain nonce, and a `_proposeBatch(safe, sender, nonce)` overload takes an explicit nonce. The batch is signed and proposed with the same nonce. - `_simulateBatch` now runs the batch through the Safe's `execTransaction` with synthetic owner approvals instead of pranking the Safe and calling each target directly, so the MultiSend encoding, the threshold check and any guard or fallback handler are exercised too. Bumps safe-utils v0.0.19 -> v0.0.22 for the nonce-aware `proposeTransactionsWithSignature` and `simulateTransactionsMultiSigNoSign`.
|
|
Changes to gas cost
🧾 Summary (20% most significant diffs)
Full diff report 👇
|
LCOV of commit
|
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Why
MultiSigBatchBasealways proposed at the Safe's on-chain nonce. That nonce only advances on execution, so any batch proposed while other transactions are already queued collides with them instead of queueing behind them.While in here, the simulation path was an approximation: it pranked the Safe and called each target directly, which skips the MultiSend encoding, the threshold check, and any guard or fallback handler.
What
_proposeBatch(safe, sender)reads the nonce from theSAFE_NONCEenvironment variable, falling back to the Safe's on-chain nonce._proposeBatch(safe, sender, nonce)takes one explicitly. The batch is signed and proposed with the same nonce — a mismatch there is rejected by the Safe API with a confusing "incorrect signer address" error._simulateBatchnow goes through the Safe'sexecTransactionviasimulateTransactionsMultiSigNoSign, which writes syntheticapprovedHashesentries for the current owners (read fromOwnerManager.getOwners()).vm.dealmoved from the Safe to the owners, since under--isolateit's the pranked owner EOA that pays gas; balances are topped up rather than overwritten.v0.0.19->v0.0.22, which is where the nonce-awareproposeTransactionsWithSignatureandsimulateTransactionsMultiSigNoSignland.The existing
_proposeBatch(safe, sender)and_simulateBatch(safe)signatures are unchanged, so no downstream script needs to change.Behavior changes worth reviewing
MultiSendCallOnlyby chain ID. The old prank loop worked on any chain against a codeless address. Every chain inChainsBaseis supported.SAFE_NONCE— correct for a "would this execute" check, but a batch queued behind pending proposals is simulated against current state, not post-pending state.initialize/getNonce/getProposeTransactionsTargetAndData/signare unchanged.Testing
forge testpasses (341 tests). The script base has no committed coverage, so I verified the new simulation path with a throwaway test — aSafeProxywith 3 owners and threshold 2,MultiSendCallOnlyetched at the canonical address:msg.senderat the target equal to the Safe andsafe.nonce()advancing to 1, with deliberately unsorted owners sorted correctly;Simulation failed, with the Safe'sGS013revert data logged.Happy to commit that test if we want script coverage in this repo — it pulls the full
Safecontract into the test build, which is the only reason I left it out.