feat/transaction-2#184
Conversation
|
Yooo! You forgot to bump the version in package.json! |
WalkthroughThis change represents a major architectural overhaul, removing all React-specific components, hooks, and contexts, and replacing them with a framework-agnostic, class-based API focused solely on transaction management. The new implementation introduces core classes and interfaces for transaction and batch handling, utility functions, and updates all supporting infrastructure, while deleting all asset, module, history, price, swap, and NFT management features. Changes
Sequence Diagram(s)sequenceDiagram
participant User
participant TransactionKit
participant EtherspotProvider
participant ModularSdk
User->>TransactionKit: new TransactionKit(config)
User->>TransactionKit: .transaction(params)
TransactionKit->>EtherspotProvider: getSdk(chainId)
EtherspotProvider->>ModularSdk: new ModularSdk(provider, chainId, ...)
TransactionKit->>TransactionKit: Store working transaction
User->>TransactionKit: .name({ transactionName })
TransactionKit->>TransactionKit: Save named transaction
User->>TransactionKit: .batch({ batchName })
TransactionKit->>TransactionKit: Create/select batch
User->>TransactionKit: .addToBatch({ transactionName, batchName })
TransactionKit->>TransactionKit: Add named transaction to batch
User->>TransactionKit: .estimate() / .send()
TransactionKit->>EtherspotProvider: getSdk(chainId)
TransactionKit->>ModularSdk: estimate/send transaction(s)
ModularSdk-->>TransactionKit: result/error
TransactionKit-->>User: estimation/send result
Possibly related PRs
Poem
Warning There were issues while running some tools. Please review the errors and either fix the tool's configuration or disable the tool if it's a critical failure. 🔧 ESLint
npm error Exit handler never called! ✨ Finishing Touches
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 (
|
|
Yooo! You forgot to bump the version in package.json! |
|
Yooo! You forgot to bump the version in package.json! |
|
Yooo! You forgot to bump the version in package.json! |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 16
🔭 Outside diff range comments (2)
package.json (2)
30-37: Remove React-specific dependencies from main dependencies.These React-related ESLint configurations should be moved to devDependencies since the library is now framework-agnostic and shouldn't impose React tooling on consumers.
"dependencies": { "@etherspot/eip1271-verification-util": "0.1.2", "@etherspot/modular-sdk": "^6.1.0", "buffer": "^6.0.3", - "eslint-config-airbnb": "^19.0.4", - "eslint-config-prettier": "^9.1.0", - "eslint-config-react": "^1.1.7", - "eslint-plugin-jsx-a11y": "^6.9.0", - "eslint-plugin-prettier": "^5.2.1", "lodash": "^4.17.21", - "prettier": "^3.3.3", "viem": "2.21.53" },Move these to devDependencies where they belong for development tooling.
84-86: Remove React peerDependency for framework-agnostic library.The library should not have React as a peerDependency if it's truly framework-agnostic.
- "peerDependencies": { - "react": ">=16.13.0" - }
♻️ Duplicate comments (1)
example/src/App.tsx (1)
29-696: Consider extracting shared test scenarios.This component shares nearly identical test scenarios with
TransactionKitTestUI.tsx. To improve maintainability and reduce duplication, consider extracting these scenarios into a shared module.
🧹 Nitpick comments (11)
src/TransactionKit/EtherspotUtils.ts (1)
14-61: Consider using a namespace or individual functions instead of a static-only class.The static analysis tool correctly identifies that this class contains only static members. Consider using a namespace or individual exported functions for better tree-shaking and cleaner API.
-export class EtherspotUtils { - static checksumAddress(address: string): string { +export namespace EtherspotUtils { + export function checksumAddress(address: string): string { // ... rest of implementation } - static async verifyEip1271Message( + export async function verifyEip1271Message( // ... rest of implementation } // ... other methods -} +}Alternatively, export individual functions:
export function checksumAddress(address: string): string { /* ... */ } export async function verifyEip1271Message(/* ... */) { /* ... */ } // etc.src/TransactionKit/EtherspotProvider.ts (2)
79-82: Consider using exponential backoff for retry delays.The current implementation uses a fixed 1-second delay between retries. For better resilience against temporary network issues, consider implementing exponential backoff.
-await new Promise((resolve) => { - setTimeout(resolve, 1000); -}); // Wait 1 sec before retrying +await new Promise((resolve) => { + setTimeout(resolve, Math.min(1000 * Math.pow(2, i - 1), 5000)); +}); // Exponential backoff: 1s, 2s, 4s (capped at 5s)
123-126: MethodclearAllCachescurrently only clears SDK cache.The method name suggests it would clear multiple types of caches, but it currently only calls
clearSdkCache(). If this is intended for future expansion, consider adding a comment to clarify. Otherwise, consider removing this method to avoid confusion./** * Clear all caches + * Note: Currently only clears SDK cache. Reserved for future cache types. */ clearAllCaches(): this { this.clearSdkCache(); return this; }example/src/TransactionKitTestUI.tsx (2)
169-169: Use optional chaining for cleaner code.The static analysis correctly identifies opportunities to use optional chaining.
-if (result && result.batches) { +if (result?.batches) { const removedBatches = Object.entries(result.batches)Also applies to: 185-195
667-670: Use optional chaining for nested property access.-if ( - result && - result.batches && - result.batches['doesnotexistbatch'] && - result.batches['doesnotexistbatch'].isSuccess -) { +if (result?.batches?.['doesnotexistbatch']?.isSuccess) {Also applies to: 683-686
example/src/App.tsx (1)
185-185: Use optional chaining for cleaner code.Similar to TransactionKitTestUI.tsx, these sections would benefit from optional chaining.
For line 185:
-if (result && result.batches) { +if (result?.batches) {For lines 683-686:
-if ( - result && - result.batches && - result.batches['doesnotexistbatch'] && - result.batches['doesnotexistbatch'].isSuccess -) { +if (result?.batches?.['doesnotexistbatch']?.isSuccess) {Also applies to: 683-686
__tests__/EtherspotProvider.test.ts (1)
490-490: Misleading comment about chainId type.The comment mentions "chainId as string" but the test doesn't actually test string-to-number conversion. The chainId is already a number in the config.
-it('should handle chainId as string (converted to number)', async () => { +it('should handle numeric chainId correctly', async () => {src/TransactionKit/TransactionKit.ts (3)
408-418: Improve error handling for value parsing.The catch block catches all errors without discrimination. Consider checking for specific error types.
let parsedValue: bigint; try { parsedValue = typeof value === 'bigint' ? value : BigInt(value); if (parsedValue < BigInt(0)) { - throw new Error(); + throw new Error('Negative value'); } - } catch { + } catch (error) { + // Check if it's a BigInt conversion error or our negative value error this.throwError( 'transaction(): value must be a non-negative bigint or numeric string.' ); }
1467-1467: Remove commented code.Commented code should be removed. Version control preserves history if needed.
- // this.groupedBatchesPerId = {};
251-1483: Consider splitting responsibilities for better maintainability.The
EtherspotTransactionKitclass handles many responsibilities:
- Transaction creation and validation
- Batch management
- Gas estimation
- Transaction sending
- State management
- Wallet address caching
- Debug logging
Consider applying the Single Responsibility Principle by extracting some concerns into separate classes.
Suggested architecture:
TransactionBuilder- Handle transaction creation and validationBatchManager- Manage batch operationsTransactionEstimator- Handle gas estimationTransactionSender- Handle sending transactionsStateManager- Manage internal stateThis would make the code more modular, testable, and maintainable.
__tests__/EtherspotTransactionKit.test.ts (1)
1083-1084: Avoid direct state manipulation in tests.Directly modifying internal state (
transactionKit.getState().batches['batch1'] = []) makes tests fragile and coupled to implementation details.Consider using the public API to create the test scenario, or add a test helper method that properly sets up an empty batch state if this is a valid edge case that needs testing.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (2)
example/package-lock.jsonis excluded by!**/package-lock.jsonpackage-lock.jsonis excluded by!**/package-lock.json
📒 Files selected for processing (59)
CHANGELOG.md(1 hunks)__mocks__/@etherspot/eip1271-verification-util.js(0 hunks)__tests__/EtherspotProvider.test.ts(1 hunks)__tests__/EtherspotTransactionKit.test.ts(1 hunks)__tests__/components/EtherspotApprovalTransaction.test.js(0 hunks)__tests__/components/EtherspotContractTransaction.test.js(0 hunks)__tests__/components/EtherspotTokenTransferTransaction.test.js(0 hunks)__tests__/hooks/useEtherspotAssets.test.js(0 hunks)__tests__/hooks/useEtherspotBalances.test.js(0 hunks)__tests__/hooks/useEtherspotHistory.test.js(0 hunks)__tests__/hooks/useEtherspotModules.test.js(0 hunks)__tests__/hooks/useEtherspotNfts.test.js(0 hunks)__tests__/hooks/useEtherspotPrices.test.js(0 hunks)__tests__/hooks/useEtherspotSwaps.test.js(0 hunks)__tests__/hooks/useEtherspotTransactions.test.js(0 hunks)__tests__/hooks/useEtherspotUtils.test.js(0 hunks)__tests__/hooks/useProviderWalletTransaction.test.js(0 hunks)__tests__/hooks/useWalletAddress.test.js(0 hunks)example/src/App.tsx(1 hunks)example/src/TransactionKitTestUI.tsx(1 hunks)example/src/index.tsx(1 hunks)package.json(1 hunks)rollup.config.js(0 hunks)src/TransactionKit/EtherspotProvider.ts(1 hunks)src/TransactionKit/EtherspotUtils.ts(1 hunks)src/TransactionKit/TransactionKit.ts(1 hunks)src/components/EtherspotApprovalTransaction.tsx(0 hunks)src/components/EtherspotBatch.tsx(0 hunks)src/components/EtherspotBatches.tsx(0 hunks)src/components/EtherspotContractTransaction.tsx(0 hunks)src/components/EtherspotTokenTransferTransaction.tsx(0 hunks)src/components/EtherspotTransaction.tsx(0 hunks)src/components/EtherspotTransactionKit.tsx(0 hunks)src/components/ProviderWalletTransaction.tsx(0 hunks)src/contexts/EtherspotBatchContext.tsx(0 hunks)src/contexts/EtherspotBatchesContext.tsx(0 hunks)src/contexts/EtherspotContext.tsx(0 hunks)src/contexts/EtherspotTransactionKitContext.tsx(0 hunks)src/contexts/ProviderWalletContext.tsx(0 hunks)src/hooks/useDeepCompare.ts(0 hunks)src/hooks/useEtherspot.tsx(0 hunks)src/hooks/useEtherspotAssets.ts(0 hunks)src/hooks/useEtherspotBalances.ts(0 hunks)src/hooks/useEtherspotHistory.ts(0 hunks)src/hooks/useEtherspotModules.ts(0 hunks)src/hooks/useEtherspotNfts.ts(0 hunks)src/hooks/useEtherspotPrices.ts(0 hunks)src/hooks/useEtherspotSwaps.ts(0 hunks)src/hooks/useEtherspotTransactions.ts(0 hunks)src/hooks/useEtherspotUtils.ts(0 hunks)src/hooks/useId.ts(0 hunks)src/hooks/useProviderWalletTransaction.ts(0 hunks)src/hooks/useWalletAddress.ts(0 hunks)src/index.ts(1 hunks)src/providers/EtherspotContextProvider.tsx(0 hunks)src/providers/EtherspotTransactionKitContextProvider.tsx(0 hunks)src/providers/ProviderWalletContextProvider.tsx(0 hunks)src/types/EtherspotTransactionKit.ts(0 hunks)src/types/TransactionKitTypes.ts(1 hunks)
💤 Files with no reviewable changes (47)
- rollup.config.js
- src/hooks/useDeepCompare.ts
- src/components/EtherspotApprovalTransaction.tsx
- src/hooks/useId.ts
- src/hooks/useEtherspot.tsx
- src/components/EtherspotTransaction.tsx
- src/contexts/ProviderWalletContext.tsx
- src/components/ProviderWalletTransaction.tsx
- tests/components/EtherspotTokenTransferTransaction.test.js
- src/components/EtherspotBatch.tsx
- src/components/EtherspotContractTransaction.tsx
- src/contexts/EtherspotBatchContext.tsx
- src/hooks/useProviderWalletTransaction.ts
- src/contexts/EtherspotBatchesContext.tsx
- src/hooks/useEtherspotPrices.ts
- mocks/@etherspot/eip1271-verification-util.js
- src/contexts/EtherspotContext.tsx
- tests/hooks/useEtherspotNfts.test.js
- tests/components/EtherspotContractTransaction.test.js
- src/contexts/EtherspotTransactionKitContext.tsx
- src/hooks/useEtherspotBalances.ts
- tests/hooks/useProviderWalletTransaction.test.js
- src/providers/ProviderWalletContextProvider.tsx
- tests/hooks/useEtherspotBalances.test.js
- tests/hooks/useEtherspotUtils.test.js
- tests/components/EtherspotApprovalTransaction.test.js
- src/components/EtherspotBatches.tsx
- tests/hooks/useEtherspotSwaps.test.js
- src/hooks/useWalletAddress.ts
- src/hooks/useEtherspotNfts.ts
- tests/hooks/useWalletAddress.test.js
- src/providers/EtherspotContextProvider.tsx
- src/components/EtherspotTransactionKit.tsx
- src/components/EtherspotTokenTransferTransaction.tsx
- tests/hooks/useEtherspotAssets.test.js
- tests/hooks/useEtherspotPrices.test.js
- tests/hooks/useEtherspotTransactions.test.js
- src/hooks/useEtherspotAssets.ts
- src/hooks/useEtherspotTransactions.ts
- tests/hooks/useEtherspotModules.test.js
- src/hooks/useEtherspotHistory.ts
- src/hooks/useEtherspotUtils.ts
- src/types/EtherspotTransactionKit.ts
- tests/hooks/useEtherspotHistory.test.js
- src/hooks/useEtherspotSwaps.ts
- src/providers/EtherspotTransactionKitContextProvider.tsx
- src/hooks/useEtherspotModules.ts
🧰 Additional context used
🧬 Code Graph Analysis (3)
src/TransactionKit/EtherspotProvider.ts (1)
__mocks__/@etherspot/modular-sdk.js (1)
ModularSdk(10-178)
example/src/TransactionKitTestUI.tsx (1)
src/TransactionKit/TransactionKit.ts (4)
TransactionKit(1486-1490)NamedTransactionState(48-68)batch(478-504)BatchState(91-97)
example/src/App.tsx (1)
src/TransactionKit/TransactionKit.ts (4)
TransactionKit(1486-1490)NamedTransactionState(48-68)batch(478-504)BatchState(91-97)
🪛 Biome (1.9.4)
src/TransactionKit/EtherspotUtils.ts
[error] 14-61: Avoid classes that contain only static members.
Prefer using simple functions instead of classes with only static members.
(lint/complexity/noStaticOnlyClass)
[error] 54-54: Using this in a static context can be confusing.
this refers to the class.
Unsafe fix: Use the class name instead.
(lint/complexity/noThisInStatic)
example/src/TransactionKitTestUI.tsx
[error] 169-169: Change to an optional chain.
Unsafe fix: Change to an optional chain.
(lint/complexity/useOptionalChain)
[error] 667-670: Change to an optional chain.
Unsafe fix: Change to an optional chain.
(lint/complexity/useOptionalChain)
example/src/App.tsx
[error] 185-185: Change to an optional chain.
Unsafe fix: Change to an optional chain.
(lint/complexity/useOptionalChain)
[error] 683-686: Change to an optional chain.
Unsafe fix: Change to an optional chain.
(lint/complexity/useOptionalChain)
🔇 Additional comments (13)
example/src/index.tsx (1)
13-13: LGTM! Clean removal of React provider wrapper.The direct rendering of
<App />aligns perfectly with the new framework-agnostic, class-based API approach, removing dependency on React contexts.src/index.ts (1)
6-9: Excellent refactoring to class-based API exports.The new export structure is clean and well-organized, properly replacing the previous React component and hook exports with the new framework-agnostic class-based modules.
CHANGELOG.md (1)
3-41: Excellent changelog documentation for major version release.The changelog clearly documents all breaking changes, architectural shifts, and migration impacts. The structured format with clear sections (Architecture Changes, Scope Reduction, API Changes, Migration Impact) makes it easy for users to understand the significant changes in v2.0.0.
example/src/TransactionKitTestUI.tsx (1)
403-405: Avoid empty catch blocks that silently swallow errors.The empty catch block could hide important errors when trying to remove a non-existent batch.
// Remove batchC if exists try { (kit.batch({ batchName: 'batchC' }) as BatchState).remove(); -} catch {} +} catch { + // Ignore error if batch doesn't exist +}Likely an incorrect or invalid review comment.
__tests__/EtherspotProvider.test.ts (1)
1-523: Excellent test coverage!This test suite provides comprehensive coverage of the EtherspotProvider class, including:
- Configuration management
- SDK caching and creation
- Retry logic with proper timing tests
- Concurrency handling
- Error scenarios
- Provider change detection
The mocking strategy is well-implemented and the test structure is clear and maintainable.
src/types/TransactionKitTypes.ts (1)
1-98: Well-structured type definitions for the new transaction kit architecture.The type definitions are comprehensive and align well with the new class-based API:
- Clear interfaces for transactions and batches
- Proper use of TypeScript features (extends, generics, enums)
- Good separation of concerns between different types
- The
UserOpinterface properly handles both synchronous and asynchronous valuessrc/TransactionKit/TransactionKit.ts (3)
1077-1085: Consider handling partial failures in batch operations.Using
Promise.allmeans if one transaction fails to be added to the batch, the entire operation fails. Consider whether partial success should be supported.If partial failures should be handled, consider using
Promise.allSettled:- await Promise.all( - batchTransactions.map(async (tx) => { - await etherspotModulaSdk.addUserOpsToBatch({ - to: tx.to || '', - value: tx.value?.toString(), - data: tx.data, - }); - }) - ); + const results = await Promise.allSettled( + batchTransactions.map(async (tx) => { + await etherspotModulaSdk.addUserOpsToBatch({ + to: tx.to || '', + value: tx.value?.toString(), + data: tx.data, + }); + }) + ); + + // Check for any failures + const failures = results.filter(r => r.status === 'rejected'); + if (failures.length > 0) { + this.log(`Failed to add ${failures.length} transactions to batch`); + // Handle partial failure... + }Also applies to: 1237-1245
963-977: Document the transaction removal behavior.After successful send, transactions are automatically removed from state. This behavior might be surprising to users.
Consider:
- Documenting this behavior clearly in the method documentation
- Providing a way to preserve transaction history if needed
- Adding a configuration option to control this behavior
/** * Send the transaction. * NOTE: Successfully sent transactions are automatically removed from state. * @param props - Send configuration */ async send(props?: SendSingleTransactionProps): Promise<...>
334-356: Avoid accessing private SDK propertiesAccessing the private
etherspotWallet.accountAddressvia@ts-expect-errorties you to SDK internals and may break on SDK updates. We didn’t find any public getter in the SDK sources—please verify in the SDK documentation or TypeScript definitions whether there’s an exposed method (for example,getCounterFactualAddress()or a dedicatedgetWalletAddress()getter) for retrieving the current wallet address. If none exists, consider:
- Requesting the SDK team to expose a public API for retrieving the wallet address.
- Relying only on officially supported methods (e.g.
getCounterFactualAddress()).- Adding a version compatibility check around this access to catch breaking changes early.
File: src/TransactionKit/TransactionKit.ts (lines 334–356)
__tests__/EtherspotTransactionKit.test.ts (4)
116-175: Well-structured tests with comprehensive coverage!The
getWalletAddresstests effectively cover various scenarios including caching behavior, error handling, and chain-specific requests. The cache verification test is particularly valuable.
177-263: LGTM!
559-723: LGTM!
879-891: Excellent approach to test async state management!Using mock implementations to capture state during async operations is a clever way to ensure state flags are properly managed throughout the operation lifecycle.
IAmKio
left a comment
IAmKio
left a comment
There was a problem hiding this comment.
This is looking fantastic - TransactionKit is finally a universal JS library and ready for a much bigger audience. It's very well coded, we've out the drama and un-necessary features and really focused on sending transactions.
I've made a few comments, abservation sand suggested a few minor changes but overall it's fantastic work! We'll catch up soon 🙏
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (1)
example/src/App.tsx (1)
16-20: Add null check forwindow.ethereum.The code assumes
window.ethereumexists, which may not be true in all environments.
🧹 Nitpick comments (2)
example/src/App.tsx (2)
183-194: Use optional chaining for cleaner code.Simplify the nested property checks using optional chaining.
- if (result && result.batches) { - const removedBatches = Object.entries(result.batches) + if (result?.batches) { + const removedBatches = Object.entries(result.batches)
681-688: Use optional chaining for nested property access.Simplify the deeply nested property checks.
- if ( - result && - result.batches && - result.batches['doesnotexistbatch'] && - result.batches['doesnotexistbatch'].isSuccess - ) { + if (result?.batches?.['doesnotexistbatch']?.isSuccess) {
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
example/package-lock.jsonis excluded by!**/package-lock.json
📒 Files selected for processing (2)
example/package.json(1 hunks)example/src/App.tsx(1 hunks)
🧰 Additional context used
🧬 Code Graph Analysis (1)
example/src/App.tsx (1)
src/TransactionKit/TransactionKit.ts (4)
TransactionKit(1486-1490)NamedTransactionState(48-68)batch(478-504)BatchState(91-97)
🪛 Biome (1.9.4)
example/src/App.tsx
[error] 184-184: Change to an optional chain.
Unsafe fix: Change to an optional chain.
(lint/complexity/useOptionalChain)
[error] 682-685: Change to an optional chain.
Unsafe fix: Change to an optional chain.
(lint/complexity/useOptionalChain)
🔇 Additional comments (2)
example/package.json (1)
13-17: Dependency pinning looks good for the example app.Using exact versions for React dependencies in the example application ensures consistent behavior across different environments.
example/src/App.tsx (1)
32-695: Excellent comprehensive test coverage!The test scenarios provide thorough coverage of the TransactionKit API, including edge cases, error conditions, and complex workflows. This will be valuable for developers learning the new API.
There was a problem hiding this comment.
Actionable comments posted: 2
♻️ Duplicate comments (6)
src/TransactionKit/EtherspotProvider.ts (1)
1-1: Add comprehensive JSDoc documentation to this file.Following the previous review feedback, this core file needs comprehensive JSDoc/TSDoc documentation to provide excellent developer experience, IDE autocompletion, and AI tool compatibility.
example/src/App.tsx (2)
12-14: Add validation and security warning for private key handling.The private key handling still lacks proper validation and security warnings as noted in previous reviews.
16-20: Add null check for window.ethereum.The code still assumes
window.ethereumexists without proper validation as flagged in previous reviews.src/TransactionKit/TransactionKit.ts (3)
1-1: Add comprehensive JSDoc documentation to this core file.As noted in previous reviews, this core file especially needs comprehensive JSDoc/TSDoc documentation to provide excellent developer experience and IDE support.
279-297: Improve type safety and logging as suggested in previous reviews.The use of
anytype and console.log approach should be addressed as noted in previous reviews.
697-726: Extract duplicate validation logic as suggested in previous reviews.The validation logic for checking invalid transactions (value=0 and data=0x) is duplicated between estimate() and send() methods as noted in previous reviews.
Also applies to: 876-890
🧹 Nitpick comments (1)
src/TransactionKit/EtherspotProvider.ts (1)
67-88: Consider making retry configuration customizable.The retry logic is well-implemented, but the retry count (3) and delay (1000ms) are hardcoded. Consider making these configurable through the constructor or a separate configuration object for better flexibility.
+export interface EtherspotProviderConfig { + provider: WalletProviderLike; + chainId: number; + bundlerApiKey?: string; + retryConfig?: { + maxAttempts?: number; + delayMs?: number; + }; +} // Then in the retry logic: - for (let i = 1; i <= 3; i++) { + const maxAttempts = this.config.retryConfig?.maxAttempts ?? 3; + const delayMs = this.config.retryConfig?.delayMs ?? 1000; + for (let i = 1; i <= maxAttempts; i++) { // ... existing logic ... - if (i < 3) { + if (i < maxAttempts) { await new Promise((resolve) => { - setTimeout(resolve, 1000); + setTimeout(resolve, delayMs); }); } else { throw new Error( - 'Failed to get counter factual address when initialising the Etherspot Modular SDK after 3 attempts.' + `Failed to get counter factual address when initialising the Etherspot Modular SDK after ${maxAttempts} attempts.` ); }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (7)
__tests__/EtherspotProvider.test.ts(1 hunks)__tests__/EtherspotTransactionKit.test.ts(1 hunks)example/src/App.tsx(1 hunks)package.json(3 hunks)src/TransactionKit/EtherspotProvider.ts(1 hunks)src/TransactionKit/TransactionKit.ts(1 hunks)src/utils/common.ts(1 hunks)
✅ Files skipped from review due to trivial changes (1)
- src/utils/common.ts
🚧 Files skipped from review as they are similar to previous changes (3)
- package.json
- tests/EtherspotProvider.test.ts
- tests/EtherspotTransactionKit.test.ts
🧰 Additional context used
🧬 Code Graph Analysis (2)
src/TransactionKit/EtherspotProvider.ts (1)
__mocks__/@etherspot/modular-sdk.js (1)
ModularSdk(10-178)
example/src/App.tsx (1)
src/TransactionKit/TransactionKit.ts (4)
TransactionKit(1636-1640)NamedTransactionState(45-65)batch(456-483)BatchState(88-94)
🪛 Biome (1.9.4)
example/src/App.tsx
[error] 184-184: Change to an optional chain.
Unsafe fix: Change to an optional chain.
(lint/complexity/useOptionalChain)
[error] 682-685: Change to an optional chain.
Unsafe fix: Change to an optional chain.
(lint/complexity/useOptionalChain)
🔇 Additional comments (6)
src/TransactionKit/EtherspotProvider.ts (1)
41-95: Excellent SDK instance management with robust retry logic.The implementation provides solid caching, provider change detection, and retry mechanisms for SDK initialization. The 3-attempt retry logic with 1-second delays is appropriate for handling transient network issues.
example/src/App.tsx (1)
32-696: Comprehensive test scenarios provide excellent coverage.The test scenarios cover a wide range of operations including error cases, edge cases, and state management. This provides excellent validation of the TransactionKit API and demonstrates proper usage patterns.
src/TransactionKit/TransactionKit.ts (4)
360-388: Excellent transaction validation with comprehensive error handling.The transaction method provides thorough validation for address format, chainId type checking, and value parsing with proper error messages. The validation logic is robust and user-friendly.
533-586: Well-implemented remove logic with proper state cleanup.The remove method handles both batch and transaction removal with proper state management, including cleanup of references in both namedTransactions and batches. The logging provides good visibility into what was removed.
1076-1238: Comprehensive batch processing with detailed logging.The batch estimation and sending methods provide excellent parallel processing, detailed logging at each step, and proper error handling. The retry logic and state management are well-implemented.
Also applies to: 1285-1555
1636-1640: Clean factory function design.The TransactionKit factory function provides a clean, functional interface for creating instances while internally using the class-based implementation. This design pattern offers flexibility for future enhancements.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (2)
.eslintrc.json (2)
40-43:no-consolerule is effectively neutralised—consider lowering severity or simplifying the rule
Declaring"no-console": ["error", { "allow": ["warn", "error"] }]turns every practical console call back into an allowed pattern while still reporting onconsole.log. In a library context (as opposed to an app) you typically either:
- Disallow all console usage (set rule to
"error"with no allow-list), or- Permit it but downgrade to
"warn"to avoid CI failures.Current setup gives the worst of both: noisy lint errors for harmless
console.log, but silent acceptance ofconsole.error/console.warn. Decide which behaviour you really want and simplify accordingly.
48-52:jsxis still listed inimport/extensions, but JSX support was removed
Since the project has dropped React / JSX completely, keeping an explicit rule entry for"jsx": "never"is redundant and may mis-signal to future contributors that.jsxfiles are still first-class citizens.- { "ts": "never", "tsx": "never", "jsx": "never", "js": "never" } + { "ts": "never", "tsx": "never", "js": "never" }This keeps the rule minimal and aligned with the new framework-agnostic direction.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
example/package-lock.jsonis excluded by!**/package-lock.json
📒 Files selected for processing (6)
.eslintrc.json(1 hunks)babel.config.js(0 hunks)example/package.json(1 hunks)package.json(3 hunks)rollup.config.js(0 hunks)tsconfig.json(0 hunks)
💤 Files with no reviewable changes (3)
- rollup.config.js
- tsconfig.json
- babel.config.js
🚧 Files skipped from review as they are similar to previous changes (2)
- example/package.json
- package.json
🔇 Additional comments (1)
.eslintrc.json (1)
32-36: No obsolete React ESLint plugins remain in package.json
Verified withrgthat botheslint-plugin-reactandeslint-plugin-react-hookshave been removed and are no longer listed under devDependencies. ✅
There was a problem hiding this comment.
Actionable comments posted: 4
♻️ Duplicate comments (4)
src/TransactionKit/EtherspotUtils.ts (2)
1-1: Move this file to {root}/lib/utils as requested.This aligns with Node.js/JS library standards for better organization.
84-86: Replacethiswith class name in static context.Using
thisin a static context is confusing since it refers to the class constructor.src/index.ts (1)
1-1: Move this file to {root}/lib as discussed.Based on the past discussion, placing everything under
libis the preferred approach.src/TransactionKit/EtherspotProvider.ts (1)
1-1: Move TransactionKit contents to {root}/lib as requested.To follow Node.js/JS library standards, move the contents of {root}/src/TransactionKit to {root}/lib.
🧹 Nitpick comments (3)
src/types/types.ts (3)
134-141: Consider validation for TransactionBuilderWith all properties being optional,
TransactionBuilderobjects could be created without essential data. Consider adding validation logic or required properties to ensure transaction integrity.You might want to:
- Make
toandchainIdrequired for valid transactions- Add a validation method in the implementation
- Use discriminated unions if different transaction types have different requirements
214-222: Remove unused parameter interfacesThe
NativeAmountParamsandToParamsinterfaces appear to be unused based on the PR objectives stating removal of asset-related functionality.Consider removing these unused interfaces to keep the codebase clean:
-export interface NativeAmountParams { - amount: number; - chainId?: number; -} - -export interface ToParams { - address: string; -}
270-289: Consider removing commented codeInstead of keeping commented code for future features, consider:
- Moving these types to a separate file (e.g.,
types/future-modules.ts)- Creating a feature branch for module support
- Adding these types when the feature is actually implemented
This keeps the codebase cleaner and avoids confusion about whether this code is active or not.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (6)
src/TransactionKit/EtherspotProvider.ts(1 hunks)src/TransactionKit/EtherspotUtils.ts(1 hunks)src/TransactionKit/TransactionKit.ts(1 hunks)src/index.ts(1 hunks)src/types/types.ts(1 hunks)src/utils/utils.ts(1 hunks)
✅ Files skipped from review due to trivial changes (1)
- src/utils/utils.ts
🚧 Files skipped from review as they are similar to previous changes (1)
- src/TransactionKit/TransactionKit.ts
🧰 Additional context used
🧬 Code Graph Analysis (1)
src/TransactionKit/EtherspotProvider.ts (2)
__mocks__/@etherspot/modular-sdk.js (1)
ModularSdk(10-178)src/types/types.ts (1)
EtherspotProviderConfig(11-15)
🪛 Biome (1.9.4)
src/TransactionKit/EtherspotUtils.ts
[error] 14-98: Avoid classes that contain only static members.
Prefer using simple functions instead of classes with only static members.
(lint/complexity/noStaticOnlyClass)
[error] 85-85: Using this in a static context can be confusing.
this refers to the class.
Unsafe fix: Use the class name instead.
(lint/complexity/noThisInStatic)
🔇 Additional comments (2)
src/types/types.ts (2)
10-15: Well-structured provider configurationThe
EtherspotProviderConfiginterface is clean and follows TypeScript best practices with appropriate use of optional properties.
244-268: Well-designed async-friendly UserOp typesThe
EtherspotPromiseOrValuepattern and consistent use ofbigintfor gas values demonstrate good type design for handling both synchronous and asynchronous operations.
There was a problem hiding this comment.
Actionable comments posted: 2
🔭 Outside diff range comments (1)
lib/utils/index.ts (1)
32-50: Fix the return logic inswitchWalletProviderToChain.The function always returns an error message even when the chain switch is successful. After the
tryblock completes successfully, it should return an empty object or success indicator.Apply this fix:
export const switchWalletProviderToChain = async ( chainId: number ): Promise<{ errorMessage?: string }> => { if (!window?.ethereum) { console.warn('Unsupported browser!'); return { errorMessage: 'Unsupported browser!' }; } try { await window.ethereum.request({ method: 'wallet_switchEthereumChain', params: [{ chainId: toHex(chainId) }], // chainId must be in hex }); + return {}; // Success - no error message } catch (e) { console.warn('Failed to switch chain', e); + return { errorMessage: 'Failed to switch chain!' }; } - - return { errorMessage: 'Failed to switch chain!' }; };
♻️ Duplicate comments (4)
example/src/App.tsx (4)
12-14: Add validation and a clear "demo only" warning for private key handling
16-20: Add null check forwindow.ethereum.
184-187: Use optional chaining for safer property access.
682-685: Use optional chaining for safer property access.
🧹 Nitpick comments (3)
lib/EtherspotProvider.ts (1)
67-71: Consider making the default bundler API key configurable.The hardcoded string
'__ETHERSPOT_BUNDLER_API_KEY__'should be externalized to a constant or configuration option for better maintainability.+const DEFAULT_BUNDLER_API_KEY = '__ETHERSPOT_BUNDLER_API_KEY__'; + export class EtherspotProvider { // ... existing code ... async getSdk( sdkChainId: number = this.config.chainId, forceNewInstance: boolean = false, customChain?: Chain ): Promise<ModularSdk> { // ... existing code ... this.sdkPerChain[sdkChainId] = (async () => { const etherspotModularSdk = new ModularSdk( this.config.provider as WalletProvider, { chainId: +sdkChainId, chain: customChain, bundlerProvider: new EtherspotBundler( +sdkChainId, - this.config.bundlerApiKey ?? '__ETHERSPOT_BUNDLER_API_KEY__' + this.config.bundlerApiKey ?? DEFAULT_BUNDLER_API_KEY ), factoryWallet: 'etherspot' as Factory, } );lib/EtherspotUtils.ts (1)
75-87: Use class name instead ofthisin static context.The static analysis correctly identifies that using
thisin a static method can be confusing. Use the class name for clarity.static isZeroAddress(address: string): boolean { const zeroAddresses = [ zeroAddressConstant, '0x000000000000000000000000000000000000dEaD', '0xdeaDDeADDEaDdeaDdEAddEADDEAdDeadDEADDEaD', '0xEeeeeEeeeEeEeeEeEeEeeEEEeeeeEeeeeeeeEEeE', '0xDDdDddDdDdddDDddDDddDDDDdDdDDdDDdDDDDDDd', '0xFFfFfFffFFfffFFfFFfFFFFFffFFFffffFfFFFfF', ]; return zeroAddresses.some((zeroAddress) => - this.addressesEqual(zeroAddress, address) + EtherspotUtils.addressesEqual(zeroAddress, address) ); }lib/interfaces/index.ts (1)
278-297: Consider tracking module support in a separate issue.The commented code indicates planned support for modular components (validators, executors, hooks, fallbacks). Rather than keeping this as commented code, consider creating a GitHub issue to track this future enhancement.
Would you like me to create an issue to track the implementation of module support with these interface definitions?
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (2)
example/package-lock.jsonis excluded by!**/package-lock.jsonpackage-lock.jsonis excluded by!**/package-lock.json
📒 Files selected for processing (14)
__tests__/EtherspotProvider.test.ts(1 hunks)__tests__/EtherspotTransactionKit.test.ts(1 hunks)example/src/App.tsx(1 hunks)lib/EtherspotProvider.ts(1 hunks)lib/EtherspotUtils.ts(1 hunks)lib/TransactionKit.ts(1 hunks)lib/index.ts(1 hunks)lib/interfaces/index.ts(1 hunks)lib/utils/index.ts(2 hunks)package.json(3 hunks)rollup.config.js(4 hunks)src/index.ts(0 hunks)src/types/Helper.ts(0 hunks)tsconfig.json(1 hunks)
💤 Files with no reviewable changes (2)
- src/types/Helper.ts
- src/index.ts
✅ Files skipped from review due to trivial changes (1)
- lib/index.ts
🚧 Files skipped from review as they are similar to previous changes (5)
- tsconfig.json
- rollup.config.js
- package.json
- tests/EtherspotProvider.test.ts
- tests/EtherspotTransactionKit.test.ts
🧰 Additional context used
🧬 Code Graph Analysis (2)
lib/EtherspotProvider.ts (2)
__mocks__/@etherspot/modular-sdk.js (1)
ModularSdk(10-178)lib/interfaces/index.ts (1)
EtherspotProviderConfig(19-23)
lib/EtherspotUtils.ts (1)
lib/utils/index.ts (1)
addressesEqual(22-30)
🪛 Biome (1.9.4)
example/src/App.tsx
[error] 184-184: Change to an optional chain.
Unsafe fix: Change to an optional chain.
(lint/complexity/useOptionalChain)
[error] 682-685: Change to an optional chain.
Unsafe fix: Change to an optional chain.
(lint/complexity/useOptionalChain)
lib/EtherspotUtils.ts
[error] 14-98: Avoid classes that contain only static members.
Prefer using simple functions instead of classes with only static members.
(lint/complexity/noStaticOnlyClass)
[error] 85-85: Using this in a static context can be confusing.
this refers to the class.
Unsafe fix: Use the class name instead.
(lint/complexity/noThisInStatic)
🔇 Additional comments (5)
lib/interfaces/index.ts (5)
14-24: Well-structured provider configuration interfaces.The
TypePerIdgeneric type andEtherspotProviderConfiginterface are properly defined with appropriate optional properties.
26-123: Excellent fluent API design with clear interface progression.The interface hierarchy (
IInitial→ITransaction→INamedTransaction→IBatchedTransaction) provides a well-structured builder pattern. The consistent inclusion ofgetState()andreset()methods across all interfaces ensures uniform state management capabilities.
125-136: Comprehensive state management interface.The
IInstanceinterface effectively captures all necessary state including transactions, batches, operation flags, and wallet addresses mapped by chain.
138-250: Well-designed parameter and result interfaces with proper error handling.The interfaces provide:
- Flexible value types (bigint | string) for compatibility
- Comprehensive error categorization with specific error types
- Clear separation between estimation and sending results
- Proper nesting for batch operations
252-276: Flexible UserOp interface supporting both synchronous and asynchronous field resolution.The
EtherspotPromiseOrValue<T>pattern elegantly handles cases where values may need asynchronous resolution, which is particularly useful for gas estimation and signature generation workflows.
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (1)
lib/TransactionKit.ts (1)
633-651: Fix the validation logic or update the comment to match the implementation.The comment states "Cannot have both value = '0' and data = '0x'" but the code checks if either
valueordataisundefined. This is a mismatch between the comment and implementation.Either update the comment to reflect what's being checked:
- // Validation: Cannot have both value = '0' and data = '0x' + // Validation: value and data must be defined if ( this.workingTransaction?.value === undefined || this.workingTransaction?.data === undefined ) {Or implement the validation described in the comment:
- // Validation: Cannot have both value = '0' and data = '0x' - if ( - this.workingTransaction?.value === undefined || - this.workingTransaction?.data === undefined - ) { + // Validation: Cannot have both value = '0' and data = '0x' + if ( + this.workingTransaction?.value === '0' && + this.workingTransaction?.data === '0x' + ) { log( - 'estimate(): value or data undefined, returning error.', + 'estimate(): both value and data are empty, returning error.', undefined, this.debugMode ); return setErrorAndReturn( - 'Invalid transaction: value and data must be defined.', + 'Invalid transaction: cannot have both value = "0" and data = "0x".', 'VALIDATION_ERROR',
🧹 Nitpick comments (1)
lib/TransactionKit.ts (1)
473-516: Redundant validation checks can be simplified.The method has multiple similar validation checks that can be consolidated for better maintainability.
- if (!this.selectedTransactionName || !this.workingTransaction) { - this.throwError( - 'update(): No named transaction to update. Call name() first.' - ); - } - if (!this.workingTransaction) { - this.throwError('update(): No working transaction to update.'); - } - const transactionName = this.selectedTransactionName; - if (!transactionName) { - this.throwError('update(): No selected transaction name.'); - } + if (!this.selectedTransactionName || !this.workingTransaction) { + this.throwError( + 'update(): No named transaction to update. Call name() first.' + ); + } + const transactionName = this.selectedTransactionName;
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
lib/TransactionKit.ts(1 hunks)
🔇 Additional comments (13)
lib/TransactionKit.ts (13)
1-66: Clean imports and well-structured class definition.The imports are appropriate, class structure is logical with proper encapsulation using private properties, and the constructor correctly initializes the provider dependency.
67-160: Well-implemented private utility methods.The error handling, state management, and wallet address caching methods are properly implemented with appropriate logging and error handling patterns.
175-236: Comprehensive input validation and clean transaction creation logic.The method properly validates all input parameters, handles BigInt conversion safely, and manages both creation and update scenarios effectively with clear error messages.
252-287: Proper transaction naming and selection logic.The method correctly handles both selecting existing transactions and creating new named transactions, with appropriate validation and state management.
302-329: Clean batch selection with proper interface restriction.The method correctly validates batch existence and returns a restricted interface that only allows appropriate batch operations.
345-390: Solid batch management with proper state synchronization.The method correctly handles batch creation, transaction updates, and maintains consistency between the batch and named transaction stores.
404-459: Comprehensive removal logic with proper state cleanup.The method correctly handles both batch and transaction removal scenarios with appropriate validation and thorough state cleanup.
652-749: Well-structured estimation workflow with proper error handling.The SDK operations, error handling, and state management are implemented correctly with consistent patterns throughout the method.
790-1034: Comprehensive send implementation with proper state cleanup.The method correctly handles the estimate-then-send workflow with appropriate error handling and important state cleanup after successful transactions.
1067-1303: Efficient batch estimation with parallel processing.The method properly handles multiple batches in parallel with consistent error handling and maintains the same patterns as single transaction estimation.
1337-1713: Comprehensive batch sending with proper lifecycle management.The method correctly implements the estimate-then-send workflow for batches with appropriate error handling and important cleanup of successful batches from state.
1724-1815: Well-implemented utility methods for debugging and advanced usage.The utility methods provide appropriate access to internal state, configuration, and underlying components while maintaining encapsulation.
1821-1826: Clean factory function for easier instantiation.The factory function provides a convenient way to instantiate the class with proper typing.
2 participants
Description
Architecture Changes
Scope Reduction
Removed
How Has This Been Tested?
Screenshots (if appropriate):
Types of changes
Summary by CodeRabbit
New Features
Breaking Changes
Bug Fixes
Documentation
Chores