fix/update-transaction-kit-2.0.3 - #191
Conversation
WalkthroughChainId is now required for transactions; TransactionParams updated accordingly and transaction() no longer provides a default. SDK initialization and estimate/send flows use per-transaction or per-batch chainId. Tests and example updated to pass explicit chainId. Version bumped to 2.0.3. Changes
Sequence Diagram(s)sequenceDiagram
autonumber
actor User
participant App as App / Tests
participant TK as TransactionKit
participant SDK as EtherspotModularSdk
Note over App,TK: chainId must be provided on calls
User->>App: call transaction/estimate/send({ chainId, ... })
App->>TK: transaction({ chainId, ... })
TK->>TK: validate chainId (defined, number, integer)
TK->>SDK: init(chainId) %%{style:fill:#f0f8ff}%%
alt estimate
TK->>SDK: estimate(...)
SDK-->>TK: gasEstimate
TK-->>App: result { chainId, gasEstimate, ... }
else send
TK->>SDK: send(...)
SDK-->>TK: txHash / userOpHash
TK->>TK: save successResult (to,value,data,chainId), clear state
TK-->>App: result { chainId, txHash/userOpHash, ... }
end
Note over TK,SDK: Provider chainId is used only if transaction chainId is null/undefined
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Suggested reviewers
Poem
Tip 🔌 Remote MCP (Model Context Protocol) integration is now available!Pro plan users can now connect to remote MCP servers from the Integrations page. Connect with popular remote MCPs such as Notion and Linear to add more context to your reviews and chats. ✨ 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. CodeRabbit Commands (Invoked using PR/Issue comments)Type 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: 4
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
example/src/App.tsx (1)
679-684: Scenario mislabel: this triggers “missing chainId” now, not “missing to”.With
chainIdrequired, this example throws for the wrong reason relative to its label “Transaction with Missing To”. ProvidechainIdand omittoto validate the intended error path.Apply:
- kit.transaction({ value: '1' } as any); + kit.transaction({ chainId: 137, value: '1' } as any);Optionally assert the error message includes “to is required” in the UI log to make the demonstration clearer.
🧹 Nitpick comments (12)
lib/interfaces/index.ts (2)
149-156: TightenTransactionBuilder.chainIdto non-optional for internal consistency.Since
transaction(props: TransactionParams)now requireschainId, any resultingTransactionBuildercreated from it should always havechainId. Keeping it optional increases nullish checks downstream.Apply:
export interface TransactionBuilder { - chainId?: number; + chainId: number; to?: string; value?: bigint | string; data?: string; transactionName?: string; batchName?: string; }If there are legitimate flows that produce “working” builders before validation, consider using a separate “DraftTransactionBuilder” for pre-validation and “TransactionBuilder” (with required
chainId) post-validation.
177-201: Model success/error results as discriminated unions to improve DX and safety.Today
TransactionEstimateResult/TransactionSendResultuse booleans with many optionals. A discriminated union avoids accessing undefined fields on failures and lets you requirechainIdon success while keeping it optional on failure.Example (outside this diff):
type EstimationSuccess = { isEstimatedSuccessfully: true; to?: string; value?: string; data?: string; chainId: number; cost?: bigint; userOp?: UserOp; }; type EstimationFailure = { isEstimatedSuccessfully: false; errorType: 'ESTIMATION_ERROR' | 'VALIDATION_ERROR'; errorMessage: string; }; export type TransactionEstimateResult = EstimationSuccess | EstimationFailure;A similar union can be defined for send results using
isSentSuccessfully.example/src/App.tsx (1)
42-45: Avoid repeating the magic number 137 forchainId.The example hardcodes
chainId: 137in many places. Define aconst CHAIN_ID = 137once and reuse it. This reduces churn if you switch networks and keeps example code tight.Apply pattern:
- kit.transaction({ chainId: 137, to: '0x...', value: '...' }); + const CHAIN_ID = 137; // near the top, once + kit.transaction({ chainId: CHAIN_ID, to: '0x...', value: '...' });Also applies to: 61-64, 91-94, 110-113, 259-262, 280-283, 304-307, 314-317, 338-341, 348-351, 366-369, 403-406, 438-438, 451-454, 491-494, 529-532, 551-554, 567-570, 576-579
__tests__/EtherspotTransactionKit.test.ts (2)
223-229: Covers non-integerchainId; add negative/zero case for completeness.You already validate non-integer (1.5). Consider a quick additional test for
chainId <= 0to catch invalid ranges.You can append:
+ it('should throw error for non-positive chainId', () => { + expect(() => { + transactionKit.transaction({ + to: '0x1234567890123456789012345678901234567890', + chainId: 0, + }); + }).toThrow('transaction(): chainId must be a valid number.'); + expect(() => { + transactionKit.transaction({ + to: '0x1234567890123456789012345678901234567890', + chainId: -1, + }); + }).toThrow('transaction(): chainId must be a valid number.'); + });
4-6: Typo in comment: “EtherspotPovider”.Nit, but easy to clean up.
-// EtherspotPovider +// EtherspotProviderlib/TransactionKit.ts (7)
114-115: Use nullish coalescing for chainId fallback in getWalletAddress()Prefer
??over||for consistency and to avoid falsey pitfalls (even though 0 isn’t a valid chainId, this keeps style consistent across the file).- const walletAddressChainId = chainId || this.etherspotProvider.getChainId(); + const walletAddressChainId = chainId ?? this.etherspotProvider.getChainId();
98-111: Docs/code mismatch: method never throws but JSDoc says it does
getWalletAddress()catches errors and returnsundefined, yet the JSDoc advertises a throw. Align the comment with behavior.- * @throws {Error} If the SDK fails to initialize or the address cannot be fetched due to a critical error. + * @throws Never. Errors from SDK init/address retrieval are caught; the method logs and returns undefined.
176-190: Good: chainId made mandatory with runtime validation; add positive-integer guardSolid move to require
chainIdand validate type/integery. Consider rejecting non-positive values to catch accidental zeros/negatives early.if (typeof chainId !== 'number' || !Number.isInteger(chainId)) { this.throwError('transaction(): chainId must be a valid number.'); } + if (chainId <= 0) { + this.throwError('transaction(): chainId must be a positive integer.'); + }
662-707: Reduce duplication in SDK prepare/clear/add/estimate/send flowsThe same sequence is repeated across single and batch paths: get SDK for a chain -> clear batch -> add ops -> estimate -> totalGas -> send. Extract helpers to DRY the logic and centralize error handling.
Example additions inside the class to reuse:
private async prepareSdk(chainId: number, forceNew = true): Promise<ModularSdk> { return this.etherspotProvider.getSdk(chainId, forceNew); } private async addTxToBatch( sdk: ModularSdk, tx: { to?: string; value?: bigint | string; data?: string } ): Promise<void> { await sdk.clearUserOpsFromBatch(); await sdk.addUserOpsToBatch({ to: tx.to || '', value: tx.value?.toString(), data: tx.data ?? '0x', }); } private async estimateCost( sdk: ModularSdk, estimateArgs: { paymasterDetails?: unknown; gasDetails?: unknown; callGasLimit?: unknown }, userOpOverrides?: Record<string, unknown> ): Promise<{ userOp: any; totalGas: bigint; cost: bigint }> { const base = await sdk.estimate(estimateArgs); const userOp = { ...base, ...(userOpOverrides || {}) }; const totalGas = BigInt((await sdk.totalGasEstimated(userOp)).toString()); const maxFeePerGas = BigInt(userOp.maxFeePerGas.toString()); return { userOp, totalGas, cost: totalGas * maxFeePerGas }; }Then each call site collapses to a small sequence, improving readability and testability.
Also applies to: 869-938, 1140-1215, 1412-1577
399-405: Typo in docstring: “trakit” → “kit”Minor doc polish.
- * Removes the currently selected transaction or batch from the trakit. + * Removes the currently selected transaction or batch from the kit.
128-129: Optional: cache/reuse SDK instances per chain during batch processingYou’re forcing new instances to avoid state pollution, which is safe but may be heavy if many batches share the same chain. Consider a short-lived per-call cache keyed by
chainIdto reuse within a singleestimateBatches()/sendBatches()invocation.I can sketch a scoped cache map (cleared at method exit) if you want to explore this optimization.
Also applies to: 663-667, 874-879, 1140-1146, 1429-1433
1816-1847: Align logging and confirm return type in getTransactionHashThe SDK’s getUserOpReceipt already returns
Promise<string | null>, so no additional extraction logic is required. However, we should replaceconsole.error/console.warnwith ourlog()helper to respectdebugModeand keep output consistent.– File: lib/TransactionKit.ts
– Method: getTransactionHash (lines 1816–1847)Suggested diff:
public async getTransactionHash( userOpHash: string, txChainId: number, timeout: number = 60 * 1000, retryInterval: number = 2000 ): Promise<string | null> { const etherspotModularSdk = await this.getSdk(txChainId); let transactionHash: string | null = null; const timeoutTotal = Date.now() + timeout; while (!transactionHash && Date.now() < timeoutTotal) { await new Promise<void>((resolve) => setTimeout(resolve, retryInterval)); try { - transactionHash = - await etherspotModularSdk.getUserOpReceipt(userOpHash); + transactionHash = await etherspotModularSdk.getUserOpReceipt(userOpHash); } catch (error) { - console.error( - 'Error fetching transaction hash. Please check if the transaction has gone through, or try to send the transaction again:', - error - ); + log( + 'getTransactionHash(): error fetching tx hash', + error, + this.debugMode + ); } } if (!transactionHash) { - console.warn( - 'Failed to get the transaction hash within time limit. Please try again' - ); + log( + 'getTransactionHash(): failed to get tx hash within time limit', + { userOpHash, timeout, retryInterval }, + this.debugMode + ); } return transactionHash; }Note: If you ever need the full transaction receipt, simply pass the hash into your provider, e.g.:
const txHash = await modularSdk.getUserOpReceipt(uoHash); if (txHash) { const receipt = await provider.getTransactionReceipt(txHash); // …use full receipt here }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
💡 Knowledge Base configuration:
- MCP integration is disabled by default for public repositories
- Jira integration is disabled by default for public repositories
- Linear integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
⛔ 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 (6)
CHANGELOG.md(1 hunks)__tests__/EtherspotTransactionKit.test.ts(19 hunks)example/src/App.tsx(19 hunks)lib/TransactionKit.ts(29 hunks)lib/interfaces/index.ts(1 hunks)package.json(1 hunks)
🧰 Additional context used
🧬 Code graph analysis (1)
lib/TransactionKit.ts (2)
lib/interfaces/index.ts (1)
TransactionParams(238-243)lib/utils/index.ts (1)
log(63-68)
🔇 Additional comments (9)
package.json (1)
4-4: Release Version 2.0.3 Verified
- Confirmed package.json version is set to 2.0.3.
- CHANGELOG.md contains the header
## [2.0.3] – 2025-08-22.All checks pass—please proceed with tagging the v2.0.3 release.
lib/interfaces/index.ts (1)
239-243: MakechainIdrequired in TransactionParams — aligned with PR objective.Type-level enforcement here is good and will catch mistakes at compile time.
__tests__/EtherspotTransactionKit.test.ts (1)
196-206: Good: defaulting optional tx fields while requiringchainId.Test now confirms
value/datadefaults with explicitchainId. This matches the new contract.lib/TransactionKit.ts (6)
128-134: LGTM: Modular SDK variable naming is now consistentThe
etherspotModularSdknaming is consistent here and matches usage elsewhere. Good cleanup.
627-629: LGTM: chainId resolution uses nullish coalescingThe error path now prefers the transaction’s
chainIdand only falls back to the provider when undefined/null. Matches the PR objective.
896-904: send(): no support for gasDetails/callGasLimit during inline estimation
estimate()acceptsgasDetails/callGasLimit, butsend()’s inline estimation forwards onlypaymasterDetails. If this is intentional, all good; otherwise consider adding the same knobs for parity.
972-980: LGTM: error-path chainId uses tx chainId first, falls back to providerConsistent with the goal of honoring transaction-specified chain context.
996-1003: Nice: preserve tx details before clearing stateCapturing
successResultavoids losing fields afterclearWorkingState(). Clean and safe.Also applies to: 1021-1027
1230-1239: Per-tx cost equals full batch total; confirm this UXEach transaction in a batch receives
cost: totalCost. This may be confusing to consumers expecting a per-tx allocation. If intended, consider documenting it explicitly or returning bothtotalCost(batch-level) andallocatedCost(per-tx split).I can draft a small change to include
allocatedCost = totalCost / Nfor display while keepingtotalCostin the batch result—let me know if you want that.Also applies to: 1626-1636
Co-authored-by: coderabbitai[bot] <136622811+coderabbitai[bot]@users.noreply.github.com>
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (2)
CHANGELOG.md (2)
3-8: Potential semver mismatch: breaking change in a patch release (2.0.3)If the API now throws when
chainIdis omitted, that’s a backward-incompatible change and typically warrants a major version bump (e.g., 3.0.0), or at minimum a clearly labeled “Breaking Changes” section with migration notes. Please confirm intended semver strategy.Run to validate impact and decide on versioning:
#!/bin/bash set -euo pipefail echo "=== Check if chainId is still defaulted anywhere ===" rg -nP --type ts '(?s)\btransaction\s*\(\s*\{\s*[^}]*\bchainId\s*=\s*' -C2 || true echo echo "=== Check if TransactionParams still allows optional chainId ===" rg -nP --type ts '\binterface\s+TransactionParams\b(?s).*?\bchainId\?' -C2 || true echo echo "=== Tests expecting default chainId behavior ===" rg -nP --glob '*test*' 'expect\(.*chainId.*\)\.to(Be|Equal)\(\s*1\s*\)' -n -C2 || true echo echo "=== Residual typos of etherspotModulaSdk ===" rg -n 'etherspotModulaSdk' -n -C2 || true
5-8: Reclassify 2.0.3 notes as Breaking Changes and expand scope to include send()/estimate() requirementGiven the API now requires
chainId(per PR description), this is a breaking change and should not be listed under “Added Changes.” Also, the PR stateschainIdis mandatory for single-transactionsend()/estimate()calls—please capture that explicitly.Apply:
-### Added Changes - -- `chainId` is mandatory in the `transaction()` method. -- For `send` and `estimate`, the `etherspotModularSdk` is initialized using the transaction's `chainId` rather than the provider's `chainId`. +### Breaking Changes +- `transaction()`: `chainId` is now required; calls without it will throw. +- `send()`/`estimate()` (single-transaction mode): `chainId` must be provided on the transaction. + +### Added Changes +- The SDK now initializes `etherspotModularSdk` using the transaction's `chainId` instead of the provider's `chainId`.
🧹 Nitpick comments (2)
CHANGELOG.md (2)
7-8: Add a Fixed section for theetherspotModularSdktypo and tighten wordingThe PR mentions a typo fix; document it. Minor copy edits improve clarity.
Apply:
## [2.0.3] - 2025-08-22 @@ -### Added Changes +### Added Changes @@ -- For `send` and `estimate`, the `etherspotModularSdk` is initialized using the transaction's `chainId` rather than the provider's `chainId`. +- The SDK initializes `etherspotModularSdk` using the transaction's `chainId` rather than the provider's `chainId`. + +### Fixed +- Corrected a typo to consistently use `etherspotModularSdk`.
3-8: Add a short Migration note to reduce consumer frictionA brief snippet helps users adopt 2.0.3 safely.
Apply:
## [2.0.3] - 2025-08-22 @@ +### Migration +- Add `chainId` to all `.transaction({...})` calls. +- For single-transaction `send()` and `estimate()`, ensure the transaction object includes `chainId`.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
💡 Knowledge Base configuration:
- MCP integration is disabled by default for public repositories
- Jira integration is disabled by default for public repositories
- Linear integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (1)
CHANGELOG.md(1 hunks)
🧰 Additional context used
🧬 Code graph analysis (1)
CHANGELOG.md (1)
lib/TransactionKit.ts (1)
EtherspotTransactionKit(37-1864)
🪛 LanguageTool
CHANGELOG.md
[grammar] ~7-~7: There might be a mistake here.
Context: ...mandatory in the transaction() method. - For send and estimate, the `etherspo...
(QB_NEW_EN)
2 participants
Description
How Has This Been Tested?
Screenshots (if appropriate):
Types of changes
Summary by CodeRabbit
New Features
Tests
Documentation
Chores