[SDK] Engine search transactions and batch transaction support #7190
Conversation
|
The latest updates on your projects. Learn more about Vercel for Git ↗︎
|
🦋 Changeset detectedLatest commit: 46aa336 The changes in this PR will be included in the next version bump. This PR includes changesets to release 3 packages
Not sure what this means? Click here to learn what changesets are. Click here if you're a maintainer who wants to add another changeset to this PR |
WalkthroughThis update enhances the Engine with server wallet management, transaction search, and batch transaction support. It introduces methods to create and list server wallets, search transactions with flexible filters and pagination, and enqueue or send multiple transactions as a batch. The transaction waiting logic is refactored into a dedicated module, and tests for these features are added. Changes
Sequence Diagram(s)sequenceDiagram
participant Client
participant Engine
participant ServerWallet
Client->>ServerWallet: enqueueBatchTransaction([tx1, tx2, ...])
ServerWallet->>Engine: Send batch transaction request
Engine-->>ServerWallet: Return batch transaction ID(s)
ServerWallet-->>Client: Return first transaction ID
Client->>ServerWallet: sendBatchTransaction([tx1, tx2, ...])
ServerWallet->>Engine: Send batch transaction request
Engine-->>ServerWallet: Return batch transaction ID(s)
ServerWallet->>Engine: waitForTransactionHash(transactionId)
Engine-->>ServerWallet: Transaction confirmed or failed
ServerWallet-->>Client: Return transaction hash or error
Client->>Engine: searchTransactions(filters, pageSize, page)
Engine-->>Client: Return filtered transaction list
Possibly related PRs
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 (
|
How to use the Graphite Merge QueueAdd either label to this PR to merge it via the merge queue:
You must have a Graphite account in order to use the merge queue. Sign up using this link. An organization admin has enabled the Graphite Merge Queue in this repository. Please do not merge from GitHub as this will restart CI on PRs being processed by the merge queue. This stack of pull requests is managed by Graphite. Learn more about stacking. |
joaquim-verges
changed the title
joaquim-verges
force-pushed
the
_Engine_Add_search_transactions_and_batch_transaction_support
branch
from
39b77f0 to
cb9786f
Compare
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (5)
packages/thirdweb/src/extensions/erc20/drop20.test.ts (1)
145-145: Fix variable name typo.There's a typo in the variable name
erc20ContractAddres- it should beerc20ContractAddress.- const erc20ContractAddres = await deployERC20Contract({ + const erc20ContractAddress = await deployERC20Contract({Also update the usage on lines 155 and 183:
- address: erc20ContractAddres, + address: erc20ContractAddress,- currencyAddress: erc20ContractAddres, + currencyAddress: erc20ContractAddress,packages/thirdweb/src/engine/wait-for-tx-hash.ts (1)
36-75: Robust polling implementation with comprehensive error handling.The polling logic is well-implemented with:
- Proper timeout checking in the while loop condition
- Comprehensive status handling for FAILED, CONFIRMED, and pending states
- Detailed error messages for different failure scenarios
- Appropriate 1-second polling interval to avoid excessive API calls
- Proper return structure matching
WaitForReceiptOptionstypeThe error handling covers:
- Transaction failures with error details
- Reverted transactions with revert data
- Timeout scenarios with clear messaging
Minor suggestion for consistency:
Consider extracting the polling interval (1000ms) as a configurable parameter or named constant for better maintainability.
+const POLLING_INTERVAL_MS = 1000; export async function waitForTransactionHash(args: { client: ThirdwebClient; transactionId: string; timeoutInSeconds?: number; + pollingIntervalMs?: number; }): Promise<WaitForReceiptOptions> { // ... existing code ... default: { // wait for the transaction to be confirmed - await new Promise((resolve) => setTimeout(resolve, 1000)); + await new Promise((resolve) => setTimeout(resolve, args.pollingIntervalMs ?? POLLING_INTERVAL_MS)); }packages/thirdweb/src/engine/search-transactions.ts (1)
132-134: Consider returning empty array instead of throwing for no results.Throwing an error when no transactions match the filters might not be the expected behavior. Search operations typically return empty results rather than errors when no matches are found.
- if (!data) { - throw new Error(`No transactions found with filters ${stringify(filters)}`); - } - - return data; + return data || [];packages/thirdweb/src/engine/server-wallet.ts (2)
167-176: Simplify redundant validation logic.The validation logic has redundant checks. The length check is sufficient, and the additional null check for
firstTransactionis unnecessary since we've already verified the array isn't empty.const enqueueTx = async (transaction: SendTransactionOption[]) => { if (transaction.length === 0) { throw new Error("No transactions to enqueue"); } - const firstTransaction = transaction[0]; - if (!firstTransaction) { - throw new Error("No transactions to enqueue"); - } - const chainId = firstTransaction.chainId; + const chainId = transaction[0].chainId;
232-238: Extract duplicate transaction ID validation logic.The logic for extracting the first transaction ID and throwing an error if none exists is duplicated. Consider extracting this into a helper function.
Add this helper function before the return statement:
+ const getFirstTransactionId = (transactionIds: string[]) => { + const transactionId = transactionIds[0]; + if (!transactionId) { + throw new Error("No transactionId returned from engine"); + } + return transactionId; + }; + return {Then update both methods:
const transactionIds = await enqueueTx([serializedTransaction]); - const transactionId = transactionIds[0]; - if (!transactionId) { - throw new Error("No transactionId returned from engine"); - } - return { transactionId }; + return { transactionId: getFirstTransactionId(transactionIds) };const transactionIds = await enqueueTx(serializedTransactions); - const transactionId = transactionIds[0]; - if (!transactionId) { - throw new Error("No transactionId returned from engine"); - } + const transactionId = getFirstTransactionId(transactionIds); return { transactionId };Also applies to: 256-262
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (8)
.changeset/engine-enhancements.md(1 hunks)packages/thirdweb/src/engine/get-status.ts(0 hunks)packages/thirdweb/src/engine/index.ts(1 hunks)packages/thirdweb/src/engine/search-transactions.ts(1 hunks)packages/thirdweb/src/engine/server-wallet.test.ts(2 hunks)packages/thirdweb/src/engine/server-wallet.ts(6 hunks)packages/thirdweb/src/engine/wait-for-tx-hash.ts(1 hunks)packages/thirdweb/src/extensions/erc20/drop20.test.ts(3 hunks)
💤 Files with no reviewable changes (1)
- packages/thirdweb/src/engine/get-status.ts
🧰 Additional context used
🧬 Code Graph Analysis (2)
packages/thirdweb/src/engine/wait-for-tx-hash.ts (4)
packages/thirdweb/src/engine/index.ts (2)
waitForTransactionHash(11-11)getTransactionStatus(7-7)packages/thirdweb/src/exports/thirdweb.ts (2)
ThirdwebClient(26-26)Hex(261-261)packages/thirdweb/src/engine/get-status.ts (1)
getTransactionStatus(74-118)packages/thirdweb/src/exports/utils.ts (1)
stringify(165-165)
packages/thirdweb/src/extensions/erc20/drop20.test.ts (2)
packages/thirdweb/src/exports/thirdweb.ts (2)
getContract(69-69)sendAndConfirmTransaction(140-140)packages/thirdweb/src/exports/utils.ts (1)
resolvePromisedValue(155-155)
⏰ Context from checks skipped due to timeout of 90000ms (8)
- GitHub Check: E2E Tests (pnpm, esbuild)
- GitHub Check: E2E Tests (pnpm, webpack)
- GitHub Check: E2E Tests (pnpm, vite)
- GitHub Check: Unit Tests
- GitHub Check: Lint Packages
- GitHub Check: Size
- GitHub Check: Build Packages
- GitHub Check: Analyze (javascript)
🔇 Additional comments (13)
packages/thirdweb/src/extensions/erc20/drop20.test.ts (3)
14-14: Import addition looks good.The
toWeiimport is properly used in the new test for precise amount comparisons.
23-24: New utility imports are correctly added.Both
getApprovalForTransactionandmintToare appropriately imported and used in the new ERC20 price test case.
140-227: Excellent test coverage for ERC20-priced token claims.This test comprehensively validates the ERC20 payment flow for token claims:
- ✅ Deploys separate ERC20 contract for payment currency
- ✅ Mints test tokens to the claiming account
- ✅ Sets claim conditions with ERC20 currency address
- ✅ Verifies correct ERC20 value calculation (2 tokens × 0.01 = 0.02)
- ✅ Handles approval workflow for ERC20 spending
- ✅ Confirms both token claim success and payment deduction
The test logic is sound and provides good coverage of the ERC20 payment functionality. The assertions properly verify both sides of the transaction - that tokens were received and that payment was correctly deducted.
packages/thirdweb/src/engine/index.ts (1)
11-15:✅ Verification successful
LGTM! Export reorganization looks good.
The export changes properly reflect the modularization of functionality:
waitForTransactionHashmoved to a dedicated module for better separation of concerns- New search functionality exports added for the enhanced Engine capabilities
Let me verify that the referenced modules exist and contain the expected exports:
🏁 Script executed:
#!/bin/bash # Verify the existence and exports of the referenced modules echo "Checking wait-for-tx-hash.js module:" fd -e ts "wait-for-tx-hash" --exec cat {} \; | head -30 echo -e "\n\nChecking search-transactions.js module:" fd -e ts "search-transactions" --exec cat {} \; | head -30Length of output: 2277
Verified exports — ready to merge
Both new modules exist with the expected exports:
waitForTransactionHashis defined and exported inwait-for-tx-hash.js.searchTransactionsandSearchTransactionsArgsare defined and exported insearch-transactions.js.All exports are correctly wired in
index.ts.packages/thirdweb/src/engine/server-wallet.test.ts (2)
99-107: Good test coverage for transaction search functionality.The test properly validates the new
Engine.searchTransactionsmethod by:
- Searching for a specific transaction ID using appropriate filters
- Asserting the response structure and content
- Verifying that exactly one transaction is returned matching the enqueued transaction ID
128-153: Comprehensive test for batch transaction functionality.The test thoroughly validates the new batch transaction feature by:
- Creating two distinct mint transactions
- Enqueuing them as a batch using
serverWallet.enqueueBatchTransaction- Waiting for completion using
Engine.waitForTransactionHash- Asserting the presence of both transaction ID and hash
The test structure follows good practices and properly exercises the new functionality.
packages/thirdweb/src/engine/wait-for-tx-hash.ts (1)
26-35: Function signature and timeout setup look good.The function properly:
- Accepts required parameters with optional timeout configuration
- Sets a reasonable default timeout of 5 minutes
- Records start time for timeout calculations
.changeset/engine-enhancements.md (1)
1-75: Comprehensive and well-structured changeset documentation.The changeset properly documents the new Engine enhancements with:
- Appropriate minor version bump classification
- Clear feature descriptions for both search and batch functionality
- Practical code examples showing real usage patterns
- Proper syntax and structure for the changeset format
The examples demonstrate:
- Basic transaction search by ID
- Advanced filtering with multiple criteria and logical operations
- Batch transaction enqueuing with proper error handling
- Integration with the wait functionality
packages/thirdweb/src/engine/search-transactions.ts (1)
18-108: Excellent documentation with comprehensive examples.The JSDoc documentation is thorough and provides multiple practical examples covering different use cases including basic searches, combined filters, and pagination. This will greatly help developers understand how to use the function effectively.
packages/thirdweb/src/engine/server-wallet.ts (4)
22-22: Import update looks good.The relocation of
waitForTransactionHashto a dedicated module improves code organization.
57-60: New batch transaction method properly added to interface.The
enqueueBatchTransactionmethod is correctly added to theServerWallettype with appropriate parameter types.
108-138: Excellent documentation for batch transaction usage.The new documentation examples clearly demonstrate how to use the batch transaction functionality, including proper setup and polling for completion. This provides great guidance for developers.
274-284: New batch send method implementation looks solid.The
sendBatchTransactionmethod correctly handles multiple transactions and uses the same waiting logic as the single transaction method. The implementation is consistent with the existing patterns.
size-limit report 📦
|
joaquim-verges
force-pushed
the
_Engine_Add_search_transactions_and_batch_transaction_support
branch
from
cb9786f to
ea3e997
Compare
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #7190 +/- ##
==========================================
- Coverage 55.68% 55.62% -0.06%
==========================================
Files 904 908 +4
Lines 58392 58530 +138
Branches 4122 4126 +4
==========================================
+ Hits 32516 32558 +42
- Misses 25770 25866 +96
Partials 106 106
🚀 New features to boost your workflow:
|
joaquim-verges
force-pushed
the
_Engine_Add_search_transactions_and_batch_transaction_support
branch
from
ea3e997 to
16958c9
Compare
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (1)
packages/thirdweb/src/engine/server-wallet.ts (1)
177-183: Chain validation properly implemented.This addresses the previous review comment about validating chain consistency for batch transactions. The implementation correctly ensures all transactions are on the same chain before processing.
🧹 Nitpick comments (1)
packages/thirdweb/src/engine/server-wallet.ts (1)
167-184: Simplify redundant validation checks.The function has redundant validation for empty arrays and undefined transactions. The first check is sufficient.
- const enqueueTx = async (transaction: SendTransactionOption[]) => { - if (transaction.length === 0) { - throw new Error("No transactions to enqueue"); - } - const firstTransaction = transaction[0]; - if (!firstTransaction) { - throw new Error("No transactions to enqueue"); - } + const enqueueTx = async (transaction: SendTransactionOption[]) => { + if (transaction.length === 0) { + throw new Error("No transactions to enqueue"); + } + const firstTransaction = transaction[0]!;
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (8)
.changeset/engine-enhancements.md(1 hunks)packages/thirdweb/src/engine/get-status.ts(0 hunks)packages/thirdweb/src/engine/index.ts(1 hunks)packages/thirdweb/src/engine/search-transactions.ts(1 hunks)packages/thirdweb/src/engine/server-wallet.test.ts(2 hunks)packages/thirdweb/src/engine/server-wallet.ts(6 hunks)packages/thirdweb/src/engine/wait-for-tx-hash.ts(1 hunks)packages/thirdweb/src/extensions/erc20/drop20.test.ts(3 hunks)
💤 Files with no reviewable changes (1)
- packages/thirdweb/src/engine/get-status.ts
🚧 Files skipped from review as they are similar to previous changes (6)
- packages/thirdweb/src/engine/server-wallet.test.ts
- packages/thirdweb/src/engine/search-transactions.ts
- packages/thirdweb/src/engine/index.ts
- packages/thirdweb/src/extensions/erc20/drop20.test.ts
- .changeset/engine-enhancements.md
- packages/thirdweb/src/engine/wait-for-tx-hash.ts
⏰ Context from checks skipped due to timeout of 90000ms (8)
- GitHub Check: E2E Tests (pnpm, esbuild)
- GitHub Check: Size
- GitHub Check: Build Packages
- GitHub Check: E2E Tests (pnpm, webpack)
- GitHub Check: Lint Packages
- GitHub Check: E2E Tests (pnpm, vite)
- GitHub Check: Unit Tests
- GitHub Check: Analyze (javascript)
🔇 Additional comments (6)
packages/thirdweb/src/engine/server-wallet.ts (6)
22-22: LGTM: Import path updated correctly.The import has been properly updated to reflect the new module location for
waitForTransactionHash.
57-60: LGTM: Type definition for batch transactions.The new
enqueueBatchTransactionmethod is properly typed to accept an array ofPreparedTransactionobjects and return a single transaction ID, which is appropriate for batch operations.
108-138: LGTM: Comprehensive documentation for batch transactions.The JSDoc examples clearly demonstrate how to use the new batch transaction functionality, including preparation, enqueuing, and polling for results.
185-191: LGTM: Proper batch transaction serialization.The execution options and parameters are correctly mapped from the transaction array, with appropriate type handling for optional values.
240-270: LGTM: Consistent implementation pattern.Both
enqueueTransactionandenqueueBatchTransactionmethods follow the same pattern: serialize transactions, callenqueueTx, and extract the first transaction ID. The error handling is appropriate.
282-292: LGTM: Batch transaction sending implementation.The
sendBatchTransactionmethod correctly handles multiple transactions and waits for the batch completion using the first transaction ID, which is the expected behavior for batch operations.
joaquim-verges
force-pushed
the
_Engine_Add_search_transactions_and_batch_transaction_support
branch
from
7056a9f to
07db3e7
Compare
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (1)
packages/thirdweb/src/engine/server-wallet.ts (1)
167-183: LGTM! Chain validation addresses previous review concern.The implementation correctly validates that all transactions in a batch are on the same chain, which addresses the concern raised in the previous review. The error message is descriptive and includes the specific index where the mismatch occurs.
🧹 Nitpick comments (2)
packages/thirdweb/src/extensions/erc1155/read/getOwnedNFTs.test.ts (1)
12-13: Consider making test assertions more resilient.The hardcoded assertions (expecting exactly 3 NFTs and quantity 411n for ID 4) could be brittle if the test contract state changes. Consider using more flexible assertions or ensuring the test data is stable.
- expect(nfts.length).toBe(3); - expect(nfts.find((nft) => nft.id === 4n)?.quantityOwned).toBe(411n); + expect(nfts.length).toBeGreaterThan(0); + const nftWithId4 = nfts.find((nft) => nft.id === 4n); + expect(nftWithId4).toBeDefined(); + expect(nftWithId4?.quantityOwned).toBeGreaterThan(0n);Alternatively, ensure the test contract data remains stable or document the expected state.
Also applies to: 22-23
packages/thirdweb/src/engine/server-wallet.ts (1)
247-270: Consider optimizing transaction serialization for better performance.The current implementation processes transactions sequentially in a loop. Consider using
Promise.allto serialize all transactions in parallel for better performance with large batches.- const serializedTransactions: SendTransactionOption[] = []; - for (const transaction of args.transactions) { - const [to, data, value] = await Promise.all([ - transaction.to ? resolvePromisedValue(transaction.to) : null, - encode(transaction), - transaction.value ? resolvePromisedValue(transaction.value) : null, - ]); - serializedTransactions.push({ - chainId: transaction.chain.id, - data, - to: to ?? undefined, - value: value ?? undefined, - }); - } + const serializedTransactions = await Promise.all( + args.transactions.map(async (transaction) => { + const [to, data, value] = await Promise.all([ + transaction.to ? resolvePromisedValue(transaction.to) : null, + encode(transaction), + transaction.value ? resolvePromisedValue(transaction.value) : null, + ]); + return { + chainId: transaction.chain.id, + data, + to: to ?? undefined, + value: value ?? undefined, + }; + }) + );
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (13)
.changeset/engine-enhancements.md(1 hunks).changeset/stupid-adults-flow.md(1 hunks)packages/engine/src/client/types.gen.ts(1 hunks)packages/thirdweb/src/bridge/Chains.test.ts(1 hunks)packages/thirdweb/src/engine/get-status.ts(0 hunks)packages/thirdweb/src/engine/index.ts(1 hunks)packages/thirdweb/src/engine/search-transactions.ts(1 hunks)packages/thirdweb/src/engine/server-wallet.test.ts(2 hunks)packages/thirdweb/src/engine/server-wallet.ts(6 hunks)packages/thirdweb/src/engine/wait-for-tx-hash.ts(1 hunks)packages/thirdweb/src/extensions/erc1155/read/getOwnedNFTs.test.ts(1 hunks)packages/thirdweb/src/extensions/erc20/drop20.test.ts(3 hunks)packages/thirdweb/src/insight/get-nfts.ts(1 hunks)
💤 Files with no reviewable changes (1)
- packages/thirdweb/src/engine/get-status.ts
✅ Files skipped from review due to trivial changes (1)
- .changeset/stupid-adults-flow.md
🚧 Files skipped from review as they are similar to previous changes (6)
- packages/thirdweb/src/engine/server-wallet.test.ts
- packages/thirdweb/src/engine/search-transactions.ts
- .changeset/engine-enhancements.md
- packages/thirdweb/src/engine/wait-for-tx-hash.ts
- packages/thirdweb/src/extensions/erc20/drop20.test.ts
- packages/thirdweb/src/engine/index.ts
🧰 Additional context used
🧬 Code Graph Analysis (2)
packages/thirdweb/src/extensions/erc1155/read/getOwnedNFTs.test.ts (1)
packages/thirdweb/src/insight/get-nfts.ts (1)
getOwnedNFTs(32-89)
packages/thirdweb/src/engine/server-wallet.ts (5)
packages/thirdweb/src/exports/thirdweb.ts (2)
PreparedTransaction(104-104)encode(126-126)packages/thirdweb/src/wallets/interfaces/wallet.ts (1)
SendTransactionOption(23-26)packages/thirdweb/src/exports/utils.ts (1)
resolvePromisedValue(155-155)packages/thirdweb/src/engine/index.ts (1)
waitForTransactionHash(11-11)packages/thirdweb/src/engine/wait-for-tx-hash.ts (1)
waitForTransactionHash(26-75)
⏰ Context from checks skipped due to timeout of 90000ms (1)
- GitHub Check: Analyze (javascript)
🔇 Additional comments (10)
packages/thirdweb/src/bridge/Chains.test.ts (1)
5-5: Good practice: Conditional test execution for authenticated tests.This change properly gates tests that require secret keys behind an environment variable check, preventing failures in environments where credentials aren't available.
packages/thirdweb/src/insight/get-nfts.ts (1)
322-325: Well-implemented NFT enhancement with ownership quantity.The addition of
quantityOwnedproperty correctly converts the balance toBigIntwhen available and maintains the optional property semantics. This enhancement aligns well with the function's existing return type signature.packages/thirdweb/src/extensions/erc1155/read/getOwnedNFTs.test.ts (1)
6-25: Comprehensive test coverage with good conditional execution.The test suite properly covers both indexer and non-indexer scenarios for the
getOwnedNFTsfunction. The conditional execution pattern is consistent with other test files.packages/engine/src/client/types.gen.ts (1)
12-12: LGTM! Appropriate type enhancement for flexible filtering.Making the
operationproperty optional inTransactionsFilterValuecorrectly supports the new flexible transaction search functionality where logical operations may not always be required.packages/thirdweb/src/engine/server-wallet.ts (6)
22-22: LGTM! Import updated to use dedicated module.The import change from
./index.jsto./wait-for-tx-hash.jsfollows the refactoring mentioned in the summary where transaction waiting logic was moved to a dedicated module.
57-60: LGTM! Well-designed batch transaction type addition.The
enqueueBatchTransactionmethod signature is consistent with the existingenqueueTransactionmethod and appropriately accepts an array of transactions while returning a single transaction ID for the batch.
108-138: LGTM! Comprehensive documentation for batch transactions.The documentation clearly explains how to use the new batch transaction functionality with practical examples, maintaining consistency with the existing documentation style.
184-210: LGTM! Well-implemented batch-capable transaction enqueuing.The refactored
enqueueTxfunction properly:
- Handles both single and batch transactions
- Maps transaction data to the expected API format
- Returns an array of transaction IDs for flexibility
- Maintains proper error handling
240-246: LGTM! Backward-compatible single transaction enqueuing.The updated
enqueueTransactionmethod maintains backward compatibility by wrapping the single transaction in an array and extracting the first ID from the result, with appropriate error handling.
271-292: LGTM! Consistent implementation of send methods.Both
sendTransactionandsendBatchTransactionmethods follow the same pattern of enqueuing transactions and then waiting for confirmation, maintaining consistency in the API design.
joaquim-verges
force-pushed
the
_Engine_Add_search_transactions_and_batch_transaction_support
branch
from
07db3e7 to
10a69d3
Compare
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (1)
packages/thirdweb/src/engine/server-wallet.ts (1)
176-183: LGTM: Chain validation correctly addresses previous review feedback.The chain validation logic properly ensures all transactions in a batch are on the same chain, which addresses the concern raised in the previous review comment. The error message is clear and includes helpful context.
🧹 Nitpick comments (2)
packages/thirdweb/src/engine/server-wallet.ts (2)
247-269: Consider refactoring to reduce code duplication.The transaction serialization logic in
enqueueBatchTransactionduplicates the serialization code fromenqueueTransaction. This could be extracted into a shared helper function to improve maintainability.Consider extracting the serialization logic:
+ const serializeTransaction = async (transaction: PreparedTransaction): Promise<SendTransactionOption> => { + const [to, data, value] = await Promise.all([ + transaction.to ? resolvePromisedValue(transaction.to) : null, + encode(transaction), + transaction.value ? resolvePromisedValue(transaction.value) : null, + ]); + return { + chainId: transaction.chain.id, + data, + to: to ?? undefined, + value: value ?? undefined, + }; + }; enqueueBatchTransaction: async (args: { transactions: PreparedTransaction[]; }) => { - const serializedTransactions: SendTransactionOption[] = []; - for (const transaction of args.transactions) { - const [to, data, value] = await Promise.all([ - transaction.to ? resolvePromisedValue(transaction.to) : null, - encode(transaction), - transaction.value ? resolvePromisedValue(transaction.value) : null, - ]); - serializedTransactions.push({ - chainId: transaction.chain.id, - data, - to: to ?? undefined, - value: value ?? undefined, - }); - } + const serializedTransactions = await Promise.all( + args.transactions.map(serializeTransaction) + );
167-170: Add validation for maximum batch size.Consider adding a maximum batch size limit to prevent potential performance issues or API limits with very large batches.
const enqueueTx = async (transaction: SendTransactionOption[]) => { if (transaction.length === 0) { throw new Error("No transactions to enqueue"); } + if (transaction.length > 100) { // or whatever the appropriate limit is + throw new Error(`Batch size too large: ${transaction.length}. Maximum allowed: 100`); + }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (16)
.changeset/engine-enhancements.md(1 hunks).changeset/stupid-adults-flow.md(1 hunks)packages/engine/src/client/sdk.gen.ts(3 hunks)packages/engine/src/client/types.gen.ts(2 hunks)packages/thirdweb/src/bridge/Chains.test.ts(1 hunks)packages/thirdweb/src/engine/create-server-wallet.ts(1 hunks)packages/thirdweb/src/engine/get-status.ts(0 hunks)packages/thirdweb/src/engine/index.ts(1 hunks)packages/thirdweb/src/engine/list-server-wallets.ts(1 hunks)packages/thirdweb/src/engine/search-transactions.ts(1 hunks)packages/thirdweb/src/engine/server-wallet.test.ts(3 hunks)packages/thirdweb/src/engine/server-wallet.ts(6 hunks)packages/thirdweb/src/engine/wait-for-tx-hash.ts(1 hunks)packages/thirdweb/src/extensions/erc1155/read/getOwnedNFTs.test.ts(1 hunks)packages/thirdweb/src/extensions/erc20/drop20.test.ts(3 hunks)packages/thirdweb/src/insight/get-nfts.ts(1 hunks)
💤 Files with no reviewable changes (1)
- packages/thirdweb/src/engine/get-status.ts
🚧 Files skipped from review as they are similar to previous changes (10)
- .changeset/stupid-adults-flow.md
- packages/thirdweb/src/insight/get-nfts.ts
- packages/thirdweb/src/bridge/Chains.test.ts
- packages/thirdweb/src/engine/server-wallet.test.ts
- packages/thirdweb/src/engine/search-transactions.ts
- packages/thirdweb/src/engine/wait-for-tx-hash.ts
- packages/thirdweb/src/extensions/erc1155/read/getOwnedNFTs.test.ts
- .changeset/engine-enhancements.md
- packages/thirdweb/src/extensions/erc20/drop20.test.ts
- packages/thirdweb/src/engine/index.ts
🧰 Additional context used
🧬 Code Graph Analysis (1)
packages/engine/src/client/sdk.gen.ts (1)
packages/engine/src/client/types.gen.ts (4)
ListAccountsData(95-100)ListAccountsResponse(120-121)CreateAccountData(123-136)CreateAccountResponse(156-157)
⏰ Context from checks skipped due to timeout of 90000ms (8)
- GitHub Check: E2E Tests (pnpm, esbuild)
- GitHub Check: Unit Tests
- GitHub Check: E2E Tests (pnpm, webpack)
- GitHub Check: Lint Packages
- GitHub Check: E2E Tests (pnpm, vite)
- GitHub Check: Size
- GitHub Check: Build Packages
- GitHub Check: Analyze (javascript)
🔇 Additional comments (11)
packages/thirdweb/src/engine/server-wallet.ts (4)
22-22: LGTM: Import update for modular organization.The import update from
"./get-status.js"to"./wait-for-tx-hash.js"aligns with the refactoring mentioned in the AI summary, improving code modularity.
57-60: LGTM: Clean type definition for batch transactions.The addition of
enqueueBatchTransactionto theServerWallettype provides a clear contract for batch transaction processing while maintaining consistency with the existingenqueueTransactionmethod.
108-137: LGTM: Comprehensive documentation for batch transactions.The documentation examples clearly demonstrate how to use the new batch transaction functionality, including proper preparation, enqueueing, and polling patterns.
282-292: LGTM: Consistent implementation of batch send functionality.The
sendBatchTransactionmethod correctly mirrors the pattern ofsendTransactionbut for batches, maintaining consistency in the API design.packages/thirdweb/src/engine/list-server-wallets.ts (1)
1-46: LGTM! Well-structured server wallet listing function.The implementation follows established patterns in the codebase with proper error handling, clear documentation, and appropriate use of utility functions. The error messages are descriptive and provide good context for debugging.
packages/thirdweb/src/engine/create-server-wallet.ts (1)
1-57: LGTM! Consistent and well-implemented server wallet creation function.The function follows the same high-quality patterns as the listing function, with proper error handling that includes contextual information (the label) in error messages. The documentation and type definitions are clear and comprehensive.
packages/engine/src/client/types.gen.ts (2)
12-12: Good improvement: Making operation field optional.Making the
operationfield optional inTransactionsFilterValueprovides better flexibility for simple filter use cases while maintaining backwards compatibility.
95-157: Well-structured account management types.The new account management types (
ListAccountsData,CreateAccountData, and their response types) follow established patterns and include appropriate optional fields like the vault access token header for account operations.packages/engine/src/client/sdk.gen.ts (3)
10-11: Proper type imports for new account functions.The type imports for
CreateAccountData,CreateAccountResponse,ListAccountsData, andListAccountsResponseare correctly added to support the new account management functions.Also applies to: 20-21
55-76: Well-implemented listAccounts function.The
listAccountsfunction correctly implements the GET endpoint pattern with proper security configuration using the API key. The function signature and return types are consistent with other SDK functions.
78-103: Proper createAccount function implementation.The
createAccountfunction correctly implements the POST endpoint pattern with appropriate Content-Type header for JSON requests and proper security configuration. The function follows established patterns in the SDK.
joaquim-verges
force-pushed
the
_Engine_Add_search_transactions_and_batch_transaction_support
branch
from
10a69d3 to
b652621
Compare
joaquim-verges
force-pushed
the
_Engine_Add_search_transactions_and_batch_transaction_support
branch
from
b652621 to
46aa336
Compare
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (1)
packages/thirdweb/src/engine/server-wallet.ts (1)
167-210: Excellent refactoring with proper validation.The
enqueueTxfunction has been well-refactored to handle batch transactions with comprehensive validation:
- Empty array validation prevents invalid calls
- Chain consistency validation ensures all transactions are on the same chain (addresses previous review feedback)
- Proper error handling with descriptive messages
- Clean mapping to engine format
🧹 Nitpick comments (1)
packages/thirdweb/src/engine/server-wallet.ts (1)
247-270: Batch transaction implementation follows good patterns.The new
enqueueBatchTransactionmethod is well-implemented with proper serialization and error handling. The method correctly processes multiple transactions and maintains API consistency by returning the first transaction ID.Consider adding a comment or documentation note that clarifies this method returns only the first transaction ID from the batch, which users might find helpful for understanding the expected behavior.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (16)
.changeset/engine-enhancements.md(1 hunks).changeset/stupid-adults-flow.md(1 hunks)packages/engine/src/client/sdk.gen.ts(3 hunks)packages/engine/src/client/types.gen.ts(2 hunks)packages/thirdweb/src/bridge/Chains.test.ts(1 hunks)packages/thirdweb/src/engine/create-server-wallet.ts(1 hunks)packages/thirdweb/src/engine/get-status.ts(0 hunks)packages/thirdweb/src/engine/index.ts(1 hunks)packages/thirdweb/src/engine/list-server-wallets.ts(1 hunks)packages/thirdweb/src/engine/search-transactions.ts(1 hunks)packages/thirdweb/src/engine/server-wallet.test.ts(3 hunks)packages/thirdweb/src/engine/server-wallet.ts(6 hunks)packages/thirdweb/src/engine/wait-for-tx-hash.ts(1 hunks)packages/thirdweb/src/extensions/erc1155/read/getOwnedNFTs.test.ts(1 hunks)packages/thirdweb/src/extensions/erc20/drop20.test.ts(3 hunks)packages/thirdweb/src/insight/get-nfts.ts(1 hunks)
💤 Files with no reviewable changes (1)
- packages/thirdweb/src/engine/get-status.ts
✅ Files skipped from review due to trivial changes (1)
- packages/thirdweb/src/extensions/erc1155/read/getOwnedNFTs.test.ts
🚧 Files skipped from review as they are similar to previous changes (13)
- .changeset/stupid-adults-flow.md
- packages/thirdweb/src/engine/list-server-wallets.ts
- packages/thirdweb/src/engine/create-server-wallet.ts
- packages/thirdweb/src/bridge/Chains.test.ts
- packages/thirdweb/src/engine/server-wallet.test.ts
- .changeset/engine-enhancements.md
- packages/thirdweb/src/insight/get-nfts.ts
- packages/thirdweb/src/extensions/erc20/drop20.test.ts
- packages/thirdweb/src/engine/wait-for-tx-hash.ts
- packages/thirdweb/src/engine/search-transactions.ts
- packages/engine/src/client/types.gen.ts
- packages/thirdweb/src/engine/index.ts
- packages/engine/src/client/sdk.gen.ts
🧰 Additional context used
🧬 Code Graph Analysis (1)
packages/thirdweb/src/engine/server-wallet.ts (4)
packages/thirdweb/src/exports/thirdweb.ts (1)
encode(126-126)packages/thirdweb/src/wallets/interfaces/wallet.ts (1)
SendTransactionOption(23-26)packages/thirdweb/src/exports/utils.ts (1)
resolvePromisedValue(155-155)packages/thirdweb/src/engine/wait-for-tx-hash.ts (1)
waitForTransactionHash(26-75)
⏰ Context from checks skipped due to timeout of 90000ms (8)
- GitHub Check: E2E Tests (pnpm, esbuild)
- GitHub Check: E2E Tests (pnpm, webpack)
- GitHub Check: Unit Tests
- GitHub Check: E2E Tests (pnpm, vite)
- GitHub Check: Size
- GitHub Check: Build Packages
- GitHub Check: Lint Packages
- GitHub Check: Analyze (javascript)
🔇 Additional comments (6)
packages/thirdweb/src/engine/server-wallet.ts (6)
22-22: Import refactoring looks good.The
waitForTransactionHashfunction has been properly moved to its dedicated module, improving code organization.
57-59: Type extension for batch transactions is well-designed.The new
enqueueBatchTransactionmethod follows the same pattern as the existingenqueueTransactionmethod, maintaining API consistency.
214-246: Clean refactoring maintains backward compatibility.The
enqueueTransactionmethod has been effectively refactored to use the new batch-capableenqueueTxfunction while preserving all existing functionality and error handling.
271-281: Send transaction refactoring maintains functionality.The
sendTransactionmethod has been properly refactored to use the newenqueueTxfunction while preserving the same enqueue-and-wait behavior.
282-292: Batch send implementation needs behavioral clarification.The
sendBatchTransactionmethod follows good patterns but only waits for the first transaction hash. Consider whether users would expect to wait for all transactions in the batch to complete, or if waiting for just the first is the intended behavior.If the current behavior is intentional, adding documentation about this would help set proper expectations.
108-137: Excellent documentation with practical examples.The new batch transaction documentation is comprehensive and follows the existing documentation patterns perfectly. The examples clearly demonstrate the workflow for batch transactions, making it easy for developers to understand and implement.
joaquim-verges
deleted the
_Engine_Add_search_transactions_and_batch_transaction_support
branch
PR-Codex overview
This PR focuses on enhancing the
Enginefunctionality by introducing server wallet management, transaction searching, and batch transaction support.Detailed summary
Engine.createServerWallet()for creating server wallets.Engine.getServerWallets()to list existing server wallets.Engine.searchTransactions()for searching transactions.serverWallet.enqueueBatchTransaction()for batch transactions.waitForTransactionHashfunction to include detailed error reporting.Summary by CodeRabbit
New Features
Bug Fixes
Tests