fix/PRO-3119/transaction-hash-sent-batch#176
Conversation
WalkthroughVersion 1.0.3 introduces new methods and parameters to improve transaction management. A new Changes
Sequence Diagram(s)sequenceDiagram
participant C as Client
participant P as EtherspotTransactionKitContextProvider
participant M as ModularSdk
C->>P: call send(userOpHash, ...transactionHash)
P->>M: getUserOpReceipt(userOpHash)
alt Receipt obtained within timeout
M-->>P: Return receipt (transaction hash)
P->>P: Append receipt to sentBatches
P-->>C: Return success with transaction details
else Timeout reached
P->>P: Log warning for missing receipt
P-->>C: Return failure/undefined result
end
Note over P: getTransactionHash follows a similar flow with retries and logging.
Poem
📜 Recent review detailsConfiguration used: CodeRabbit UI 📒 Files selected for processing (2)
🔇 Additional comments (3)
✨ Finishing Touches
🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
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: 2
🧹 Nitpick comments (4)
src/contexts/EtherspotTransactionKitContext.tsx (1)
17-20: API design for getTransactionHash is clear and consistentThe signature for
getTransactionHashfollows the same pattern as other asynchronous methods in the interface, taking clear parameters (userOpHash and batchId) and returning an appropriate Promise type.However, I suggest documenting the purpose of this method with JSDoc comments:
+/** + * Gets the transaction hash for a given user operation hash and batch ID + * @param userOpHash - The hash of the user operation + * @param batchId - The ID of the batch + * @returns The transaction hash if found, or null if not available + */ getTransactionHash: ( userOpHash: string, batchId: number ) => Promise<string | null>;CHANGELOG.md (1)
3-8: Version bump with clear documentation of new features.Nice work documenting the changes in the new version. The changelog clearly explains what was added and follows the established format.
One minor grammar suggestion: Since "userOp" starts with a vowel sound, consider changing "a userOp hash" to "an userOp hash" in line 7.
🧰 Tools
🪛 LanguageTool
[style] ~8-~8: Three successive sentences begin with the same word. Consider rewording the sentence or use a thesaurus to find a synonym.
Context: ...transaction hash with an userOp hash. - AddedtransactionHashtosend()in `Ether...(ENGLISH_WORD_REPEAT_BEGINNING_RULE)
src/providers/EtherspotTransactionKitContextProvider.tsx (2)
216-232: Clarify variable naming for better readability.The variable
userOpsReceiptsuggests it contains a full receipt object, but it appears to actually store just the transaction hash. Consider renaming this variable to better reflect its content.- let userOpsReceipt = null; + let transactionHash = null; const timeout = Date.now() + 30 * 1000; // 30 seconds timeout - while (!userOpsReceipt && Date.now() < timeout) { + while (!transactionHash && Date.now() < timeout) { await new Promise<void>((resolve) => { setTimeout(resolve, 2000); }); // Retry every 2 sec try { - userOpsReceipt = + transactionHash = await etherspotModulaSdk.getUserOpReceipt(userOpHash); } catch (error) { console.error('Error fetching transaction hash:', error); } } - if (!userOpsReceipt) { + if (!transactionHash) {
271-297: Extract common retry logic to reduce code duplication.The
getTransactionHashfunction contains similar retry logic to what's in thesendfunction. Consider extracting this logic into a reusable helper function to avoid duplication.Here's how you could refactor this:
// Helper function to retry an operation with timeout const retryWithTimeout = async <T>( operation: () => Promise<T>, timeoutMs: number = 30000, intervalMs: number = 2000 ): Promise<T | null> => { let result = null; const timeout = Date.now() + timeoutMs; while (!result && Date.now() < timeout) { await new Promise<void>((resolve) => { setTimeout(resolve, intervalMs); }); try { result = await operation(); if (result) break; } catch (error) { console.error('Error during retry operation:', error); } } if (!result) { console.warn(`Operation timed out after ${timeoutMs}ms`); } return result; }; // Then use it in getTransactionHash const getTransactionHash = async ( userOpHash: string, batchId: number ): Promise<string | null> => { const etherspotModulaSdk = await getSdk(batchId); return await retryWithTimeout( () => etherspotModulaSdk.getUserOpReceipt(userOpHash), 30000 ); }; // And similarly in the send function const userOpsReceipt = await retryWithTimeout( () => etherspotModulaSdk.getUserOpReceipt(userOpHash) );
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
package-lock.jsonis excluded by!**/package-lock.json
📒 Files selected for processing (8)
CHANGELOG.md(1 hunks)__mocks__/@etherspot/modular-sdk.js(1 hunks)__tests__/hooks/useEtherspotTransactions.test.js(22 hunks)package.json(1 hunks)src/contexts/EtherspotTransactionKitContext.tsx(1 hunks)src/hooks/useWalletAddress.ts(0 hunks)src/providers/EtherspotTransactionKitContextProvider.tsx(2 hunks)src/types/EtherspotTransactionKit.ts(1 hunks)
💤 Files with no reviewable changes (1)
- src/hooks/useWalletAddress.ts
🧰 Additional context used
🪛 LanguageTool
CHANGELOG.md
[style] ~8-~8: Three successive sentences begin with the same word. Consider rewording the sentence or use a thesaurus to find a synonym.
Context: ...transaction hash with an userOp hash. - Added transactionHash to send() in `Ether...
(ENGLISH_WORD_REPEAT_BEGINNING_RULE)
🔇 Additional comments (7)
package.json (1)
4-4: Version bump 1.0.2 → 1.0.3 reflects addition of transaction hash functionalityThe version increment from 1.0.2 to 1.0.3 appropriately follows semantic versioning for a non-breaking functionality addition.
src/types/EtherspotTransactionKit.ts (1)
38-38: Good addition of transactionHash to SentBatch interfaceThe addition of an optional
transactionHashproperty to theSentBatchinterface is appropriate and aligns with the PR objective to expose transaction hashes. Making it optional maintains backward compatibility.__mocks__/@etherspot/modular-sdk.js (1)
110-112: Mock implementation for getUserOpReceipt looks goodThis mock implementation correctly handles the
getUserOpReceiptmethod for testing purposes, returning a formatted string based on the input hash. This addition supports testing the new transaction hash retrieval functionality.__tests__/hooks/useEtherspotTransactions.test.js (3)
302-326: Good implementation of timer mocks for testing async timeout behavior.The use of Jest's fake timers to test the asynchronous timeout behavior is appropriate. This approach allows you to simulate the 30-second timeout without actually waiting that long during tests.
367-387: Consistent usage of timer mocks throughout tests.The pattern of using fake timers, advancing time, and then restoring real timers is consistently applied throughout the test file, which is good practice.
446-496: Well-structured test for multiple batch operations.The test correctly verifies the handling of multiple grouped batches, including the timeout behavior. The increased timeout value (31000ms) gives enough buffer for the test to complete after the simulated 30-second delay.
src/providers/EtherspotTransactionKitContextProvider.tsx (1)
299-320: Added getTransactionHash to context data.The new
getTransactionHashfunction is properly added to the context data, making it available to components that consume this context.
|
|
||
| while (!transactionHash && Date.now() < timeout) { | ||
| await new Promise<void>((resolve) => { | ||
| setTimeout(resolve, 2000); | ||
| }); // Retry every 2 sec | ||
|
|
||
| try { | ||
| transactionHash = await etherspotModulaSdk.getUserOpReceipt(userOpHash); | ||
| } catch (error) { | ||
| console.error('Error fetching transaction hash:', error); | ||
| } | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Add early exit to the retry loop in getTransactionHash.
Similar to the recommendation for the send function, the retry loop should exit early once a transaction hash is successfully retrieved.
while (!transactionHash && Date.now() < timeout) {
await new Promise<void>((resolve) => {
setTimeout(resolve, 2000);
}); // Retry every 2 sec
try {
transactionHash = await etherspotModulaSdk.getUserOpReceipt(userOpHash);
+ if (transactionHash) {
+ break; // Exit the loop once we have the transaction hash
+ }
} catch (error) {
console.error('Error fetching transaction hash:', error);
}
}📝 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.
| while (!transactionHash && Date.now() < timeout) { | |
| await new Promise<void>((resolve) => { | |
| setTimeout(resolve, 2000); | |
| }); // Retry every 2 sec | |
| try { | |
| transactionHash = await etherspotModulaSdk.getUserOpReceipt(userOpHash); | |
| } catch (error) { | |
| console.error('Error fetching transaction hash:', error); | |
| } | |
| } | |
| while (!transactionHash && Date.now() < timeout) { | |
| await new Promise<void>((resolve) => { | |
| setTimeout(resolve, 2000); | |
| }); // Retry every 2 sec | |
| try { | |
| transactionHash = await etherspotModulaSdk.getUserOpReceipt(userOpHash); | |
| if (transactionHash) { | |
| break; // Exit the loop once we have the transaction hash | |
| } | |
| } catch (error) { | |
| console.error('Error fetching transaction hash:', error); | |
| } | |
| } |
|
|
||
| // get transaction hash or userOp receipt... | ||
| let userOpsReceipt = null; | ||
| const timeout = Date.now() + 30 * 1000; // 30 seconds timeout | ||
|
|
||
| while (!userOpsReceipt && Date.now() < timeout) { | ||
| await new Promise<void>((resolve) => { | ||
| setTimeout(resolve, 2000); | ||
| }); // Retry every 2 sec | ||
|
|
||
| try { | ||
| userOpsReceipt = | ||
| await etherspotModulaSdk.getUserOpReceipt(userOpHash); | ||
| } catch (error) { | ||
| console.error('Error fetching transaction hash:', error); | ||
| } | ||
| } | ||
|
|
||
| if (!userOpsReceipt) { | ||
| console.warn( | ||
| 'Failed to get the transaction hash within 30 seconds.' | ||
| ); | ||
| } else { | ||
| sentBatches.push({ | ||
| ...estimatedBatch, | ||
| userOpHash, | ||
| transactionHash: userOpsReceipt, | ||
| }); | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Implement early exit from retry loop on success.
The current implementation continues to retry fetching the transaction receipt even after successfully retrieving it, until the timeout is reached. This is inefficient and may cause unnecessary API calls.
while (!userOpsReceipt && Date.now() < timeout) {
await new Promise<void>((resolve) => {
setTimeout(resolve, 2000);
}); // Retry every 2 sec
try {
userOpsReceipt =
await etherspotModulaSdk.getUserOpReceipt(userOpHash);
+ if (userOpsReceipt) {
+ break; // Exit the loop once we have the receipt
+ }
} catch (error) {
console.error('Error fetching transaction hash:', error);
}
}📝 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.
| // get transaction hash or userOp receipt... | |
| let userOpsReceipt = null; | |
| const timeout = Date.now() + 30 * 1000; // 30 seconds timeout | |
| while (!userOpsReceipt && Date.now() < timeout) { | |
| await new Promise<void>((resolve) => { | |
| setTimeout(resolve, 2000); | |
| }); // Retry every 2 sec | |
| try { | |
| userOpsReceipt = | |
| await etherspotModulaSdk.getUserOpReceipt(userOpHash); | |
| } catch (error) { | |
| console.error('Error fetching transaction hash:', error); | |
| } | |
| } | |
| if (!userOpsReceipt) { | |
| console.warn( | |
| 'Failed to get the transaction hash within 30 seconds.' | |
| ); | |
| } else { | |
| sentBatches.push({ | |
| ...estimatedBatch, | |
| userOpHash, | |
| transactionHash: userOpsReceipt, | |
| }); | |
| } | |
| // get transaction hash or userOp receipt... | |
| let userOpsReceipt = null; | |
| const timeout = Date.now() + 30 * 1000; // 30 seconds timeout | |
| while (!userOpsReceipt && Date.now() < timeout) { | |
| await new Promise<void>((resolve) => { | |
| setTimeout(resolve, 2000); | |
| }); // Retry every 2 sec | |
| try { | |
| userOpsReceipt = | |
| await etherspotModulaSdk.getUserOpReceipt(userOpHash); | |
| if (userOpsReceipt) { | |
| break; // Exit the loop once we have the receipt | |
| } | |
| } catch (error) { | |
| console.error('Error fetching transaction hash:', error); | |
| } | |
| } | |
| if (!userOpsReceipt) { | |
| console.warn( | |
| 'Failed to get the transaction hash within 30 seconds.' | |
| ); | |
| } else { | |
| sentBatches.push({ | |
| ...estimatedBatch, | |
| userOpHash, | |
| transactionHash: userOpsReceipt, | |
| }); | |
| } |
There was a problem hiding this comment.
@IAmKio Thank you for confirming! This optimization will prevent unnecessary API calls by exiting the loop as soon as we successfully retrieve the receipt. It's a small change that will make the code more efficient.
⟠ ⟠ ⟠
( ͡° ͜ ͡°)
\ (•◡•) /
If you found this review helpful, would you consider giving us a shout-out on X?
Thank you for using CodeRabbit!
IAmKio
left a comment
IAmKio
left a comment
There was a problem hiding this comment.
Looks good, just some minor suggestions 👍
|
|
||
| // get transaction hash or userOp receipt... | ||
| let userOpsReceipt = null; | ||
| const timeout = Date.now() + 30 * 1000; // 30 seconds timeout | ||
|
|
||
| while (!userOpsReceipt && Date.now() < timeout) { | ||
| await new Promise<void>((resolve) => { | ||
| setTimeout(resolve, 2000); | ||
| }); // Retry every 2 sec | ||
|
|
||
| try { | ||
| userOpsReceipt = | ||
| await etherspotModulaSdk.getUserOpReceipt(userOpHash); | ||
| } catch (error) { | ||
| console.error('Error fetching transaction hash:', error); | ||
| } | ||
| } | ||
|
|
||
| if (!userOpsReceipt) { | ||
| console.warn( | ||
| 'Failed to get the transaction hash within 30 seconds.' | ||
| ); | ||
| } else { | ||
| sentBatches.push({ | ||
| ...estimatedBatch, | ||
| userOpHash, | ||
| transactionHash: userOpsReceipt, | ||
| }); | ||
| } |
| console.error('Error fetching transaction hash:', error); | ||
| } | ||
| } | ||
|
|
||
| if (!userOpsReceipt) { | ||
| console.warn( | ||
| 'Failed to get the transaction hash within 30 seconds.' | ||
| ); |
There was a problem hiding this comment.
Remember to suggest to the developer what they should try to do next
| try { | ||
| transactionHash = await etherspotModulaSdk.getUserOpReceipt(userOpHash); | ||
| } catch (error) { | ||
| console.error('Error fetching transaction hash:', error); |
| } | ||
|
|
||
| if (!transactionHash) { | ||
| console.warn('Failed to get the transaction hash within 30 seconds.'); |
| const getTransactionHash = async ( | ||
| userOpHash: string, | ||
| batchId: number | ||
| ): Promise<string | null> => { |
There was a problem hiding this comment.
Suggestion: allow the timeout and retryInternal to be configurable in getTransactionHash as optional parameters just incase a developer wants to override this
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
CHANGELOG.md (1)
5-10: Clear and Informative Bullet Points with a Stylistic Recommendation
The bullet points under "### Added Changes" accurately communicate the new functionalities: addinggetTransactionHash, updating thesend()method with a newtransactionHashparameter, and noting the version update ofetherspot-modular. For improved clarity and to address stylistic feedback from static analysis, consider rephrasing at least one of the bullet items to avoid starting successive items with the same word "Added." For example, you might replace one with "Introduced" or "Implemented" to enhance readability.🧰 Tools
🪛 LanguageTool
[style] ~8-~8: Three successive sentences begin with the same word. Consider rewording the sentence or use a thesaurus to find a synonym.
Context: ...transaction hash with an userOp hash. - AddedtransactionHashtosend()in `Ether...(ENGLISH_WORD_REPEAT_BEGINNING_RULE)
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
package-lock.jsonis excluded by!**/package-lock.json
📒 Files selected for processing (7)
CHANGELOG.md(1 hunks)package.json(2 hunks)src/components/EtherspotTransactionKit.tsx(1 hunks)src/contexts/EtherspotContext.tsx(1 hunks)src/hooks/useWalletAddress.ts(2 hunks)src/providers/EtherspotContextProvider.tsx(1 hunks)src/providers/EtherspotTransactionKitContextProvider.tsx(2 hunks)
✅ Files skipped from review due to trivial changes (3)
- src/contexts/EtherspotContext.tsx
- src/providers/EtherspotContextProvider.tsx
- src/components/EtherspotTransactionKit.tsx
🚧 Files skipped from review as they are similar to previous changes (3)
- src/providers/EtherspotTransactionKitContextProvider.tsx
- package.json
- src/hooks/useWalletAddress.ts
🧰 Additional context used
🪛 LanguageTool
CHANGELOG.md
[style] ~8-~8: Three successive sentences begin with the same word. Consider rewording the sentence or use a thesaurus to find a synonym.
Context: ...transaction hash with an userOp hash. - Added transactionHash to send() in `Ether...
(ENGLISH_WORD_REPEAT_BEGINNING_RULE)
🔇 Additional comments (1)
CHANGELOG.md (1)
3-4: New Version Entry Added
The new version entry for [1.0.3] - 2025-02-19 is clearly delineated and follows the established changelog format. This entry correctly reflects the version update in line with the PR objectives.
2 participants
Description
How Has This Been Tested?
Screenshots (if appropriate):
Types of changes
Summary by CodeRabbit
New Features
Chores
@etherspot/modular-sdkfrom "^5.0.0" to "^5.1.0".