feat/update-modular-sdk-tx-kit-2 - #188
Conversation
WalkthroughThis update introduces new methods for accessing the Etherspot provider and retrieving transaction hashes in the transaction kit. The provider access pattern is clarified and expanded, and a polling-based Changes
Sequence Diagram(s)sequenceDiagram
participant User
participant EtherspotTransactionKit
participant EtherspotProvider
participant ModularSDK
User->>EtherspotTransactionKit: getTransactionHash(userOpHash, chainId, ...)
EtherspotTransactionKit->>EtherspotProvider: getSdk(chainId)
EtherspotProvider->>ModularSDK: getUserOpReceipt(userOpHash)
ModularSDK-->>EtherspotProvider: {receipt with transactionHash or null}
EtherspotProvider-->>EtherspotTransactionKit: receipt
alt transactionHash found
EtherspotTransactionKit-->>User: transactionHash
else not found & timeout not reached
EtherspotTransactionKit->>ModularSDK: (retry after interval)
else timeout reached
EtherspotTransactionKit-->>User: null
end
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~15–20 minutes Possibly related PRs
Suggested reviewers
Poem
Note ⚡️ Unit Test Generation is now available in beta!Learn more here, or try it out under "Finishing Touches" below. 📜 Recent review detailsConfiguration used: CodeRabbit UI 📒 Files selected for processing (1)
🚧 Files skipped from review as they are similar to previous changes (1)
✨ Finishing Touches🧪 Generate unit tests
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. 🪧 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 (2)
CHANGELOG.md (1)
3-10: LGTM! Comprehensive and accurate changelog entry.The changelog correctly documents all changes in this patch release, including the two new methods and dependency update. The descriptions clearly explain the purpose of each addition.
Consider varying the sentence beginnings to improve readability:
### Added Changes - Added `getEtherspotProvider` to access directly the Etherspot Provider. - Introduced `getTransactionHash` to get a transaction hash with a userOp Hash and chain id. - Updated the `modular-sdk` version to 6.1.1.lib/TransactionKit.ts (1)
1798-1837: Consider adjusting the polling loop timingThe
getTransactionHashmethod implementation is well-structured with proper error handling and timeout logic. However, there's a potential timing issue:while (!transactionHash && Date.now() < timeoutTotal) { await new Promise<void>((resolve) => setTimeout(resolve, retryInterval)); // This delays before first attempt try { transactionHash = await etherspotModulaSdk.getUserOpReceipt(userOpHash); } catch (error) { // ... } }The delay occurs before every attempt, including the first one. Consider moving the delay to the end of the loop or adding a condition to skip it on the first iteration for more responsive behavior:
+let isFirstAttempt = true; while (!transactionHash && Date.now() < timeoutTotal) { + if (!isFirstAttempt) { await new Promise<void>((resolve) => setTimeout(resolve, retryInterval)); + } + isFirstAttempt = false; try { transactionHash = await etherspotModulaSdk.getUserOpReceipt(userOpHash); } catch (error) { // ... } }
📜 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)__tests__/EtherspotTransactionKit.test.ts(4 hunks)example/src/App.tsx(11 hunks)lib/EtherspotUtils.ts(1 hunks)lib/TransactionKit.ts(7 hunks)lib/interfaces/index.ts(1 hunks)package.json(2 hunks)
🧰 Additional context used
🧬 Code Graph Analysis (2)
__tests__/EtherspotTransactionKit.test.ts (1)
__mocks__/@etherspot/modular-sdk.js (1)
ModularSdk(10-178)
lib/TransactionKit.ts (1)
lib/EtherspotProvider.ts (1)
EtherspotProvider(15-154)
🪛 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: ...cess directly the Etherspot Provider. - Added getTransactionHash to get a transacti...
(ENGLISH_WORD_REPEAT_BEGINNING_RULE)
🔇 Additional comments (19)
example/src/App.tsx (2)
12-14: LGTM! Well-implemented bigint serialization handler.The
bigIntReplacerfunction correctly handlesbigintserialization by converting to strings, which is essential sinceJSON.stringify()cannot serializebigintvalues by default.
140-142: LGTM! Comprehensive application of bigint replacer.The
bigIntReplacerfunction is consistently applied to allJSON.stringifycalls throughout the file, ensuring robust handling ofbigintvalues in transaction estimates, sends, error messages, and state logging.Also applies to: 153-155, 172-172, 177-179, 191-191, 488-490, 501-503, 678-679, 694-695, 716-716, 760-760
package.json (2)
28-28: LGTM! Appropriate dependency version update.The update from
"6.1.0"to"^6.1.1"correctly enables automatic minor and patch updates while supporting the new methods introduced in this PR.
68-68: LGTM! Good cleanup of empty peerDependencies.Removing the empty
peerDependenciesobject is a good housekeeping improvement.lib/EtherspotUtils.ts (1)
85-85: LGTM! Improved static method call clarity.Changing from
this.addressesEqual(...)toEtherspotUtils.addressesEqual(...)makes the static method call explicit and improves code readability. This is a good practice for static method invocations.lib/interfaces/index.ts (2)
40-41: LGTM! Well-designed provider access pattern.The change from
EtherspotProvidertoWalletProviderLikeforgetProvider()provides better abstraction, while the newgetEtherspotProvider()method offers direct access when needed. This separation of concerns is well-designed.
44-49: LGTM! Well-structured transaction hash polling interface.The
getTransactionHash()method signature is well-designed with:
- Required parameters for core functionality (
userOpHash,txChainId)- Optional parameters for customizing polling behavior (
timeout,retryInterval)- Appropriate return type (
Promise<string | null>) that handles both success and failure cases__tests__/EtherspotTransactionKit.test.ts (6)
107-107: LGTM: Correct API method updateThe change from
getProvider()togetEtherspotProvider()correctly aligns with the API refactoring wheregetEtherspotProvider()now returns the underlyingEtherspotProviderinstance.
799-799: LGTM: Test correctly updated for new APIThe test correctly calls
getEtherspotProvider()which should return the underlyingEtherspotProviderinstance, matching the updated API design.
966-968: LGTM: Correct method chaining for API updateThe change to
getEtherspotProvider().getSdkis correct sincegetProvider()now returns aWalletProviderLikewithout thegetSdkmethod, whilegetEtherspotProvider()provides access to the underlyingEtherspotProviderwith SDK functionality.
1101-1101: LGTM: Correct error condition testingThe test correctly mocks
getProvider()to return null to validate the error handling inestimateBatches(), which matches the implementation's provider validation logic.
1109-1109: LGTM: Correct error condition testingThe test correctly mocks
getProvider()to return null to validate the error handling insendBatches(), which matches the implementation's provider validation logic.
1116-1192: LGTM: Comprehensive test coverage for new getTransactionHash methodThe new test suite provides excellent coverage of the
getTransactionHashpolling functionality:
- Immediate success case - Tests when transaction hash is available on first call
- Polling behavior - Validates retry logic with eventual success
- Timeout handling - Ensures null is returned when timeout is exceeded
- Error resilience - Confirms polling continues despite SDK errors
The test implementation correctly mocks the SDK's
getUserOpReceiptmethod and uses realistic timeout/retry parameters. The test logic aligns well with the actual implementation.lib/TransactionKit.ts (6)
1-1: LGTM: Required import for API refactoringThe addition of
WalletProviderLikeimport is necessary to support the updatedgetProvider()method return type.
592-592: LGTM: Good abstraction of provider accessThe change from
this.etherspotProvider.getProvider()tothis.getProvider()provides better encapsulation and a consistent interface for provider access.
820-820: LGTM: Consistent provider access patternUsing
this.getProvider()maintains consistency with the abstracted provider access pattern established throughout the class.
1089-1089: LGTM: Consistent provider access abstractionThe abstracted provider access through
this.getProvider()maintains consistency and improves maintainability.
1360-1360: LGTM: Consistent provider access patternUsing the abstracted
this.getProvider()method maintains consistency across all provider access points in the class.
1756-1776: LGTM: Well-designed API separationThe refactored provider access methods provide clear separation of concerns:
getProvider()returns the underlyingWalletProviderLikefor general web3 interactionsgetEtherspotProvider()returns the wrapper class for advanced Etherspot-specific operationsThe documentation clearly explains the intended use cases for each method, making the API more intuitive and user-friendly.
vignesha22
left a comment
vignesha22
left a comment
There was a problem hiding this comment.
just one minor change rest all good. Make sure to change that before merging
3 participants
Description
How Has This Been Tested?
Screenshots (if appropriate):
Types of changes
Summary by CodeRabbit
New Features
Bug Fixes
Refactor
Tests
Chores