Integrate Ecosystem Signing Policies #149
Conversation
WalkthroughThe changes introduce optional Changes
Sequence Diagram(s)sequenceDiagram
participant Caller
participant SmartWallet
participant PersonalAccount
participant EnclaveAPI
Caller->>SmartWallet: HashAndSignUserOp(userOp, chainId)
SmartWallet->>SmartWallet: EncodeUserOperation(userOp)
SmartWallet->>PersonalAccount: PersonalSign(userOpHash, originalMessage=userOp, chainId)
PersonalAccount->>EnclaveAPI: POST /sign-message { message, isRaw, originalMessage, chainId }
EnclaveAPI-->>PersonalAccount: signature
PersonalAccount-->>SmartWallet: signature
SmartWallet-->>Caller: signature
Assessment against linked issues
📜 Recent review detailsConfiguration used: CodeRabbit UI 📒 Files selected for processing (1)
🚧 Files skipped from review as they are similar to previous changes (1)
⏰ Context from checks skipped due to timeout of 90000ms (1)
✨ Finishing Touches
🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
SupportNeed help? Create a ticket on our support page for assistance with any issues or questions. Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
![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
♻️ Duplicate comments (2)
Thirdweb/Thirdweb.Wallets/IThirdwebWallet.cs (1)
68-69: Fix typo in XML documentation.Same typo as above: "polciies" should be "policies".
Thirdweb/Thirdweb.Wallets/EngineWallet/EngineWallet.cs (1)
210-210: Consider utilizing the new parameters or document why they're unused.Same issue as the byte array overload - the new parameters should likely be used in the engine request payload for proper ecosystem signing policy support.
🧹 Nitpick comments (3)
Thirdweb/Thirdweb.Wallets/EngineWallet/EngineWallet.cs (1)
191-191: Consider utilizing the new parameters or document why they're unused.The new optional parameters
originalMessageandchainIdare added to match the interface but are not used in the implementation. For ecosystem signing policies, these parameters should likely be included in the JSON payload sent to the engine backend.Consider updating the payload to include these parameters:
public async Task<string> PersonalSign(byte[] rawMessage, object originalMessage = null, BigInteger? chainId = null) { if (rawMessage == null) { throw new ArgumentNullException(nameof(rawMessage), "Message to sign cannot be null."); } - var payload = new { messagePayload = new { message = rawMessage.BytesToHex(), isBytes = true } }; + var payload = new { + messagePayload = new { + message = rawMessage.BytesToHex(), + isBytes = true, + originalMessage, + chainId = chainId?.ToString() + } + };Thirdweb/Thirdweb.Wallets/SmartWallet/SmartWallet.cs (2)
1086-1089: Consider utilizing the new optional parameters.The
PersonalSignmethod signatures have been properly updated with optional parameters to maintain backward compatibility. However, the implementation doesn't utilize the neworiginalMessageandchainIdparameters - they're passed through but not used locally.This may be intentional if this SmartWallet is purely delegating to the underlying personal account, but consider documenting this behavior or validating that the parameters are being properly forwarded to the ecosystem signing infrastructure.
The method at line 1098 only passes the
messageparameter:-return await this._personalAccount.PersonalSign(message).ConfigureAwait(false); +return await this._personalAccount.PersonalSign(message, originalMessage, chainId).ConfigureAwait(false);Also applies to: 1094-1115
876-876: Verify encoding performance impact.The user operation is now encoded twice in the signing flow - once here to pass as context, and again later when the operation is submitted. Consider if this encoding step could be optimized or cached.
Monitor performance impact in production, especially for high-frequency user operations. The encoding step could potentially be cached if it becomes a bottleneck.
Also applies to: 949-949
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (7)
Thirdweb/Thirdweb.RPC/ThirdwebRPC.cs(2 hunks)Thirdweb/Thirdweb.Wallets/EngineWallet/EngineWallet.cs(2 hunks)Thirdweb/Thirdweb.Wallets/IThirdwebWallet.cs(1 hunks)Thirdweb/Thirdweb.Wallets/InAppWallet/EcosystemWallet/EcosystemWallet.cs(3 hunks)Thirdweb/Thirdweb.Wallets/PrivateKeyWallet/PrivateKeyWallet.cs(2 hunks)Thirdweb/Thirdweb.Wallets/SmartWallet/SmartWallet.cs(3 hunks)Thirdweb/Thirdweb.Wallets/SmartWallet/Thirdweb.AccountAbstraction/AATypes.cs(2 hunks)
⏰ Context from checks skipped due to timeout of 90000ms (1)
- GitHub Check: build-test-cov
🔇 Additional comments (13)
Thirdweb/Thirdweb.RPC/ThirdwebRPC.cs (2)
28-28: LGTM! Good addition of centralized JSON serialization settings.The addition of
_jsonSerializerSettingswithNullValueHandling.Ignoreis a good practice that ensures consistent JSON serialization behavior across RPC requests.
171-171: LGTM! Proper utilization of the serialization settings.The updated
SendBatchAsyncmethod correctly uses the centralized JSON serialization settings, ensuring null values are ignored in batch requests.Thirdweb/Thirdweb.Wallets/SmartWallet/Thirdweb.AccountAbstraction/AATypes.cs (2)
122-122: LGTM! Proper JSON serialization configuration.The
JsonObjectattribute withItemNullValueHandling.Ignoreensures that null properties inUserOperationHexifiedV6are not included in JSON serialization, which is important for clean API payloads.
159-159: LGTM! Consistent serialization behavior.The
JsonObjectattribute applied toUserOperationHexifiedV7maintains consistency with the V6 implementation and ensures proper null value handling during serialization.Thirdweb/Thirdweb.Wallets/IThirdwebWallet.cs (2)
62-62: LGTM! Well-designed interface extension.The addition of optional parameters maintains backward compatibility while supporting ecosystem signing policies. The parameter types are appropriate for the intended use case.
71-71: LGTM! Consistent method signature extension.The string overload of
PersonalSignis consistently updated with the same optional parameters as the byte array overload.Thirdweb/Thirdweb.Wallets/PrivateKeyWallet/PrivateKeyWallet.cs (2)
212-222: LGTM! Interface consistency maintained for ecosystem signing policies.The addition of optional
originalMessageandchainIdparameters maintains interface consistency with other wallet implementations. The parameters are appropriately unused in this local signing implementation since ecosystem signing policies likely apply only to remote/enclave-based wallets.
224-234: LGTM! String overload updated consistently.The string overload of
PersonalSignhas been updated with the same optional parameters as the byte array overload, maintaining method signature consistency across the interface.Thirdweb/Thirdweb.Wallets/InAppWallet/EcosystemWallet/EcosystemWallet.cs (3)
51-51: Good practice: JSON serialization settings configured for optional parameters.The
JsonSerializerSettingswithNullValueHandling.Ignoreis well-configured to handle the new optional parameters. This ensures that null values are omitted from the JSON payload sent to the enclave signing endpoint, resulting in cleaner API requests.
1006-1025: Well-implemented: Optional parameters integrated into enclave API payload.The
PersonalSignmethod for byte arrays correctly incorporates the new optional parameters into themessagePayload. The parameters are properly included in the API request to the enclave signing endpoint, enabling ecosystem signing policies to access additional context like the original message and chain ID.
1035-1054: Consistent implementation: String overload matches byte array handling.The string overload of
PersonalSignmaintains consistency with the byte array version by including the same optional parameters in the payload structure and using the same JSON serialization settings.Thirdweb/Thirdweb.Wallets/SmartWallet/SmartWallet.cs (2)
876-883: LGTM - Consistent implementation of ecosystem signing context.The V6
HashAndSignUserOpmethod correctly encodes the user operation and passes it as additional context to the personal account'sPersonalSignmethod. The conditional logic properly handles both external and non-external account types.
949-956: LGTM - V7 implementation matches V6 pattern.The V7
HashAndSignUserOpmethod follows the same pattern as V6, ensuring consistency across entry point versions. The encoded user operation provides necessary context for ecosystem signing policies.
Co-authored-by: coderabbitai[bot] <136622811+coderabbitai[bot]@users.noreply.github.com> Signed-off-by: Firekeeper <[email protected]>
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## main #149 +/- ##
==========================================
+ Coverage 60.94% 60.96% +0.01%
==========================================
Files 41 41
Lines 6563 6566 +3
Branches 832 832
==========================================
+ Hits 4000 4003 +3
Misses 2376 2376
Partials 187 187 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
Closes TOOL-4019
Pending tests
PR-Codex overview
This PR focuses on enhancing the
PersonalSignmethods across various wallet classes to include optional parameters fororiginalMessageandchainId. It also introduces new classes for user operations and improves JSON serialization settings.Detailed summary
UserOperationHexifiedV6andUserOperationHexifiedV7classes with JSON serialization.PersonalSignmethods inPrivateKeyWallet,EngineWallet, andIThirdwebWalletto includeoriginalMessageandchainIdparameters.ThirdwebRPCandEcosystemWalletto ignore null values.EcosystemWalletfor signing messages to include new parameters.HashAndSignUserOpmethods inSmartWalletto pass new parameters during signing.Summary by CodeRabbit
New Features
Improvements
Documentation