Add ServerWallet implementation and deprecate EngineWallet #153
Conversation
Introduces the ServerWallet class as a replacement for EngineWallet, including execution options and transaction handling. Marks EngineWallet as obsolete. Updates the console example to use ServerWallet and adds ENGINE_API_URL to constants. Closes TOOL-5016 --Will be merged once tested w/ auto = EIP7702 in future Engine Cloud release--
WalkthroughThe changes introduce a new Changes
Sequence Diagram(s)sequenceDiagram
participant Program
participant ServerWallet
participant EngineAPI
Program->>ServerWallet: Create (label, executionOptions, vaultAccessToken)
ServerWallet->>EngineAPI: GET /wallets (with label)
EngineAPI-->>ServerWallet: Wallet details
ServerWallet-->>Program: ServerWallet instance
Program->>ServerWallet: GetAddress()
ServerWallet-->>Program: Wallet address
Program->>ServerWallet: PersonalSign(message)
ServerWallet->>EngineAPI: POST /sign/personal
EngineAPI-->>ServerWallet: Signature
ServerWallet-->>Program: Signature
Program->>ServerWallet: SignTypedDataV4(data)
ServerWallet->>EngineAPI: POST /sign/typedData
EngineAPI-->>ServerWallet: Signature
ServerWallet-->>Program: Signature
Program->>ServerWallet: SendTransaction(txInput)
ServerWallet->>EngineAPI: POST /transactions
EngineAPI-->>ServerWallet: Transaction ID
ServerWallet->>EngineAPI: GET /transactions/{id}/status (polling)
EngineAPI-->>ServerWallet: Transaction hash
ServerWallet-->>Program: Transaction hash
Warning Review ran into problems🔥 ProblemsErrors were encountered while retrieving linked issues. Errors (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: 0
🧹 Nitpick comments (3)
Thirdweb/Thirdweb.Wallets/EngineWallet/EngineWallet.cs (1)
14-15: Deprecation attribute is good; consider enforcing it laterNice touch adding
[Obsolete]with a clear upgrade path.
For a smoother migration schedule you could:-[Obsolete("The EngineWallet is deprecated and will be removed in a future version. Please use ServerWallet instead.")] +[Obsolete( + "EngineWallet is deprecated and will be removed in a future version. Use ServerWallet instead.", + DiagnosticId = "TW001", // optional custom ID + UrlFormat = "https://github.com/thirdweb-dev/dotnet/releases" +)]…and once users have had a release cycle to migrate, flip the second parameter of
Obsoletetotrueto turn warnings into compile-time errors.Thirdweb/Thirdweb.Wallets/ServerWallet/ServerWallet.Types.cs (1)
59-64: Consider setting IdempotencyKey in constructor for consistency.The
ERC4337ExecutionOptionsconstructor setsChainIdandSignerAddressbut doesn't initializeIdempotencyKeylike the baseExecutionOptionsclass. For consistency and to prevent potential issues, consider adding idempotency key initialization.public ERC4337ExecutionOptions(BigInteger chainId, string signerAddress) { this.ChainId = chainId; this.SignerAddress = signerAddress; + this.IdempotencyKey = Guid.NewGuid().ToString(); }Thirdweb/Thirdweb.Wallets/ServerWallet/ServerWallet.cs (1)
108-130: Transaction status polling could benefit from better error handling.The
WaitForTransactionHashmethod polls for transaction completion but could be improved:
- Consider exponential backoff instead of fixed 100ms delays
- Add maximum retry limit to prevent infinite loops
- Handle network errors during polling
public async Task<string> WaitForTransactionHash(string txid) { var cancellationToken = new CancellationTokenSource(); cancellationToken.CancelAfter(this.Client.FetchTimeoutOptions.GetTimeout(TimeoutType.Other)); var transactionHash = string.Empty; + var retryCount = 0; + var maxRetries = 300; // 30 seconds with 100ms delays while (string.IsNullOrEmpty(transactionHash) && !cancellationToken.IsCancellationRequested) { + if (retryCount++ > maxRetries) + { + throw new TimeoutException($"Transaction {txid} did not complete within the expected time."); + } await ThirdwebTask.Delay(100); - var statusResponse = await this._engineClient.GetAsync($"{Constants.ENGINE_API_URL}/v1/transactions?id={txid}").ConfigureAwait(false); + try + { + var statusResponse = await this._engineClient.GetAsync($"{Constants.ENGINE_API_URL}/v1/transactions?id={txid}").ConfigureAwait(false); + // ... existing logic + } + catch (HttpRequestException ex) + { + // Log and continue polling on network errors + continue; + } } return transactionHash; }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (5)
Thirdweb.Console/Program.cs(1 hunks)Thirdweb/Thirdweb.Utils/Constants.cs(1 hunks)Thirdweb/Thirdweb.Wallets/EngineWallet/EngineWallet.cs(1 hunks)Thirdweb/Thirdweb.Wallets/ServerWallet/ServerWallet.Types.cs(1 hunks)Thirdweb/Thirdweb.Wallets/ServerWallet/ServerWallet.cs(1 hunks)
🧰 Additional context used
🧬 Code Graph Analysis (2)
Thirdweb/Thirdweb.Wallets/EngineWallet/EngineWallet.cs (1)
Thirdweb/Thirdweb.Utils/Utils.cs (1)
Obsolete(141-145)
Thirdweb.Console/Program.cs (2)
Thirdweb/Thirdweb.Wallets/ServerWallet/ServerWallet.cs (2)
ServerWallet(14-444)ServerWallet(26-32)Thirdweb/Thirdweb.Wallets/ServerWallet/ServerWallet.Types.cs (1)
ERC4337ExecutionOptions(59-63)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: build-test-cov
🔇 Additional comments (9)
Thirdweb/Thirdweb.Utils/Constants.cs (1)
13-13: Visibility ofENGINE_API_URLmight be too restrictive
ServerWallet(located underThirdweb.Wallets) will need to reference this constant.
IfThirdweb.Utilscompiles into a separate assembly, theinternalmodifier will makeENGINE_API_URLinaccessible and force a string-literal duplication elsewhere.Confirm both files live in the same assembly, or switch the modifier to
public/internal const …inside a sharedInternalsVisibleToscope to avoid that pitfall.Thirdweb.Console/Program.cs (1)
343-392: Excellent comprehensive ServerWallet example!The implementation demonstrates all key ServerWallet features including:
- Proper environment variable usage for sensitive vault access tokens
- Both AutoExecutionOptions and ERC4337ExecutionOptions configurations
- Complete workflow from creation to signing to transaction execution
- Clear console output for educational purposes
The code follows good async/await patterns and provides a thorough demonstration of the new ServerWallet capabilities.
Thirdweb/Thirdweb.Wallets/ServerWallet/ServerWallet.Types.cs (1)
1-119: Well-structured type system for ServerWallet execution options.The implementation provides:
- Clean inheritance hierarchy with
ExecutionOptionsbase class- Proper JSON serialization attributes for API communication
- Comprehensive ERC4337 options for smart account functionality
- Appropriately scoped internal classes for response handling
The type system effectively supports both auto-execution and ERC4337 smart account modes.
Thirdweb/Thirdweb.Wallets/ServerWallet/ServerWallet.cs (6)
46-102: Excellent factory method implementation with comprehensive validation.The
Createmethod demonstrates excellent practices:
- Thorough null checking and argument validation
- Proper HTTP client configuration with required headers
- Comprehensive error handling with informative messages
- Secure handling of vault access tokens
- Clear logic for execution options configuration
The method properly handles both auto and ERC4337 execution modes and provides helpful error messages when wallets are not found.
132-170: Robust transaction conversion with proper authorization list handling.The
ToEngineTransactionmethod effectively:
- Validates input parameters
- Properly sets execution options
- Handles authorization lists for EIP-7702 transactions
- Converts transaction data to engine-compatible format
The implementation correctly handles optional fields and provides appropriate defaults.
208-268: Secure implementation of personal signing with proper validation.Both
PersonalSignoverloads demonstrate:
- Comprehensive input validation
- Proper API endpoint usage
- Secure payload construction with signing options
- Consistent error handling and response parsing
The implementation correctly handles both string and byte array message formats.
270-312: Excellent typed data signing implementation with preprocessing.The
SignTypedDataV4methods show:
- Proper JSON preprocessing using utility functions
- Dynamic chain ID extraction from domain
- Comprehensive input validation
- Support for both raw JSON and typed data objects
The implementation correctly handles EIP-712 typed data signing requirements.
324-354: Robust transaction sending with proper queue handling.The
SendTransactionandExecuteTransactionmethods demonstrate:
- Proper payload serialization using
ToEngineTransaction- Comprehensive error handling for queue operations
- Proper transaction ID validation
- Integration with transaction polling for completion
The implementation effectively bridges between Thirdweb transaction inputs and engine API requirements.
366-404: Well-implemented address recovery methods using established libraries.The recovery methods properly:
- Use Nethereum's established cryptographic utilities
- Provide comprehensive input validation
- Handle both personal sign and typed data scenarios
- Return addresses in correct format
The implementation leverages proven cryptographic libraries for security.
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #153 +/- ##
==========================================
- Coverage 60.74% 58.14% -2.60%
==========================================
Files 41 43 +2
Lines 6570 6862 +292
Branches 832 873 +41
==========================================
- Hits 3991 3990 -1
- Misses 2392 2684 +292
- Partials 187 188 +1 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
Introduces EIP7702ExecutionOptions and EOAExecutionOptions classes for ServerWallet execution modes. Updates ServerWallet logic to support these new execution options and improves type handling. Also comments out ServerWallet usage in the console example and fixes account parsing in ServerWallet creation.
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (3)
Thirdweb/Thirdweb.Wallets/ServerWallet/ServerWallet.Types.cs (1)
32-43: Fix the documentation comment.The comment incorrectly describes this as "Externally Owned Account (EOA)" when it should describe EIP-7702 execution options.
Apply this diff to correct the documentation:
-/// <summary> -/// Externally Owned Account (EOA) execution options -/// </summary> +/// <summary> +/// EIP-7702 execution options +/// </summary>Thirdweb/Thirdweb.Wallets/ServerWallet/ServerWallet.cs (2)
68-68: Address the TODO comment about pagination.The TODO comment indicates that pagination support is missing for server wallet retrieval. This could be problematic for accounts with many wallets.
Do you want me to generate a solution that implements pagination support for the server wallet list endpoint?
88-111: Consider refactoring complex execution options configuration.The execution options configuration logic is comprehensive but complex. Consider extracting this into a separate method for better maintainability.
Consider extracting into a helper method:
+private static void ConfigureExecutionOptions(ExecutionOptions executionOptions, string signerWalletAddress, string smartWalletAddress) +{ + // Move the execution options configuration logic here +}
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
Thirdweb.Console/Program.cs(2 hunks)Thirdweb/Thirdweb.Wallets/ServerWallet/ServerWallet.Types.cs(1 hunks)Thirdweb/Thirdweb.Wallets/ServerWallet/ServerWallet.cs(1 hunks)
🧰 Additional context used
🧬 Code Graph Analysis (1)
Thirdweb/Thirdweb.Wallets/ServerWallet/ServerWallet.cs (5)
Thirdweb/Thirdweb.Utils/Utils.cs (5)
IThirdwebHttpClient(979-987)Utils(27-1337)ToChecksumAddress(387-390)BytesToHex(121-124)PreprocessTypedDataJson(1029-1072)Thirdweb/Thirdweb.Wallets/EngineWallet/EngineWallet.cs (17)
Task(93-113)Task(160-170)Task(172-180)Task(182-190)Task(192-209)Task(211-228)Task(230-253)Task(255-265)Task(267-285)Task(287-290)Task(292-311)Task(313-317)Task(319-322)Task(324-327)Task(329-344)Task(346-367)ToEngineTransaction(115-154)Thirdweb/Thirdweb.Utils/ThirdwebTask.cs (1)
ThirdwebTask(5-47)Thirdweb/Thirdweb.Transactions/ThirdwebTransactionReceipt.cs (1)
ThirdwebTransactionReceipt(10-106)Thirdweb/Thirdweb.Transactions/ThirdwebTransactionInput.cs (1)
EIP7702Authorization(212-220)
🪛 Gitleaks (8.27.2)
Thirdweb.Console/Program.cs
30-30: Detected a Generic API Key, potentially exposing access to various services and sensitive operations.
(generic-api-key)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: build-test-cov
🔇 Additional comments (19)
Thirdweb/Thirdweb.Wallets/ServerWallet/ServerWallet.Types.cs (7)
6-17: LGTM! Well-designed base class for execution options.The base
ExecutionOptionsclass follows good design patterns with nullableChainIdfor flexibility and appropriate JSON serialization attributes.
19-30: LGTM! Auto execution options correctly implemented.The
AutoExecutionOptionsclass properly extends the base class with appropriate type and address properties.
45-56: LGTM! EOA execution options correctly implemented.The
EOAExecutionOptionsclass properly implements externally owned account execution with correct documentation and structure.
58-90: LGTM! Comprehensive ERC-4337 execution options.The
ERC4337ExecutionOptionsclass includes all necessary properties for smart account configuration and enforces required parameters through the constructor.
92-100: LGTM! Well-structured response wrapper.The
QueuedTransactionResponseclass follows standard API response patterns with appropriate internal visibility and JSON serialization.
102-110: LGTM! Appropriate result container structure.The
QueuedTransactionResultclass correctly wraps the transactions array with proper JSON serialization.
112-145: LGTM! Well-designed transaction data structures.The
QueuedTransactionandInnerTransactionclasses provide comprehensive representation of queued transactions with proper metadata and transaction data, with appropriate JSON serialization configuration.Thirdweb.Console/Program.cs (1)
343-384: LGTM! Comprehensive ServerWallet examples.The ServerWallet examples effectively demonstrate various execution options, signing methods, and transaction handling. The commented-out format is appropriate for example code.
Thirdweb/Thirdweb.Wallets/ServerWallet/ServerWallet.cs (11)
11-32: LGTM! Well-structured ServerWallet class design.The class properly implements
IThirdwebWalletwith appropriate encapsulation, constructor validation, and JSON serialization configuration.
36-80: Well-implemented factory method with comprehensive validation.The
Createmethod properly validates inputs, handles HTTP communication, and implements robust label matching with helpful error messages.
122-144: LGTM! Robust transaction hash polling implementation.The
WaitForTransactionHashmethod properly implements timeout handling, polling logic, and error checking for transaction status.
146-181: LGTM! Comprehensive transaction conversion logic.The
ToEngineTransactionmethod correctly handles all transaction properties including authorization lists and maintains proper chain ID configuration.
187-197: LGTM! Proper address handling with checksum conversion.The
GetAddressmethod correctly handles both cases and applies checksum formatting when appropriate.
219-279: LGTM! Well-implemented personal signing methods.Both
PersonalSignoverloads properly handle different input formats, construct appropriate API payloads, and parse responses correctly.
281-323: LGTM! Robust typed data signing implementation.The
SignTypedDataV4methods properly preprocess JSON, extract chain ID from domain, and handle both generic and JSON string variants.
325-328: Appropriate use of NotImplementedException.Correctly throws
NotImplementedExceptionforSignTransactionsince ServerWallet usesSendTransactioninstead.
335-365: LGTM! Comprehensive transaction sending and execution.The
SendTransactionandExecuteTransactionmethods properly handle transaction conversion, API communication, queue management, and receipt waiting.
377-415: LGTM! Proper signature recovery implementation.The recovery methods correctly use Nethereum cryptographic utilities with appropriate input validation and error handling.
427-452: Appropriate handling of unsupported operations.Methods like
LinkAccount,UnlinkAccount, andGetLinkedAccountscorrectly throwNotImplementedExceptionas these operations are not applicable to ServerWallet.
| @@ -27,7 +27,7 @@ | |||
| var privateKey = Environment.GetEnvironmentVariable("PRIVATE_KEY"); | |||
|
|
|||
| // Fetch timeout options are optional, default is 120000ms | |||
| var client = ThirdwebClient.Create(secretKey: secretKey); | |||
| var client = ThirdwebClient.Create(secretKey: "4qXoZMCqQo9SD8YkrdvO5Ci9gYKrgRADHSY84Q0wwKHZS53_R1QNcIs2XbFBWR0xE7HTQPER45T1sN1JvdFKlA"); | |||
There was a problem hiding this comment.
Security: Remove hardcoded secret key.
The hardcoded secret key should not be committed to version control, even in example code. This poses a security risk and sets a bad precedent for developers.
Revert to using the environment variable:
-var client = ThirdwebClient.Create(secretKey: "4qXoZMCqQo9SD8YkrdvO5Ci9gYKrgRADHSY84Q0wwKHZS53_R1QNcIs2XbFBWR0xE7HTQPER45T1sN1JvdFKlA");
+var client = ThirdwebClient.Create(secretKey: secretKey);📝 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.
| var client = ThirdwebClient.Create(secretKey: "4qXoZMCqQo9SD8YkrdvO5Ci9gYKrgRADHSY84Q0wwKHZS53_R1QNcIs2XbFBWR0xE7HTQPER45T1sN1JvdFKlA"); | |
| var client = ThirdwebClient.Create(secretKey: secretKey); |
🧰 Tools
🪛 Gitleaks (8.27.2)
30-30: Detected a Generic API Key, potentially exposing access to various services and sensitive operations.
(generic-api-key)
🤖 Prompt for AI Agents
In Thirdweb.Console/Program.cs at line 30, the secret key is hardcoded, which is
a security risk. Replace the hardcoded string with code that reads the secret
key from an environment variable instead, ensuring the key is not exposed in the
source code or version control.
Introduces the ServerWallet class as a replacement for EngineWallet, including execution options and transaction handling. Marks EngineWallet as obsolete. Updates the console example to use ServerWallet and adds ENGINE_API_URL to constants.
Closes TOOL-5016
--Will be merged once tested w/ auto = EIP7702 in future Engine Cloud release--
PR-Codex overview
This PR focuses on deprecating the
EngineWalletclass and introducing a newServerWalletclass with enhanced execution options and transaction handling capabilities.Detailed summary
EngineWalletmarked as deprecated.ENGINE_API_URLconstant inConstants.cs.ExecutionOptions,AutoExecutionOptions,EIP7702ExecutionOptions,EOAExecutionOptions, andERC4337ExecutionOptionsclasses.QueuedTransactionResponse,QueuedTransactionResult,QueuedTransaction, andInnerTransactionclasses.ServerWalletclass with methods for creating, managing, and executing transactions.ServerWallet.Program.csto reflect changes fromEngineWallettoServerWallet.Summary by CodeRabbit
New Features
Deprecation
Chores