fix(safe): return per-op SafeOp hash for length=1 multi-chain helper#158
Merged
Conversation
getMultiChainSingleSignatureUserOperationsEip712Hash now returns the per-UserOperation SafeOp digest when called with a single op, matching the on-chain Safe4337MultiChainSignatureModule's merkleTreeDepth == 0 branch (which verifies against keccak256(SafeOp), not the Merkle wrapper). The companion typed-data helper now throws for length<2.
📝 WalkthroughWalkthroughgetMultiChainSingleSignatureUserOperationsEip712Hash now accepts an optional Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Possibly related PRs
Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. Review rate limit: 3/5 reviews remaining, refill in 16 minutes and 40 seconds. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/account/Safe/SafeMultiChainSigAccount.ts (1)
771-795:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winPoint single-op callers to
SafeMultiChainSigAccountV1helpers instead.The new guidance currently recommends
SafeAccountV0_3_0.getUserOperationEip712Data/getUserOperationEip712Hash_V9, but those helpers do not default toSafeMultiChainSigAccountV1.DEFAULT_SAFE_4337_MODULE_ADDRESS. If callers follow this message literally without adding an override, they'll hash the wrong verifying contract and produce invalid signatures for this account class.Suggested wording update
- * SafeOp digest, not a Merkle wrapper, so the wrapper typed data would be - * misleading. Use {`@link` SafeAccountV0_3_0.getUserOperationEip712Data} - * (or {`@link` getUserOperationEip712Hash_V9}) for single-op signing. + * SafeOp digest, not a Merkle wrapper, so the wrapper typed data would be + * misleading. Use {`@link` SafeMultiChainSigAccountV1.getUserOperationEip712Data} + * or {`@link` SafeMultiChainSigAccountV1.getUserOperationEip712Hash} for + * single-op signing. ... - "getMultiChainSingleSignatureUserOperationsEip712Data requires >= 2 userOperations. " + - "For a single UserOperation, use SafeAccount.getUserOperationEip712Data_V9 / getUserOperationEip712Hash_V9: " + + "getMultiChainSingleSignatureUserOperationsEip712Data requires >= 2 userOperations. " + + "For a single UserOperation, use SafeMultiChainSigAccountV1.getUserOperationEip712Data / " + + "SafeMultiChainSigAccountV1.getUserOperationEip712Hash: " + "the on-chain depth=0 path verifies against the per-op SafeOp digest, not a Merkle wrapper.",🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/account/Safe/SafeMultiChainSigAccount.ts` around lines 771 - 795, The error message thrown in SafeMultiChainSigAccount.getMultiChainSingleSignatureUserOperationsEip712Data incorrectly directs single-op users to SafeAccountV0_3_0 helpers which do not default to SafeMultiChainSigAccountV1.DEFAULT_SAFE_4337_MODULE_ADDRESS; update the RangeError text thrown in getMultiChainSingleSignatureUserOperationsEip712Data to point callers to the SafeMultiChainSigAccountV1 helper methods (e.g., SafeMultiChainSigAccountV1.getUserOperationEip712Data and SafeMultiChainSigAccountV1.getUserOperationEip712Hash_V9) and mention that those helpers use DEFAULT_SAFE_4337_MODULE_ADDRESS (or that an overrides.safe4337ModuleAddress must be supplied) so signatures hash the correct verifying contract.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Outside diff comments:
In `@src/account/Safe/SafeMultiChainSigAccount.ts`:
- Around line 771-795: The error message thrown in
SafeMultiChainSigAccount.getMultiChainSingleSignatureUserOperationsEip712Data
incorrectly directs single-op users to SafeAccountV0_3_0 helpers which do not
default to SafeMultiChainSigAccountV1.DEFAULT_SAFE_4337_MODULE_ADDRESS; update
the RangeError text thrown in
getMultiChainSingleSignatureUserOperationsEip712Data to point callers to the
SafeMultiChainSigAccountV1 helper methods (e.g.,
SafeMultiChainSigAccountV1.getUserOperationEip712Data and
SafeMultiChainSigAccountV1.getUserOperationEip712Hash_V9) and mention that those
helpers use DEFAULT_SAFE_4337_MODULE_ADDRESS (or that an
overrides.safe4337ModuleAddress must be supplied) so signatures hash the correct
verifying contract.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 85697116-4169-4ee8-9b03-871824bab0ab
📒 Files selected for processing (2)
src/account/Safe/SafeMultiChainSigAccount.tstest/signer/signer.test.js
…ect module default The previous message directed callers to SafeAccount.getUserOperationEip712Data_V9 and getUserOperationEip712Hash_V9, which default safe4337ModuleAddress to the parent's module address rather than the multi-chain module. Following the message verbatim would hash against the wrong verifying contract. Point at SafeMultiChainSigAccountV1.getUserOperationEip712Data and getUserOperationEip712Hash instead, and document the override requirement when using parent helpers directly.
There was a problem hiding this comment.
🧹 Nitpick comments (1)
src/account/Safe/SafeMultiChainSigAccount.ts (1)
749-758: ⚡ Quick winReuse the class-level hash helper here.
The length=1 branch already has the inputs that
SafeMultiChainSigAccountV1.getUserOperationEip712Hash()normalizes for you. Calling that wrapper instead ofSafeAccount.getUserOperationEip712Hash_V9()keeps the Safe 4337 module / entrypoint defaults centralized and avoids this branch drifting if the multi-chain defaults change again.♻️ Proposed fix
- return SafeAccount.getUserOperationEip712Hash_V9(u.userOperation, u.chainId, { - validAfter: u.validAfter, - validUntil: u.validUntil, - safe4337ModuleAddress: - overrides.safe4337ModuleAddress ?? - SafeMultiChainSigAccountV1.DEFAULT_SAFE_4337_MODULE_ADDRESS, - entrypointAddress: overrides.entrypointAddress, - }); + return SafeMultiChainSigAccountV1.getUserOperationEip712Hash(u.userOperation, u.chainId, { + validAfter: u.validAfter, + validUntil: u.validUntil, + safe4337ModuleAddress: overrides.safe4337ModuleAddress, + entrypointAddress: overrides.entrypointAddress, + });🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/account/Safe/SafeMultiChainSigAccount.ts` around lines 749 - 758, The single-item branch directly calls SafeAccount.getUserOperationEip712Hash_V9; instead, reuse the class-level helper SafeMultiChainSigAccountV1.getUserOperationEip712Hash so the module/entrypoint defaults stay centralized. Replace the call in the length===1 branch (where u = userOperationsToSignsToSign[0]) with SafeMultiChainSigAccountV1.getUserOperationEip712Hash(u.userOperation, u.chainId, { validAfter: u.validAfter, validUntil: u.validUntil, safe4337ModuleAddress: overrides.safe4337ModuleAddress, entrypointAddress: overrides.entrypointAddress }) so the same normalization and defaulting logic is used.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@src/account/Safe/SafeMultiChainSigAccount.ts`:
- Around line 749-758: The single-item branch directly calls
SafeAccount.getUserOperationEip712Hash_V9; instead, reuse the class-level helper
SafeMultiChainSigAccountV1.getUserOperationEip712Hash so the module/entrypoint
defaults stay centralized. Replace the call in the length===1 branch (where u =
userOperationsToSignsToSign[0]) with
SafeMultiChainSigAccountV1.getUserOperationEip712Hash(u.userOperation,
u.chainId, { validAfter: u.validAfter, validUntil: u.validUntil,
safe4337ModuleAddress: overrides.safe4337ModuleAddress, entrypointAddress:
overrides.entrypointAddress }) so the same normalization and defaulting logic is
used.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 86163b59-e856-4695-b3d7-66645c3f1fb6
📒 Files selected for processing (1)
src/account/Safe/SafeMultiChainSigAccount.ts
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.
Summary by CodeRabbit
Improvements
Bug Fixes / Validation
Tests