feat(sdk): ContractWriter + AA support #461
Conversation
|
Note Other AI code review bot(s) detectedCodeRabbit has detected other AI code review bot(s) in this pull request and will avoid duplicating their findings in the review comments. This may lead to a less comprehensive review. WalkthroughAdds a ContractWriter abstraction (EOA and AA v0.6 implementations), ABI entries for AA execute/user operation event, types/interfaces for v0.6 bundler interactions, migrates client and services to route writes through ContractWriter, and updates configs, errors, signers, and tests accordingly. Changes
Sequence Diagram(s)sequenceDiagram
participant Client as NitroliteClient
participant Writer as AAV06ContractWriter
participant Public as PublicClient
participant Bundler as BundlerClientV06
participant Chain as Blockchain
Client->>Writer: write(callsDetails)
Writer->>Writer: _prepareCalldata / _encodeExecuteBatchCall
Writer->>Public: getChainId, get account nonce
Writer->>Bundler: estimateUserOperation(chainId, partialUserOp)
Bundler-->>Writer: GasParametersV06
Writer->>Writer: format + sign userOp
Writer->>Bundler: sendUserOperation(chainId, rpcUserOp)
Bundler-->>Writer: userOpHash
loop poll
Writer->>Bundler: fetchLogs(chainId, [entryPoint], topics)
Bundler-->>Writer: UserOperationEvent or empty
end
Writer->>Client: return txHash (from UserOperationEvent / receipt)
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45–75 minutes
Possibly related PRs
Suggested reviewers
Poem
Pre-merge checks and finishing touches❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✨ Finishing touches
🧪 Generate unit tests (beta)
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. Comment |
Summary of ChangesHello @MaxMoskalenko, I'm Gemini Code Assist1! I'm currently reviewing this pull request and will post my feedback shortly. In the meantime, here's a summary to help you and other reviewers quickly get up to speed! This pull request significantly enhances the SDK's flexibility by introducing an abstraction layer for on-chain transaction execution. By decoupling the client from direct Highlights
Using Gemini Code AssistThe full guide for Gemini Code Assist can be found on our documentation page, here are some quick tips. Invoking Gemini You can request assistance from Gemini at any point by creating a comment using either
Customization To customize Gemini Code Assist for GitHub experience, repository maintainers can create a configuration file and/or provide a custom code review style guide (such as PEP-8 for Python) by creating and adding files to a Limitations & Feedback Gemini Code Assist may make mistakes. Please leave feedback on any instances where its feedback is incorrect or counter productive. You can react with 👍 and 👎 on @gemini-code-assist comments. If you're interested in giving your feedback about your experience with Gemini Code Assist for Github and other Google products, sign up here. You can also get AI-powered code generation, chat, as well as code reviews directly in the IDE at no cost with the Gemini Code Assist IDE Extension. Footnotes
|
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request introduces a ContractWriter abstraction, a significant architectural improvement for supporting different transaction submission strategies like EOA and Account Abstraction. The refactoring is extensive and well-applied across the client, services, and preparer classes. However, I've identified a few critical issues that need to be addressed before merging. Specifically, there's a bug in the EOAContractWriter's transaction handling, a broken approveTokens method due to the refactoring, and a logic issue in the AAV06ContractWriter's polling mechanism. I've also included several medium-severity comments violência TODOs and code clarity.
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 8
🧹 Nitpick comments (13)
sdk/src/client/services/Erc20Service.ts (1)
126-127: Address the TODO comment before merging.The method lacks JSDoc documentation. Consider adding documentation similar to
prepareApproveto describe the purpose, parameters, and return value.Would you like me to generate the JSDoc documentation for this method?
sdk/src/client/helpers.ts (1)
4-10: Consider using SDK error classes for consistency.The function throws a generic
Errorinstead of using the SDK's custom error hierarchy (e.g.,InvalidStateTransitionErroror a new specific error). This inconsistency could make error handling harder for consumers.+import { Errors } from '../errors'; import { Account, Address, Hash } from 'viem'; import { WriteResult } from './contract_writer/types'; export function getLastTxHashFromWriteResult(writeResult: WriteResult): Hash { if (writeResult.txHashes.length < 1) { - throw new Error('No transaction hashes returned from write operation'); + throw new Errors.InvalidParameterError('No transaction hashes returned from write operation', { writeResult }); } return writeResult.txHashes[writeResult.txHashes.length - 1]; }sdk/src/abis/aa/execute.ts (1)
1-78: Consider addingas constassertion for better type inference.Adding
as constallows TypeScript to infer literal types for the ABI, which improves type safety when used with viem's typed contract functions.-export const AAExecuteAbi = [ +export const AAExecuteAbi = [ // ... content unchanged ... -]; +] as const;sdk/src/client/contract_writer/eoa.ts (1)
56-60: Consider removing theas anytype assertion.The
as anycast bypasses TypeScript's type safety. Consider using proper type narrowing or investigating why the types don't align betweensimulateContractresult andwriteContractparameters.sdk/test/unit/client/index.test.ts (1)
122-136: Consider maintaining specific error type assertions.The error expectations changed from specific error types (
TokenError,ContractCallError) to genericError. While this may be intentional given the refactoring, consider restoring specific error type assertions once the error handling in the production code is finalized to ensure proper error categorization.sdk/src/client/contract_writer/types.ts (2)
1-1: Unused import:ContractFunctionParameters.The
ContractFunctionParameterstype is imported but not used in this file.-import { Abi, Account, Address, Chain, ContractFunctionParameters, Hex } from 'viem'; +import { Abi, Account, Address, Chain, Hex } from 'viem';
7-34: Consider documenting the rationale for type restrictions.The
typefield is restricted to only"eip7702"while other transaction types (legacy,eip2930,eip1559) are commented out. This appears intentional for AA support, but a brief comment explaining why would help future maintainers understand the design decision.sdk/src/client/contract_writer/aa_v06.ts (3)
37-45: Add parameter validation as noted by TODO.The TODO at line 38 indicates missing validations. Consider validating:
publicClientis definedsmartAccountis definedbundlerClientis definedpollingIntervalandpollingTimeoutare positive numbers if providedDo you want me to generate the validation logic?
54-56: Clarify the purpose ofwithBatch()method.The
withBatch()method returnsfalse, but this class batches calls viaexecuteBatch. This method isn't part of theContractWriterinterface. If it's meant to indicate whether external batching is supported vs internal batching, consider adding a JSDoc comment to explain its purpose.
78-85: Type assertion with@ts-ignoremasks potential type mismatch.The
@ts-ignorecomment suppresses type checking. The nonce conversion at line 82 manually converts to hex string, but ifpartialUserOperation.nonceis already a bigint, this should work. Consider using proper typing instead of suppressing errors.- const userOperation = this._formatUserOperation( - // @ts-ignore - { - ...partialUserOperation, - nonce: ('0x' + partialUserOperation.nonce.toString(16)) as Hex, - ...gasParameters, - }, - ) as Required<UserOperation<'0.6'>>; + const userOperation = this._formatUserOperation({ + ...partialUserOperation, + nonce: numberToHex(partialUserOperation.nonce), + ...gasParameters, + } as RpcUserOperation) as Required<UserOperation<'0.6'>>;sdk/src/client/services/NitroliteService.ts (3)
1-1: Unused import:ContractFunctionParameters.The
ContractFunctionParameterstype is imported but not used in this file.-import { Account, Address, PublicClient, WalletClient, Hash, zeroAddress, Hex, ContractFunctionParameters } from 'viem'; +import { Account, Address, PublicClient, WalletClient, Hash, zeroAddress, Hex } from 'viem';
34-41: Outdated comments reference walletClient.The comments (lines 34-41) still reference "simulateContract" and "writeContract" patterns, but the implementation now uses
contractWriter.write. Consider updating the comments to reflect the new abstraction.
184-197: Address TODO comments for documentation.Multiple
prepareXCallParamsmethods (lines 184, 243, 302, 381, 440, 505, 582, 647, 708) have// TODO: commentplaceholders. These public methods should have JSDoc documentation explaining their purpose and parameters, similar to the existingprepareXmethods.Do you want me to generate the JSDoc comments for these methods?
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (16)
sdk/src/abis/aa/execute.ts(1 hunks)sdk/src/abis/aa/user_op_event.ts(1 hunks)sdk/src/client/contract_writer/aa_v06.ts(1 hunks)sdk/src/client/contract_writer/aa_v06_types.ts(1 hunks)sdk/src/client/contract_writer/eoa.ts(1 hunks)sdk/src/client/contract_writer/types.ts(1 hunks)sdk/src/client/helpers.ts(1 hunks)sdk/src/client/index.ts(15 hunks)sdk/src/client/prepare.ts(4 hunks)sdk/src/client/services/Erc20Service.ts(3 hunks)sdk/src/client/services/NitroliteService.ts(25 hunks)sdk/src/client/state.ts(1 hunks)sdk/src/client/types.ts(3 hunks)sdk/src/errors.ts(2 hunks)sdk/test/unit/client/index.test.ts(14 hunks)sdk/test/unit/client/services/NitroliteService.test.ts(3 hunks)
🧰 Additional context used
🧠 Learnings (1)
📚 Learning: 2025-11-05T10:19:06.155Z
Learnt from: dimast-x
Repo: erc7824/nitrolite PR: 411
File: clearnode/custody.go:449-465
Timestamp: 2025-11-05T10:19:06.155Z
Learning: In clearnode/custody.go's handleResized function during positive resize operations (deposits), the ledger flow is: on-chain funds first credit the channel account, are immediately debited from the channel account, then credit the unified wallet account. The channel account acts as a pass-through for deposits. The unified wallet account is where actual deposited funds are held. This is recorded as a deposit transaction from channelAccountID to walletAccountID.
Applied to files:
sdk/src/client/services/NitroliteService.ts
🧬 Code graph analysis (9)
sdk/test/unit/client/index.test.ts (1)
clearnode/pkg/rpc/error.go (1)
Error(48-50)
sdk/src/client/helpers.ts (2)
sdk/src/client/contract_writer/types.ts (1)
WriteResult(40-42)clearnode/pkg/sign/sign.go (1)
Address(29-34)
sdk/src/client/services/Erc20Service.ts (2)
sdk/src/client/contract_writer/types.ts (1)
ContractCallParams(7-34)sdk/src/abis/token/index.ts (1)
Erc20Abi(6-34)
sdk/src/client/contract_writer/aa_v06.ts (4)
sdk/src/client/contract_writer/aa_v06_types.ts (2)
BundlerClientV06(26-30)PartialUserOperationV06(4-15)sdk/src/client/contract_writer/types.ts (4)
ContractWriter(3-5)CallsDetails(36-38)WriteResult(40-42)ContractCallParams(7-34)sdk/src/abis/aa/user_op_event.ts (1)
UserOpEventAbi(3-51)sdk/src/abis/aa/execute.ts (1)
AAExecuteAbi(1-78)
sdk/src/client/types.ts (1)
sdk/src/client/contract_writer/types.ts (1)
ContractWriter(3-5)
sdk/src/client/contract_writer/eoa.ts (2)
sdk/src/client/contract_writer/types.ts (4)
ContractWriter(3-5)CallsDetails(36-38)WriteResult(40-42)ContractCallParams(7-34)clearnode/pkg/rpc/error.go (1)
Error(48-50)
sdk/src/client/prepare.ts (1)
sdk/src/client/helpers.ts (1)
getAccountAddress(12-17)
sdk/src/client/contract_writer/types.ts (1)
clearnode/pkg/sign/sign.go (1)
Address(29-34)
sdk/test/unit/client/services/NitroliteService.test.ts (2)
sdk/src/client/services/NitroliteService.ts (1)
NitroliteService(60-915)sdk/src/errors.ts (1)
Errors(372-399)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (3)
- GitHub Check: Build and Publish (Clearnode)
- GitHub Check: Test (Integration) / Test Integration
- GitHub Check: Analyze (go)
🔇 Additional comments (24)
sdk/src/errors.ts (2)
191-202: LGTM!The new
ContractWriterRequiredErrorfollows the established pattern ofWalletClientRequiredErrorand integrates well with the existing error hierarchy.
385-385: LGTM!The export placement is consistent with the existing error organization.
sdk/src/client/services/Erc20Service.ts (1)
149-161: LGTM!The refactor cleanly delegates parameter construction to
prepareApproveCallParamswhile preserving the existing error handling and simulation logic.sdk/src/client/helpers.ts (1)
12-17: LGTM!The type guard pattern correctly handles both
Accountobjects and rawAddressstrings, providing proper type narrowing.sdk/src/client/types.ts (2)
117-153: Good use of discriminated union for mutually exclusive configuration paths.The design cleanly separates legacy WalletClient-based configuration from the new ContractWriter approach. The
?: neverpattern correctly ensures consumers cannot mix both approaches.
137-147: LGTM!Requiring
accountexplicitly when usingContractWriteris the correct design choice, as the contract writer abstraction may not have inherent knowledge of the executing account (especially for Account Abstraction scenarios).sdk/src/abis/aa/user_op_event.ts (1)
1-51: LGTM!The ABI definition correctly represents the ERC-4337 UserOperationEvent from EntryPoint v0.6, with proper indexed/non-indexed field configuration and accurate type definitions.
sdk/src/client/contract_writer/eoa.ts (1)
62-74: LGTM!The
waitForTransactionandwithBatchmethods are correctly implemented. The revert handling is appropriate for EOA transactions.sdk/src/client/prepare.ts (2)
24-24: LGTM!The updated
PreparerDependencies.accounttype toAccount | Addressand usage ofgetAccountAddresshelper provides flexibility to support both EOA and AA flows while maintaining correct address resolution.Also applies to: 40-40
72-72: Consistent address resolution.The
getAccountAddresshelper is correctly used in bothprepareDepositTransactionsandprepareDepositAndCreateChannelTransactionsto derive the owner address, ensuring consistent behavior regardless of whether anAccountobject or rawAddressis provided.Also applies to: 133-133
sdk/test/unit/client/index.test.ts (3)
47-50: LGTM!The mock
contractWritersetup correctly implements theContractWriterinterface pattern, returning the expectedWriteResultshape withtxHashes.
59-67: LGTM!The mock service correctly reflects the shift to
prepareXCallParamsmethods, aligning with the new contract-writer-based orchestration pattern.
94-108: LGTM!Test cases consistently verify the new
contractWriter.writeflow with the expected{ calls: [...] }payload shape and properly assert on the returned transaction hash fromWriteResult.txHashes.Also applies to: 167-187, 195-223, 226-252, 254-301, 346-369, 383-396
sdk/test/unit/client/services/NitroliteService.test.ts (2)
40-43: LGTM!The test setup correctly initializes
mockContractWriterand passes it to theNitroliteServiceconstructor, aligning with the updated constructor signature that accepts aContractWriterdependency.
159-180: LGTM!The execution tests correctly verify the new
contractWriter.writeflow:
- Expects the call payload shape
{ calls: [{ ...req, account }] }- Returns the first hash from
txHashes- Error handling properly uses
ContractWriterRequiredErrorinstead of the deprecatedWalletClientRequiredErrorsdk/src/client/contract_writer/aa_v06_types.ts (1)
1-30: Well-structured type definitions for AA v0.6 support.The type definitions cleanly model the v0.6 UserOperation lifecycle:
PartialUserOperationV06correctly makes gas-related fields optional for the estimation phaseGasParametersV06captures the bundler's gas estimation responseBundlerClientV06interface provides a clean abstraction for bundler interactionssdk/src/client/contract_writer/types.ts (1)
36-42: Clean interface design for batched writes.The
CallsDetailsandWriteResultinterfaces provide a straightforward abstraction for batching multiple contract calls and returning multiple transaction hashes.sdk/src/client/index.ts (4)
35-46: Good separation of concerns with ContractWriter abstraction.The refactoring cleanly separates transaction writing concerns from the main client. The
accounttype widening toAccount | Addressproperly supports both initialization paths.
65-78: Clear initialization paths with proper validation.The constructor correctly handles both initialization modes with appropriate error messages. The mutual exclusivity between
walletClientandcontractWriteris properly enforced.
109-130: Clean refactoring of deposit to use batched writes.The
depositmethod now properly batches the optional ERC20 approval with the deposit call, allowing both to be executed atomically when using AA. Good use ofgetAccountAddressandgetLastTxHashFromWriteResulthelpers.
139-317: Consistent refactoring across all write operations.All channel operations (
createChannel,depositAndCreateChannel,checkpointChannel,challengeChannel,resizeChannel,closeChannel,withdrawal) follow a consistent pattern:
- Prepare call params using
prepareXCallParamshelpers- Execute via
contractWriter.write- Extract final hash via
getLastTxHashFromWriteResultError handling is properly preserved.
sdk/src/client/contract_writer/aa_v06.ts (1)
170-184: Clean batch encoding implementation.The
_encodeExecuteBatchCallmethod properly encodes multiple calls for the smart account'sexecuteBatchfunction with appropriate defaults for optional fields.sdk/src/client/services/NitroliteService.ts (2)
137-153: Good ABI conversion with proper readonly typing.The
convertStateForABImethod properly converts mutable arrays to readonly arrays for viem/ABI compatibility. The explicit typing ofsigsat line 151 ensures type safety.
229-241: Consistent and clean refactoring of all write operations.All write operations (
deposit,createChannel,depositAndCreateChannel,joinChannel,checkpoint,challenge,resize,close,withdraw) follow a consistent pattern:
- Ensure contractWriter and account are available
- Prepare the request via simulation
- Execute via
executeWriteContract- Proper error handling with
NitroliteErrorpass-throughThis refactoring maintains the existing API while enabling AA support.
Also applies to: 288-300, 362-379, 426-438, 491-503, 563-580, 633-645, 694-706, 751-763
| nonce: numberToHex(authorization.nonce), | ||
| r: authorization.r ? numberToHex(BigInt(authorization.r), { size: 32 }) : pad('0x', { size: 32 }), | ||
| s: authorization.s ? numberToHex(BigInt(authorization.s), { size: 32 }) : pad('0x', { size: 32 }), | ||
| yParity: authorization.yParity ? numberToHex(authorization.yParity, { size: 1 }) : pad('0x', { size: 32 }), |
There was a problem hiding this comment.
Inconsistent padding size for yParity fallback.
Line 244 uses size: 32 for padding zero yParity, but the truthy branch (line 244) uses size: 1. This inconsistency could cause issues with authorization formatting.
- yParity: authorization.yParity ? numberToHex(authorization.yParity, { size: 1 }) : pad('0x', { size: 32 }),
+ yParity: authorization.yParity ? numberToHex(authorization.yParity, { size: 1 }) : pad('0x', { size: 1 }),📝 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.
| yParity: authorization.yParity ? numberToHex(authorization.yParity, { size: 1 }) : pad('0x', { size: 32 }), | |
| yParity: authorization.yParity ? numberToHex(authorization.yParity, { size: 1 }) : pad('0x', { size: 1 }), |
🤖 Prompt for AI Agents
In sdk/src/client/contract_writer/aa_v06.ts around line 244, the yParity
fallback uses pad('0x', { size: 32 }) while the truthy branch uses
numberToHex(..., { size: 1 }); make the fallback padding size consistent with
the truthy branch by changing the pad call to use { size: 1 } so both branches
produce a 1-byte hex value for yParity.
Codecov Report✅ All modified and coverable lines are covered by tests. 📢 Thoughts on this report? Let us know! |
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (11)
sdk/src/client/contract_writer/eoa.ts (1)
60-71: Address the TODO and improve type safety.
- The
as anyassertion on line 70 bypasses type checking and could hide type mismatches.- The TODO indicates missing error handling - consider wrapping the calls in try-catch and throwing a more specific error type from
errors.ts.private async _writeCall(callParams: ContractCallParams): Promise<Hex> { - // TODO: add error handling - const { request } = await this.publicClient.simulateContract({ - ...callParams, - account: this.account, - }); - - return this.walletClient.writeContract({ - ...request, - account: this.account, - } as any); + try { + const { request } = await this.publicClient.simulateContract({ + ...callParams, + account: this.account, + }); + + return this.walletClient.writeContract({ + ...request, + account: this.account, + } as Parameters<typeof this.walletClient.writeContract>[0]); + } catch (error) { + throw new Errors.ContractCallError( + `Failed to execute ${callParams.functionName}`, + error as Error, + ); + } }sdk/src/client/state.ts (1)
193-199: JSDoc describes backward compatibility behavior that may be outdated.The comments reference "backward compatibility with channels created prior to v0.5.0" but the implementation simply checks if the participant matches the account address. Consider updating the documentation to accurately reflect the current behavior.
sdk/src/client/services/Erc20Service.ts (1)
152-163: Add JSDoc documentation as indicated by the TODO.-// TODO: comment +/** + * Prepares the call parameters for an ERC20 approve transaction. + * Useful for batching multiple calls via ContractWriter. + * @param tokenAddress Address of the ERC20 token. + * @param spender Address of the spender. + * @param amount Amount to approve. + * @returns The prepared ContractCallParams object. + * @throws {AccountRequiredError} If no account is available. + */ prepareApproveCallParams(tokenAddress: Address, spender: Address, amount: bigint): ContractCallParams {sdk/src/client/contract_writer/aa_v06.ts (6)
31-31: Consider a more descriptive class name.The TODO suggests finding a better name. Consider
AccountAbstractionWriterV06orSmartAccountContractWriterV06to more clearly convey that this handles AA v0.6 smart account operations.
64-76: Critical: Value field hardcoded to zero breaks ETH transfers.Line 73 hardcodes
value: BigInt(0), completely ignoringcallParams.value. This prevents any ETH transfers through the AA contract writer, breaking deposit flows and any operation requiring value transfer.Apply this diff to fix:
private _prepareCalldata(callParams: ContractCallParams): Call { const encoded = encodeFunctionData({ abi: callParams.abi, functionName: callParams.functionName, args: callParams.args, }); return { to: callParams.address, - value: BigInt(0), + value: callParams.value ?? 0n, data: encoded, }; }
84-91: Fix the type mismatch instead of using@ts-ignore.The
@ts-ignoresuppresses a legitimate type error betweenPartialUserOperationV06(usesbigint) andRpcUserOperation(usesHexstrings). This hides a type incompatibility that could cause runtime issues.Instead of suppressing the error, properly convert all
bigintfields toHexbefore passing to_formatUserOperation:- const userOperation = this._formatUserOperation( - // @ts-ignore - { - ...partialUserOperation, - nonce: ('0x' + partialUserOperation.nonce.toString(16)) as Hex, - ...gasParameters, - }, - ) as Required<UserOperation<'0.6'>>; + const rpcUserOp: RpcUserOperation = { + sender: partialUserOperation.sender, + nonce: numberToHex(partialUserOperation.nonce), + initCode: partialUserOperation.initCode, + callData: partialUserOperation.callData, + paymasterAndData: partialUserOperation.paymasterAndData, + signature: partialUserOperation.signature, + ...gasParameters, + }; + const userOperation = this._formatUserOperation(rpcUserOp) as Required<UserOperation<'0.6'>>;
143-158: Critical: Missing return after resolve causes unhandled error.After resolving the promise with
txHashat line 154, execution continues to line 157 which unconditionally throws an error. This causes an unhandled error in the interval callback and breaks the control flow.Add a return statement after resolve:
if (logs.length > 0) { const txHash = logs[logs.length - 1].transactionHash; if (txHash) { clearInterval(intervalId); resolve(txHash); + return; } throw new Error('UserOperationEvent log does not contain transactionHash'); }
137-147: Non-null assertion may cause runtime error if chain not configured.Line 144 uses
this.publicClient.chain!.idwith a non-null assertion. If the publicClient was instantiated without a chain configuration, this will throw at runtime.Add a defensive check:
+ const chainId = this.publicClient.chain?.id; + if (!chainId) { + clearInterval(intervalId); + reject(new Error('PublicClient chain is not configured')); + return; + } const logs = await this.bundlerClient.fetchLogs( - this.publicClient.chain!.id, + chainId, [entryPoint06Address], [[toEventSelector(UserOpEventAbi)], [userOpHash]], );
243-252: Inconsistent padding size for yParity fallback.Line 250 pads the yParity fallback to 32 bytes (
{ size: 32 }), while the truthy branch uses 1 byte ({ size: 1 }). This inconsistency can cause authorization formatting issues.Make the padding size consistent:
- yParity: authorization.yParity ? numberToHex(authorization.yParity, { size: 1 }) : pad('0x', { size: 32 }), + yParity: authorization.yParity ? numberToHex(authorization.yParity, { size: 1 }) : pad('0x', { size: 1 }),sdk/src/client/services/NitroliteService.ts (2)
187-200: Add JSDoc comments to allprepareXCallParamsmethods.The TODO comments at lines 187, 246, 305, 384, 443, 508, 585, 650, and 711 indicate missing documentation for the new
prepareXCallParamshelper methods. These public methods should have JSDoc comments explaining their purpose, parameters, and return values for better usability and maintainability.Example for
prepareDepositCallParams:+ /** + * Builds ContractCallParams for a deposit operation. + * Used internally by prepareDeposit and can be used for custom batch operations. + * @param tokenAddress Address of the token (use zeroAddress for ETH). + * @param amount Amount to deposit. + * @returns ContractCallParams configured for the deposit operation. + */ - // TODO: comment prepareDepositCallParams(tokenAddress: Address, amount: bigint): ContractCallParams {Apply similar documentation to all other
prepareXCallParamsmethods.Also applies to: 246-259, 305-324, 384-395, 443-456, 508-526, 585-598, 650-663, 711-722
84-92: Fix the type mismatch instead of using@ts-ignore.The
@ts-ignoreat lines 89-90 suppresses a type mismatch becausewalletClientis typed as bareWalletClient, butEOAContractWriterexpectsWalletClient<Transport, Chain, ParseAccount<Account>>.Update the constructor parameter type to match
EOAContractWriterConfig:constructor( publicClient: PublicClient, addresses: ContractAddresses, - walletClient?: WalletClient, + walletClient?: WalletClient<Transport, Chain, ParseAccount<Account>>, account?: Account | Address, contractWriter?: ContractWriter, ) {Then remove the
@ts-ignore:if (contractWriter) { this.contractWriter = contractWriter; } else if (walletClient) { this.contractWriter = new EOAContractWriter({ publicClient, - // @ts-ignore walletClient, }); }
🧹 Nitpick comments (6)
sdk/src/rpc/api.ts (1)
832-884: Implementation is correct.The function properly validates that the
AccountsupportssignTypedData(which is optional on theAccounttype), and the signing call correctly omits theaccountparameter since it's already bound to theAccountinstance.Consider extracting shared logic to reduce duplication. Both
createEIP712AuthMessageSignerandcreateEIP712AuthMessageSignerFromAccountshare nearly identical validation and message construction logic. You could extract a helper function:// Helper to extract and validate challenge, then build the message function buildEIP712AuthMessage( payload: RPCData, walletAddress: Address, partialMessage: PartialEIP712AuthMessage, ): EIP712AuthMessage { const method = payload[1]; if (method !== RPCMethod.AuthVerify) { throw new Error( `This EIP-712 signer is designed only for the '${RPCMethod.AuthVerify}' method, but received '${method}'.`, ); } const params = payload[2]; if (!('challenge' in params) || typeof params.challenge !== 'string') { throw new Error('Invalid payload for AuthVerify: The challenge string is missing or malformed.'); } return { ...partialMessage, challenge: params.challenge, wallet: walletAddress, }; }This would consolidate the validation logic and reduce maintenance burden if the message structure changes.
sdk/src/client/contract_writer/types.ts (1)
1-1: Remove unused importContractFunctionParameters.The
ContractFunctionParameterstype is imported but not used anywhere in this file.-import { Abi, Account, Address, Chain, ContractFunctionParameters, Hex } from 'viem'; +import { Abi, Account, Address, Chain, Hex } from 'viem';sdk/src/client/contract_writer/eoa.ts (1)
73-81: Consider including the transaction hash in the error message.When a transaction reverts, including the hash helps with debugging and tracing.
if (receipt.status === 'reverted') { - throw new Error(`Transaction reverted`); + throw new Error(`Transaction ${hash} reverted`); }sdk/src/client/state.ts (1)
200-206: Use strict equality for address comparison.Address comparisons should use strict equality (
===) and consider case-insensitive comparison since Ethereum addresses can have mixed case (checksum encoding).function _checkParticipantAndGetSigner(deps: PreparerDependencies, participant: Address): StateSigner { let signer = deps.stateSigner; - if (participant == deps.account.address) { + if (participant.toLowerCase() === deps.account.address.toLowerCase()) { signer = new AccountStateSigner(deps.account); } return signer; }sdk/src/client/services/Erc20Service.ts (2)
71-79: Avoid@ts-ignore- fix the type mismatch instead.The
@ts-ignoresuppresses a type error that should be addressed. TheEOAContractWriterexpects a specificWalletClienttype withParseAccount<Account>, but the constructor receives a genericWalletClient.Consider updating the constructor signature to accept the correct type:
constructor( publicClient: PublicClient, - walletClient?: WalletClient, + walletClient?: WalletClient<any, any, Account>, account?: Account | Address, contractWriter?: ContractWriter, )Or handle the type assertion explicitly with a comment explaining why it's safe.
21-50: Consider extracting duplicate helper logic.The
executeWriteContractfunction duplicates logic found inNitroliteClient._getLastTxHashFromWriteResult. Consider extracting this to a shared utility to maintain DRY principles.// In a shared utility file export function getLastTxHashFromWriteResult(result: WriteResult): Hash { if (result.txHashes.length < 1) { throw new Error('No transaction hashes returned from write operation'); } return result.txHashes[result.txHashes.length - 1]; }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (11)
sdk/src/client/contract_writer/aa_v06.ts(1 hunks)sdk/src/client/contract_writer/eoa.ts(1 hunks)sdk/src/client/contract_writer/types.ts(1 hunks)sdk/src/client/index.ts(14 hunks)sdk/src/client/prepare.ts(0 hunks)sdk/src/client/services/Erc20Service.ts(7 hunks)sdk/src/client/services/NitroliteService.ts(25 hunks)sdk/src/client/signer.ts(2 hunks)sdk/src/client/state.ts(3 hunks)sdk/src/client/types.ts(3 hunks)sdk/src/rpc/api.ts(2 hunks)
💤 Files with no reviewable changes (1)
- sdk/src/client/prepare.ts
🧰 Additional context used
🧠 Learnings (1)
📚 Learning: 2025-11-05T10:19:06.155Z
Learnt from: dimast-x
Repo: erc7824/nitrolite PR: 411
File: clearnode/custody.go:449-465
Timestamp: 2025-11-05T10:19:06.155Z
Learning: In clearnode/custody.go's handleResized function during positive resize operations (deposits), the ledger flow is: on-chain funds first credit the channel account, are immediately debited from the channel account, then credit the unified wallet account. The channel account acts as a pass-through for deposits. The unified wallet account is where actual deposited funds are held. This is recorded as a deposit transaction from channelAccountID to walletAccountID.
Applied to files:
sdk/src/client/services/NitroliteService.ts
🧬 Code graph analysis (8)
sdk/src/client/types.ts (1)
sdk/src/client/contract_writer/types.ts (1)
ContractWriter(3-6)
sdk/src/client/signer.ts (2)
sdk/src/utils/state.ts (1)
getPackedState(10-35)sdk/src/client/types.ts (1)
State(83-85)
sdk/src/client/services/Erc20Service.ts (4)
sdk/src/client/contract_writer/types.ts (2)
ContractWriter(3-6)ContractCallParams(8-35)sdk/src/errors.ts (1)
Errors(372-399)sdk/src/client/contract_writer/eoa.ts (1)
EOAContractWriter(23-82)sdk/src/abis/token/index.ts (1)
Erc20Abi(6-34)
sdk/src/client/contract_writer/aa_v06.ts (5)
sdk/src/client/contract_writer/aa_v06_types.ts (2)
BundlerClientV06(26-30)PartialUserOperationV06(4-15)sdk/src/client/contract_writer/types.ts (3)
ContractWriter(3-6)CallsDetails(37-39)WriteResult(41-43)sdk/src/errors.ts (1)
Errors(372-399)sdk/src/abis/aa/user_op_event.ts (1)
UserOpEventAbi(3-51)sdk/src/abis/aa/execute.ts (1)
AAExecuteAbi(1-78)
sdk/src/rpc/api.ts (1)
sdk/src/rpc/types/index.ts (6)
PartialEIP712AuthMessage(132-137)EIP712AuthDomain(151-153)MessageSigner(93-93)RPCData(19-19)EIP712AuthMessage(142-145)EIP712AuthTypes(158-171)
sdk/src/client/services/NitroliteService.ts (3)
sdk/src/client/contract_writer/types.ts (2)
ContractWriter(3-6)ContractCallParams(8-35)sdk/src/client/contract_writer/eoa.ts (1)
EOAContractWriter(23-82)sdk/src/errors.ts (1)
Errors(372-399)
sdk/src/client/state.ts (1)
sdk/src/client/signer.ts (1)
AccountStateSigner(71-99)
sdk/src/client/contract_writer/eoa.ts (2)
sdk/src/client/contract_writer/types.ts (4)
ContractWriter(3-6)CallsDetails(37-39)WriteResult(41-43)ContractCallParams(8-35)sdk/src/errors.ts (1)
Errors(372-399)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (2)
- GitHub Check: Test (Clearnode) / Test Clearnode
- GitHub Check: Analyze (go)
🔇 Additional comments (15)
sdk/src/rpc/api.ts (1)
1-1: LGTM!The
Accounttype import is correctly added to support the newcreateEIP712AuthMessageSignerFromAccountfunction.sdk/src/client/contract_writer/types.ts (1)
3-43: LGTM!The type definitions are well-structured. The
ContractWriterinterface provides a clean abstraction, andContractCallParamsproperly leverages viem's type system with appropriate generics for ABI and chain flexibility.sdk/src/client/contract_writer/eoa.ts (2)
28-36: LGTM!Constructor properly validates required parameters and throws appropriate errors. This addresses the previous review feedback about missing validations.
38-54: LGTM!The
forEachwith async callback bug has been correctly fixed using afor...ofloop, ensuring sequential execution and proper awaiting of transactions.sdk/src/client/signer.ts (2)
65-99: LGTM!The
AccountStateSignerimplementation is well-structured with appropriate runtime guards for the optionalsignMessagemethod. The JSDoc documentation clearly explains the use case.
54-58: Minor formatting change looks good.sdk/src/client/index.ts (5)
64-76: LGTM!The constructor properly handles mutual exclusivity between
walletClientandcontractWriter, with clear error messaging when neither is provided.
78-85: Erc20Service receives contractWriter; approve flow should work.The services now receive
this.contractWriterwhich resolves the previous review concern aboutErc20Service.approve()failing. TheundefinedforwalletClientparameter is acceptable sincecontractWriteris provided.
454-460: LGTM!The helper correctly validates non-empty results and returns the last transaction hash, which is appropriate for batch operations where the final hash is most relevant.
107-128: LGTM!The
depositmethod properly accumulates calls (approval + deposit) and executes them through the contract writer. The pattern correctly handles the ERC20 approval check before deposit.
164-205: LGTM!The
depositAndCreateChannelmethod correctly batches approval and deposit+create operations, following the same pattern asdeposit.sdk/src/client/types.ts (1)
96-149: LGTM!The discriminated union pattern with
nevertypes ensures mutual exclusivity betweenwalletClientandcontractWriterat compile time. The deprecation notice on the legacy config is appropriate for guiding migration.sdk/src/client/services/Erc20Service.ts (1)
85-91: LGTM!The
ensureContractWritermethod properly guards write operations and throws the appropriate error type.sdk/src/client/services/NitroliteService.ts (2)
28-57: LGTM! Clean refactor to ContractWriter abstraction.The
executeWriteContractfunction correctly wraps the prepared request in a calls array and delegates to the contractWriter, extracting the last transaction hash from the batched result. This provides a clean abstraction over EOA and AA write paths.
99-105: Good defensive programming with ensureContractWriter guard.The
ensureContractWritermethod provides a clean validation point that throwsContractWriterRequiredErrorwhen no contract writer is available. This pattern is consistent withensureAccountand provides clear error messaging.
MaxMoskalenko
force-pushed
the
feat/aa-support
branch
from
0ad95ad to
4b82e5b
Compare
There was a problem hiding this comment.
Actionable comments posted: 2
♻️ Duplicate comments (4)
sdk/src/client/services/Erc20Service.ts (1)
152-163: Address the TODO comment for documentation.The
prepareApproveCallParamsmethod is missing documentation as noted by the TODO. This was flagged in a previous review.sdk/src/client/contract_writer/aa_v06.ts (1)
77-91: Avoid@ts-ignoreby properly shaping the RpcUserOperation inputThe
@ts-ignorehere still hides the mismatch betweenPartialUserOperationV06(bigint fields) andRpcUserOperation<'0.6'>(hex string fields). This makes the code harder to reason about and refactor.Consider explicitly constructing a
RpcUserOperation<'0.6'>object by converting all bigint fields (from bothpartialUserOperationandgasParameters) to hex strings withnumberToHexbefore passing it into_formatUserOperation, and then drop the@ts-ignore.sdk/src/client/services/NitroliteService.ts (2)
67-97: Constructor: resolvewalletClienttyping instead of@ts-ignore, and consider account fallback fromcontractWriterTwo related concerns here:
@ts-ignoreonwalletClientto buildEOAContractWriterYou still have:
this.contractWriter = new EOAContractWriter({ publicClient, // @ts-ignore walletClient, });which suppresses a real type mismatch between the bare
WalletClientparameter and the more specific generic expected byEOAContractWriter. Prefer aligning the types instead of ignoring them – e.g. by tightening the constructor signature to the sameWalletClient<Transport, Chain, ParseAccount<Account>>shape used inEOAContractWriterConfig, or by relaxingEOAContractWriterConfigif intentional.Using
contractWriterwithout an explicitaccountWhen only
contractWriteris provided (nowalletClientand noaccount),this.accountremainsundefined, andensureAccount()will throw even though theContractWriteritself knows its account (getAccount()). If you intend to support “AA‑only” or “writer‑only” usage, consider:
- Either requiring
accountexplicitly when acontractWriteris passed, or- Falling back to
this.contractWriter.getAccount()insideensureAccount()whenthis.accountis not set.This would make the service easier to consume in AA scenarios.
305-663: Document the newprepare*CallParamshelpers (existing TODO) instead of leaving placeholdersSeveral new helpers are introduced with
// TODO: comment:
prepareDepositCallParamsprepareCreateChannelCallParamsprepareDepositAndCreateChannelCallParamsprepareJoinChannelCallParamsprepareCheckpointCallParamsprepareChallengeCallParamsprepareResizeCallParamsprepareCloseCallParamsprepareWithdrawCallParamsThese are part of the public-ish surface used to build AA/EOA call params, and they parallel the already well‑documented
prepareXmethods below them. It would be good to replace the TODOs with brief JSDoc (purpose, parameters, return type) for consistency and discoverability.
🧹 Nitpick comments (9)
sdk/src/rpc/api.ts (1)
832-884: Consider extracting shared logic to reduce duplication withcreateEIP712AuthMessageSigner.This new function duplicates ~90% of the logic from
createEIP712AuthMessageSigner(lines 776-830). The only difference is the signing mechanism (account.signTypedDatavswalletClient.signTypedData).Consider extracting the shared validation and message construction logic:
function _prepareEIP712AuthMessage( address: Address, payload: RPCData, partialMessage: PartialEIP712AuthMessage, ): EIP712AuthMessage { const method = payload[1]; if (method !== RPCMethod.AuthVerify) { throw new Error(`This EIP-712 signer is designed only for the '${RPCMethod.AuthVerify}' method, but received '${method}'.`); } const params = payload[2]; if (!('challenge' in params) || typeof params.challenge !== 'string') { throw new Error('Invalid payload for AuthVerify: The challenge string is missing or malformed.'); } return { ...partialMessage, challenge: params.challenge, wallet: address, }; }Then both signers can call this helper. This would reduce maintenance burden and ensure consistent behavior.
sdk/test/unit/client/index.test.ts (1)
13-18: Variable ordering relies on closure semantics - consider reordering for clarity.
mockSignMessage(line 13) referencesmockSignature, which is defined later (line 18). This works because the closure capturesmockSignatureby reference, and the mock function isn't called until after all declarations complete. However, this ordering can be confusing to readers:+const mockSignature = '0x' + '1234567890abcdef'.repeat(8) + '1b'; const mockSignMessage = jest.fn(() => Promise.resolve(mockSignature)); const mockAccount = { address: '0x1234567890123456789012345678901234567890' as Address, signMessage: mockSignMessage, }; -const mockSignature = '0x' + '1234567890abcdef'.repeat(8) + '1b';sdk/src/client/contract_writer/types.ts (1)
26-34: Clarify or remove commented-out code.The commented-out
gasPriceand transaction types (legacy,eip2930,eip1559) should either be removed or documented with the rationale for exclusion. If onlyeip7702is intentionally supported, add a comment explaining why.// Transaction parameters value?: bigint; gas?: bigint; - // gasPrice?: bigint; maxFeePerGas?: bigint; maxPriorityFeePerGas?: bigint; nonce?: number; // Additional optional parameters dataSuffix?: `0x${string}`; - // type?: 'legacy' | 'eip2930' | 'eip1559'; + /** Currently only EIP-7702 delegated accounts are supported for AA flows */ type?: "eip7702";sdk/src/client/index.ts (1)
454-460: Consider using a specific error type.The helper correctly validates and extracts the last transaction hash. For consistency with the SDK's error handling patterns, consider using a specific error type instead of the generic
Error.private _getLastTxHashFromWriteResult(writeResult: WriteResult): Hash { if (writeResult.txHashes.length < 1) { - throw new Error('No transaction hashes returned from write operation'); + throw new Errors.ContractCallError('No transaction hashes returned from write operation'); } return writeResult.txHashes[writeResult.txHashes.length - 1]; }sdk/src/client/contract_writer/eoa.ts (2)
60-70: Document rationale for type assertion.The
as anycast on line 69 suppresses type checking. While this may be necessary due to viem's complex union types, adding a brief comment explaining why would help maintainability. Also consider wrapping errors fromsimulateContractandwriteContractin SDK-specific error types.private async _writeCall(callParams: ContractCallParams): Promise<Hex> { const { request } = await this.publicClient.simulateContract({ ...callParams, account: this.account, }); + // Type assertion needed due to viem's complex union types for transaction parameters. + // The runtime behavior is correct as simulateContract returns compatible parameters. return this.walletClient.writeContract({ ...request, account: this.account, } as any); }
72-80: Consider more specific error handling for reverted transactions.The current error message
Transaction revertedis generic. Consider including the transaction hash in the error for debugging purposes, and using an SDK-specific error type.async waitForTransaction(hash: Hash): Promise<TransactionReceipt> { const receipt = await this.publicClient.waitForTransactionReceipt({ hash }); if (receipt.status === 'reverted') { - throw new Error(`Transaction reverted`); + throw new Errors.TransactionError('Transaction reverted', undefined, { hash }); } return receipt; }sdk/src/client/services/Erc20Service.ts (1)
34-50: Duplicate hash extraction logic.The pattern of extracting the last transaction hash from
WriteResultis duplicated here and inNitroliteClient._getLastTxHashFromWriteResult. Consider extracting this to a shared utility function.+// In a shared utility file or as part of WriteResult +function getLastTxHash(result: WriteResult): Hash { + if (result.txHashes.length < 1) { + throw new Error('No transaction hashes returned from write operation'); + } + return result.txHashes[result.txHashes.length - 1]; +}sdk/src/client/contract_writer/aa_v06.ts (1)
136-153: Use validatedchainIdvariable and remove remaining non‑null assertionYou already guard
this.publicClient.chainand capturechainId:const chainId = this.publicClient.chain?.id; if (!chainId) { ... }but then you still call:
const logs = await this.bundlerClient.fetchLogs( this.publicClient.chain!.id, [entryPoint06Address], [[toEventSelector(UserOpEventAbi)], [userOpHash]], );to pass the chain ID. It’s clearer and safer to reuse the validated
chainIdvariable and drop the!:- const logs = await this.bundlerClient.fetchLogs( - this.publicClient.chain!.id, + const logs = await this.bundlerClient.fetchLogs( + chainId, [entryPoint06Address], [[toEventSelector(UserOpEventAbi)], [userOpHash]], );This keeps the runtime check and removes an unnecessary non-null assertion.
sdk/src/client/services/NitroliteService.ts (1)
28-57: UpdateexecuteWriteContractto reflect ContractWriter usage and avoid double simulationThis helper now:
- Accepts a
PreparedContractRequest(fromsimulateContract),- Wraps it in a
callsarray withaccount,- Invokes
contractWriter.write({ calls }),- Then returns the last
txHash.Two points:
- The JSDoc still describes this as a low-level wrapper around
writeContractandsimulateContract, but it no longer toucheswalletClientdirectly. The comment should be updated to describe it as a generic helper that adapts a prepared request into aContractWritercall and returns the last tx hash.- For
EOAContractWriter, this implies simulating twice: once inprepareXand again inside_writeCall. If you want to avoid the extra RPC round‑trip, consider either:
- Passing
ContractCallParamsdirectly tocontractWriter.write(using theprepareXCallParamshelpers instead ofprepareX), or- Adding a dedicated path for writers that can accept a pre‑simulated request vs. those that expect raw call params.
Neither blocks correctness, but clarifying the intent and docs will make the abstraction easier to maintain.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (14)
sdk/src/client/contract_writer/aa_v06.ts(1 hunks)sdk/src/client/contract_writer/eoa.ts(1 hunks)sdk/src/client/contract_writer/types.ts(1 hunks)sdk/src/client/index.ts(14 hunks)sdk/src/client/prepare.ts(0 hunks)sdk/src/client/services/Erc20Service.ts(7 hunks)sdk/src/client/services/NitroliteService.ts(25 hunks)sdk/src/client/signer.ts(2 hunks)sdk/src/client/state.ts(3 hunks)sdk/src/client/types.ts(3 hunks)sdk/src/rpc/api.ts(2 hunks)sdk/test/unit/client/index.test.ts(14 hunks)sdk/test/unit/client/services/Erc20Service.test.ts(2 hunks)sdk/test/unit/client/state.test.ts(4 hunks)
💤 Files with no reviewable changes (1)
- sdk/src/client/prepare.ts
🚧 Files skipped from review as they are similar to previous changes (1)
- sdk/src/client/signer.ts
🧰 Additional context used
🧠 Learnings (1)
📚 Learning: 2025-11-05T10:19:06.155Z
Learnt from: dimast-x
Repo: erc7824/nitrolite PR: 411
File: clearnode/custody.go:449-465
Timestamp: 2025-11-05T10:19:06.155Z
Learning: In clearnode/custody.go's handleResized function during positive resize operations (deposits), the ledger flow is: on-chain funds first credit the channel account, are immediately debited from the channel account, then credit the unified wallet account. The channel account acts as a pass-through for deposits. The unified wallet account is where actual deposited funds are held. This is recorded as a deposit transaction from channelAccountID to walletAccountID.
Applied to files:
sdk/src/client/services/NitroliteService.ts
🧬 Code graph analysis (10)
sdk/src/client/index.ts (3)
sdk/src/client/contract_writer/types.ts (3)
ContractWriter(3-6)CallsDetails(37-39)WriteResult(41-43)sdk/src/client/contract_writer/eoa.ts (1)
EOAContractWriter(23-81)sdk/src/client/services/Erc20Service.ts (1)
Erc20Service(56-213)
sdk/src/client/contract_writer/eoa.ts (2)
sdk/src/client/contract_writer/types.ts (4)
ContractWriter(3-6)CallsDetails(37-39)WriteResult(41-43)ContractCallParams(8-35)sdk/src/errors.ts (1)
Errors(372-399)
sdk/test/unit/client/index.test.ts (1)
sdk/src/client/index.ts (1)
NitroliteClient(34-461)
sdk/src/client/contract_writer/aa_v06.ts (4)
sdk/src/client/contract_writer/aa_v06_types.ts (2)
BundlerClientV06(26-30)PartialUserOperationV06(4-15)sdk/src/errors.ts (1)
Errors(372-399)sdk/src/abis/aa/user_op_event.ts (1)
UserOpEventAbi(3-51)sdk/src/abis/aa/execute.ts (1)
AAExecuteAbi(1-78)
sdk/src/client/types.ts (1)
sdk/src/client/contract_writer/types.ts (1)
ContractWriter(3-6)
sdk/test/unit/client/state.test.ts (1)
clearnode/pkg/rpc/api.go (1)
StateIntent(767-767)
sdk/src/rpc/api.ts (1)
sdk/src/rpc/types/index.ts (6)
PartialEIP712AuthMessage(132-137)EIP712AuthDomain(151-153)MessageSigner(93-93)RPCData(19-19)EIP712AuthMessage(142-145)EIP712AuthTypes(158-171)
sdk/test/unit/client/services/Erc20Service.test.ts (2)
sdk/src/client/services/Erc20Service.ts (1)
Erc20Service(56-213)sdk/src/errors.ts (1)
Errors(372-399)
sdk/src/client/state.ts (1)
sdk/src/client/signer.ts (1)
AccountStateSigner(71-99)
sdk/src/client/services/NitroliteService.ts (3)
sdk/src/client/contract_writer/types.ts (2)
ContractWriter(3-6)ContractCallParams(8-35)sdk/src/client/contract_writer/eoa.ts (1)
EOAContractWriter(23-81)sdk/src/errors.ts (1)
Errors(372-399)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (3)
- GitHub Check: Build and Publish (Clearnode)
- GitHub Check: Test (Integration) / Test Integration
- GitHub Check: Analyze (go)
🔇 Additional comments (27)
sdk/src/rpc/api.ts (1)
1-1: LGTM!The
Accountimport is correctly added to support the newcreateEIP712AuthMessageSignerFromAccountfunction.sdk/src/client/state.ts (2)
200-206: LGTM! Signer selection now correctly uses Account-based comparison.The refactor to compare against
deps.account.addressand instantiateAccountStateSignerwhen matched aligns well with the broader ContractWriter/AA support changes. This allows proper signer resolution for both EOA and AA wallet flows.
5-5: LGTM!The
AccountStateSignerimport is correctly added to support the new signer instantiation path.sdk/test/unit/client/services/Erc20Service.test.ts (2)
20-20: LGTM!The
waitForTransactionReceiptmock is correctly added to support theEOAContractWriterflow that theErc20Servicenow uses internally when initialized with awalletClient.
124-127: LGTM!The test correctly reflects the error type change from
WalletClientRequiredErrortoContractWriterRequiredError, aligning with theErc20Servicerefactor that now validatesContractWriteravailability instead ofWalletClientdirectly.sdk/test/unit/client/state.test.ts (2)
37-55: Address mismatch betweenaccountandparticipants[0]may cause confusion.The test setup has:
deps.account.address = '0xOWNER'(line 38)deps.walletClient.account.address = '0xWALLET'(line 43)defaultChannel.participants[0] = '0xWALLET'(line 55)In
_checkParticipantAndGetSigner, the comparison isparticipant == deps.account.address, which evaluates to'0xWALLET' == '0xOWNER'(false). This causes the function to returndeps.stateSignerinstead of creating a newAccountStateSigner.If the intent is to test the
AccountStateSignerpath, consider aligning the addresses:defaultChannel = { - participants: ['0xWALLET', guestAddress], + participants: ['0xOWNER', guestAddress], adjudicator: adjudicatorAddress,If the current setup intentionally tests the fallback path, a clarifying comment would help future maintainers understand this design choice.
96-102: LGTM!The reformatted assertion maintains the same test logic with improved readability.
sdk/test/unit/client/index.test.ts (5)
44-47: LGTM!The
mockContractWritersetup correctly mocks theContractWriterinterface, returning aWriteResultwithtxHashesarray that aligns with the implementation inNitroliteClient._getLastTxHashFromWriteResult.
56-74: LGTM!The mock service methods are correctly updated to use the
prepare*CallParamspattern. The test structure properly validates that:
prepare*CallParamsmethods are called with correct arguments- Results are collected into the
callsarraycontractWriter.writereceives the expected call structure
123-137: Verify the intended error handling behavior for deposit failures.The test expectations changed from specific error types (
TokenError,ContractCallError) to genericError. Looking at theNitroliteClient.depositimplementation, it doesn't wrap errors in custom error types—it simply re-throws whatevercontractWriter.writethrows.If this is the intended behavior (letting errors propagate unwrapped from deposit), the tests are correct. However, if deposit should wrap errors similar to other operations (e.g.,
closeChannelwraps inContractCallError), consider adding error wrapping:// In NitroliteClient.deposit try { // ... existing code } catch (err) { throw new Errors.ContractCallError('Failed to execute deposit on contract', err as Error); }
168-188: LGTM!The
createChanneltest correctly validates the new flow:
_prepareAndSignInitialStateis called with paramsprepareCreateChannelCallParamsreceives channel and signed initial statecontractWriter.writeis called with the prepared call- Result includes the expected
channelId,initialState, andtxHash
384-398: LGTM!The
withdrawaltests correctly verify theprepareWithdrawCallParams→contractWriter.writeflow, including proper error wrapping withContractCallErroron failure.sdk/src/client/contract_writer/types.ts (1)
1-6: Well-designed abstraction for contract writing.The
ContractWriterinterface provides a clean abstraction that decouples the SDK from specific wallet implementations, enabling both EOA and AA support. The batchedCallsDetailsapproach is appropriate for account abstraction use cases.sdk/src/client/index.ts (3)
64-76: Constructor validation logic is correct.The mutual exclusivity check between
walletClientandcontractWriteris properly implemented. The'walletClient' in configpattern ensures type narrowing works correctly with the union type.
78-85: Service initialization correctly passes contractWriter.Both
NitroliteServiceandErc20Serviceare initialized withundefinedforwalletClientsincecontractWriteris passed directly. This aligns with the service constructors that prioritizecontractWriterwhen provided.
107-128: Batched deposit flow is well-implemented.The conditional approval followed by deposit properly batches calls. For EOA, these execute sequentially; for AA, they can be bundled into a single UserOperation. Returning the last hash (deposit tx) is appropriate.
sdk/src/client/types.ts (1)
96-149: Well-structured discriminated union for configuration.The type design properly enforces mutual exclusivity between
walletClientandcontractWriterusing thenevertype. The deprecation notice on the legacy configuration provides clear migration guidance. This pattern enables proper type narrowing in the constructor.sdk/src/client/contract_writer/eoa.ts (2)
28-36: Constructor properly validates required parameters.The validation for
publicClient,walletClient, andwalletClient.accountprevents runtime errors. This addresses the earlier review feedback about missing constructor validations.
38-54: Sequential execution correctly implemented with for...of.The previous
forEachasync bug has been fixed. Thefor...ofloop properly awaits each transaction before proceeding to the next, ensuring correct sequential execution and populatedtxHashesarray.sdk/src/client/services/Erc20Service.ts (2)
85-91: Guard method correctly throws SDK-specific error.The
ensureContractWritermethod properly usesContractWriterRequiredErrorinstead of the previousWalletClientRequiredError, maintaining consistency with the new abstraction.
201-212: Approve flow correctly migrated to contractWriter.The
approvemethod now usesensureContractWriter()and delegates toexecuteWriteContractwith the contractWriter. This aligns with the PR's goal of abstracting wallet interactions.sdk/src/client/contract_writer/aa_v06.ts (2)
63-75:_prepareCalldatanow correctly forwardsvaluefor ETH transfersUsing
callParams.value ?? 0nensures value-carrying calls (e.g. ETH deposits) are no longer silently zeroed while still defaulting cleanly when value is omitted. This fixes the earlier broken ETH-transfer behavior.
1-20: The review comment is incorrect. Theerrors.tsfile exports both a named export (export const Errors) and a default export (export default Errors), which means both default and named imports are valid:
import Errors from '../../errors'(default) ✓import { Errors } from '../../errors'(named) ✓The file
aa_v06.tscorrectly uses the default import and will not fail at compile time or runtime. While there is inconsistency in import patterns across the codebase (some files use default, others use named, and some use namespace imports), this is not a compilation error or runtime issue—only a style inconsistency that may warrant standardization if desired.sdk/src/client/services/NitroliteService.ts (4)
99-105:ensureContractWriterguard is appropriate for new abstractionThe new guard cleanly enforces that a
ContractWriteris configured before any write operation proceeds, and uses the dedicatedContractWriterRequiredError. This is consistent with the rest of the error model.
187-200: Deposit call params look correct and propagate ETHvalue
prepareDepositCallParamscorrectly:
- Derives
accountAddressfromAccount | Address,- Uses
this.custodyAddressandcustodyAbi,- Passes
[accountAddress, tokenAddress, amount]as args,- Sets
valuetoamountonly whentokenAddress === zeroAddress.This matches the custody deposit expectations and ensures ETH deposits carry the intended value.
232-382: Write methods correctly route throughContractWriterwhile preserving error semanticsFor
deposit,createChannel, anddepositAndCreateChannel, the flow:
- Ensures a
ContractWriterandAccount,- Uses the corresponding
prepareXmethod to simulate and get arequest,- Delegates execution to
executeWriteContract(contractWriter, request, account),- Wraps non‑Nitrolite errors in
TransactionError,preserves previous behavior but now abstracts over EOA vs AA write paths and still returns a single transaction hash (last in batch). This looks consistent across these methods.
384-766: Join / checkpoint / challenge / resize / close / withdraw: consistent ContractWriter integrationAll remaining write methods (
joinChannel,checkpoint,challenge,resize,close,withdraw) follow the same pattern:
- Ensure
contractWriterandaccount,- Build call params via
prepare*CallParams→prepare*→simulateContract,- Execute via
executeWriteContract,- Map errors into the appropriate
TransactionError/ContractCallErrorflavors.The argument ordering and ABI conversions (
convertStateForABI,convertChannelForABI) remain unchanged, so the behavioral surface should be preserved while gaining the new writer abstraction.
| private _formatAuthorization = (authorization: SignedAuthorization) => { | ||
| return { | ||
| address: authorization.address, | ||
| chainId: numberToHex(authorization.chainId), | ||
| nonce: numberToHex(authorization.nonce), | ||
| r: authorization.r ? numberToHex(BigInt(authorization.r), { size: 32 }) : pad('0x', { size: 32 }), | ||
| s: authorization.s ? numberToHex(BigInt(authorization.s), { size: 32 }) : pad('0x', { size: 32 }), | ||
| yParity: authorization.yParity ? numberToHex(authorization.yParity, { size: 1 }) : pad('0x', { size: 32 }), | ||
| }; |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
find . -type f -name "aa_v06.ts" | head -20Repository: erc7824/nitrolite
Length of output: 103
🏁 Script executed:
git ls-files sdk/src/client/contract_writer/ | grep -E "\.(ts|tsx)$"Repository: erc7824/nitrolite
Length of output: 226
🏁 Script executed:
wc -l sdk/src/client/contract_writer/aa_v06.tsRepository: erc7824/nitrolite
Length of output: 105
🏁 Script executed:
sed -n '240,265p' sdk/src/client/contract_writer/aa_v06.tsRepository: erc7824/nitrolite
Length of output: 1101
🏁 Script executed:
cat -n sdk/src/client/contract_writer/aa_v06_types.tsRepository: erc7824/nitrolite
Length of output: 1242
🏁 Script executed:
head -50 sdk/src/client/contract_writer/aa_v06.tsRepository: erc7824/nitrolite
Length of output: 1817
🏁 Script executed:
rg "SignedAuthorization" sdk/src/client/contract_writer/Repository: erc7824/nitrolite
Length of output: 244
🏁 Script executed:
rg "yParity" sdk/src --type ts --type tsx -B 3 -A 3Repository: erc7824/nitrolite
Length of output: 88
🏁 Script executed:
rg "SignedAuthorization|authorization.*yParity" . --type ts --type tsx -B 2 -A 2Repository: erc7824/nitrolite
Length of output: 88
🏁 Script executed:
find . -type f -name "*.test.ts" -o -name "*.spec.ts" | xargs rg -l "aa_v06|yParity|formatAuthorization" 2>/dev/null | head -5Repository: erc7824/nitrolite
Length of output: 43
🏁 Script executed:
rg "yParity" sdk/src -B 3 -A 3Repository: erc7824/nitrolite
Length of output: 762
🏁 Script executed:
rg "authorization" sdk/src -B 2 -A 2 | head -100Repository: erc7824/nitrolite
Length of output: 3928
🏁 Script executed:
rg "0x00|0x01" sdk/src/client/contract_writer/ -B 2 -A 2 | grep -i "parity\|auth" | head -20Repository: erc7824/nitrolite
Length of output: 43
🌐 Web query:
EIP-7702 yParity value 0 valid
💡 Result:
Yes — y_parity = 0 is valid. EIP‑7702 accepts any 1‑byte parity (auth.y_parity < 2**8) and uses it directly in ecrecover (authority = ecrecover(msg, y_parity, r, s)). [1][2]
Sources:
- EIP‑7702 specification (authorization tuple, bounds and ecrecover step). [1]
- Ethereum.org Pectra/7702 summary (authorization tuple uses y_parity). [2]
🌐 Web query:
viem SignedAuthorization yParity type
💡 Result:
Short answer: yParity is the signature "y parity" (the ECDSA recovery parity / v bit) represented as 0 or 1. Viem uses yParity (0 | 1) in SignedAuthorization/signature objects to carry the recovery parity (instead of v=27/28 or alongside it). [1][2][3]
Sources:
- viem parseSignature / signature handling (shows yParity logic and accepted values 0/1). [1]
- viem signAuthorization / docs (SignedAuthorization examples include yParity: 0). [2]
- EIP-7702 spec (authorization tuple includes signature_y_parity). [3]
🏁 Script executed:
rg "pad.*size.*32" sdk/src/client/contract_writer/aa_v06.ts -B 1 -A 1Repository: erc7824/nitrolite
Length of output: 466
Fix yParity encoding: handle 0 correctly and pad to 1 byte
Two issues in _formatAuthorization:
authorization.yParity ? ... : ...treats0(a valid yParity value per EIP-7702) as falsy, soyParity = 0incorrectly triggers the fallback path.- The fallback pads to 32 bytes, while the truthy branch uses 1 byte—breaking the expected encoding format.
Suggested fix:
return {
address: authorization.address,
chainId: numberToHex(authorization.chainId),
nonce: numberToHex(authorization.nonce),
r: authorization.r ? numberToHex(BigInt(authorization.r), { size: 32 }) : pad('0x', { size: 32 }),
s: authorization.s ? numberToHex(BigInt(authorization.s), { size: 32 }) : pad('0x', { size: 32 }),
- yParity: authorization.yParity ? numberToHex(authorization.yParity, { size: 1 }) : pad('0x', { size: 32 }),
+ yParity:
+ authorization.yParity !== undefined
+ ? numberToHex(authorization.yParity, { size: 1 })
+ : pad('0x', { size: 1 }),
};This preserves 0 as a valid value (0x00) and keeps padding consistent at 1 byte.
🤖 Prompt for AI Agents
sdk/src/client/contract_writer/aa_v06.ts around lines 249 to 257: yParity is
treated as falsy so a value of 0 falls back to the wrong branch and the fallback
pads to 32 bytes while the true branch uses 1 byte; change the check to
explicitly test for undefined/null (so 0 is accepted), convert yParity to a
BigInt when present and call numberToHex(..., { size: 1 }), and change the
fallback pad size to 1 byte to match the expected encoding.
There was a problem hiding this comment.
Actionable comments posted: 2
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
integration/tests/challenge_channel.test.ts(1 hunks)sdk/src/client/services/Erc20Service.ts(7 hunks)sdk/src/client/services/NitroliteService.ts(25 hunks)
🧰 Additional context used
🧠 Learnings (1)
📚 Learning: 2025-11-05T10:19:06.155Z
Learnt from: dimast-x
Repo: erc7824/nitrolite PR: 411
File: clearnode/custody.go:449-465
Timestamp: 2025-11-05T10:19:06.155Z
Learning: In clearnode/custody.go's handleResized function during positive resize operations (deposits), the ledger flow is: on-chain funds first credit the channel account, are immediately debited from the channel account, then credit the unified wallet account. The channel account acts as a pass-through for deposits. The unified wallet account is where actual deposited funds are held. This is recorded as a deposit transaction from channelAccountID to walletAccountID.
Applied to files:
sdk/src/client/services/NitroliteService.ts
🧬 Code graph analysis (2)
sdk/src/client/services/Erc20Service.ts (4)
sdk/src/client/contract_writer/types.ts (2)
ContractWriter(3-6)ContractCallParams(8-35)sdk/src/errors.ts (1)
Errors(372-399)sdk/src/client/contract_writer/eoa.ts (1)
EOAContractWriter(23-81)sdk/src/abis/token/index.ts (1)
Erc20Abi(6-34)
sdk/src/client/services/NitroliteService.ts (4)
sdk/src/client/contract_writer/types.ts (2)
ContractWriter(3-6)ContractCallParams(8-35)sdk/src/client/contract_writer/eoa.ts (1)
EOAContractWriter(23-81)sdk/src/errors.ts (1)
Errors(372-399)sdk/src/abis/generated.ts (1)
custodyAbi(2-1567)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (3)
- GitHub Check: Build and Publish (Clearnode)
- GitHub Check: Test (Integration) / Test Integration
- GitHub Check: Analyze (go)
🔇 Additional comments (8)
sdk/src/client/services/Erc20Service.ts (3)
21-50: LGTM! Clean ContractWriter integration.The refactored
executeWriteContractcorrectly wraps the prepared request in a calls array, invokescontractWriter.write(), and returns the last transaction hash from the result. This design properly supports both single and batched write operations.
59-92: Constructor initialization logic is sound.The constructor properly initializes the
contractWriterby either accepting it directly or wrapping an existingwalletClientwithEOAContractWriter. The fallback pattern ensures backward compatibility while supporting the new abstraction.
160-170: Well-structured helper for batched operations.The
prepareApproveCallParamsmethod cleanly encapsulates the preparation of contract call parameters, making it easy to batch approve operations. The implementation correctly usesensureAccount()and returns a properly structuredContractCallParamsobject.sdk/src/client/services/NitroliteService.ts (4)
28-57: Excellent consistency with Erc20Service pattern.The
executeWriteContractimplementation mirrors the pattern in Erc20Service, ensuring consistency across services. The batched write flow with last-txHash extraction is well-designed.
84-105: Solid initialization and guard patterns.The constructor's conditional initialization of
contractWriterand theensureContractWriter()guard method provide a clean, safe interface for write operations. The pattern aligns well with the Erc20Service implementation.
194-206: Comprehensive and consistent prepareXCallParams helpers.The suite of
prepareXCallParamsmethods (deposit, createChannel, depositAndCreateChannel, joinChannel, checkpoint, challenge, resize, close, withdraw) provides a uniform interface for building contract call parameters. Each method correctly:
- Calls
ensureAccount()for validation- Converts types to ABI-compatible formats where needed
- Returns properly structured
ContractCallParamsThis design enables clean batching of operations via the ContractWriter abstraction.
Also applies to: 259-271, 326-344, 412-422, 478-490, 551-568, 635-647, 707-719, 774-784
238-250: All write operations properly migrated to ContractWriter.All write methods (deposit, createChannel, depositAndCreateChannel, joinChannel, checkpoint, challenge, resize, close, withdraw) have been consistently updated to:
- Obtain a
contractWriterviaensureContractWriter()- Prepare the request using the corresponding
prepareXmethod- Execute via
executeWriteContract(contractWriter, request, account)- Maintain proper error handling
The refactoring is thorough and maintains consistent patterns across the service.
Also applies to: 303-315, 385-402, 456-468, 528-540, 608-625, 685-697, 753-765, 816-828
integration/tests/challenge_channel.test.ts (1)
69-73: > Likely an incorrect or invalid review comment.
MaxMoskalenko
force-pushed
the
feat/aa-support
branch
from
a4a3db6 to
9ae33ad
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (3)
sdk/src/client/services/Erc20Service.ts (1)
1-50: Fix JSDoc drift:executeWriteContractdocs still describewalletClient+writeContract.
The function now takescontractWriterand callscontractWriter.write().sdk/src/client/services/NitroliteService.ts (2)
69-97: DeriveaccountfromcontractWriter.getAccount()when writer is injected (AA path can currently break).Same issue as in
Erc20Service: injectingcontractWriterwithout an explicitaccountwill make allprepare*/*methods throwAccountRequiredError.- if (contractWriter) { - this.contractWriter = contractWriter; - } else if (walletClient) { - this.contractWriter = new EOAContractWriter({ - publicClient, - // @ts-ignore - walletClient, - }); - } + let writer = contractWriter; + if (!writer && walletClient) { + writer = new EOAContractWriter({ + publicClient, + walletClient: walletClient as any, + }); + } + this.contractWriter = writer; this.publicClient = publicClient; - this.account = account || walletClient?.account; + this.account = account ?? writer?.getAccount?.() ?? walletClient?.account; this.addresses = addresses;
28-57: Fix JSDoc drift:executeWriteContractdocs still mentionwalletClient+writeContract.
♻️ Duplicate comments (3)
sdk/src/client/services/Erc20Service.ts (1)
71-79: Remove@ts-ignore/fix typo and typewalletClientcorrectly (suppression hides a real mismatch).At minimum, Line 76 should be
@ts-ignore(singular). Prefer removing suppression entirely by typing the ctor param to matchEOAContractWriter’s expectedWalletClient<Transport, Chain, ParseAccount<Account>>(or wideningEOAContractWriterconfig if that’s the intended API).sdk/src/client/services/NitroliteService.ts (2)
1-7: Remove@ts-ignoreand alignWalletClientgenerics withEOAContractWriter(suppression hides a real mismatch).Either type the ctor’s
walletClientparam to the specific generic form expected byEOAContractWriter, or widenEOAContractWriter’s config type so callers don’t need suppression.Also applies to: 84-92
1-1: Drop unusedContractFunctionParametersimport (if still unused).It’s imported on Line 1 but not referenced in the shown file.
🧹 Nitpick comments (2)
sdk/src/client/services/Erc20Service.ts (2)
21-50: Avoid double-simulating for direct writes (service simulates, thenEOAContractWritersimulates again).
Forapprove(), you dosimulateContract()in the service, thenEOAContractWriter._writeCall()simulates again; that’s extra RPC latency and can diverge if defaults change.Consider changing the “direct write” path to pass
ContractCallParamsintocontractWriter.write()(and keepprepareApprove()for users who explicitly wantrequest), e.g.:const params = this.prepareApproveCallParams(...) await contractWriter.write({ calls: [params] })(If you truly need to execute prepared
requests, consider extendingContractWriterwith a separate method for that, to keep types honest.)Also applies to: 101-219
10-10: Prefer a real viem type forPreparedContractRequestinstead ofany(improves safety with AA + batching).If viable, replace
anywithSimulateContractReturnType['request'](or an extracted alias) to reduce accidental shape drift across viem upgrades.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
integration/tests/challenge_channel.test.ts(2 hunks)sdk/src/client/services/Erc20Service.ts(7 hunks)sdk/src/client/services/NitroliteService.ts(25 hunks)
🧰 Additional context used
🧠 Learnings (1)
📚 Learning: 2025-11-05T10:19:06.155Z
Learnt from: dimast-x
Repo: erc7824/nitrolite PR: 411
File: clearnode/custody.go:449-465
Timestamp: 2025-11-05T10:19:06.155Z
Learning: In clearnode/custody.go's handleResized function during positive resize operations (deposits), the ledger flow is: on-chain funds first credit the channel account, are immediately debited from the channel account, then credit the unified wallet account. The channel account acts as a pass-through for deposits. The unified wallet account is where actual deposited funds are held. This is recorded as a deposit transaction from channelAccountID to walletAccountID.
Applied to files:
sdk/src/client/services/NitroliteService.ts
🧬 Code graph analysis (3)
sdk/src/client/services/Erc20Service.ts (3)
sdk/src/client/contract_writer/types.ts (2)
ContractWriter(3-6)ContractCallParams(8-35)sdk/src/errors.ts (1)
Errors(372-399)sdk/src/abis/token/index.ts (1)
Erc20Abi(6-34)
sdk/src/client/services/NitroliteService.ts (5)
sdk/src/client/contract_writer/types.ts (2)
ContractWriter(3-6)ContractCallParams(8-35)sdk/src/client/contract_writer/eoa.ts (1)
EOAContractWriter(23-81)sdk/src/errors.ts (1)
Errors(372-399)sdk/src/abis/generated.ts (1)
custodyAbi(2-1567)sdk/src/abis/custody/index.ts (1)
custodyAbi(1-1)
integration/tests/challenge_channel.test.ts (1)
integration/common/ws.ts (1)
getChannelUpdatePredicateWithStatus(181-191)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (3)
- GitHub Check: Build and Publish (Clearnode)
- GitHub Check: Test (Integration) / Test Integration
- GitHub Check: Analyze (go)
🔇 Additional comments (3)
integration/tests/challenge_channel.test.ts (2)
8-8: LGTM! Import formatting consolidated.The multi-line import was consolidated to a single line with no functional change.
53-57: Good race condition fix! Setting up the listener before triggering the action.Creating the promise before calling
challengeChannelensures the message listener is registered before the action that triggers the message, preventing a race condition where the message could arrive before the listener is set up.Note: The 5000ms timeout starts when the promise is created, not when it's awaited on line 75. If
waitForTransaction(line 72) takes longer than 5000ms, the promise could timeout. However, on test networks, transaction confirmations are typically very fast, so this should not be an issue in practice.sdk/src/client/services/NitroliteService.ts (1)
187-206:prepare*CallParamshelpers look consistent and well-documented.One thing to confirm:
prepareDepositAndCreateChannelCallParams()passesargs: [tokenAddress, amount, abiChannel, abiState]whiledepositincludes an explicitaccountAddressin args—please verify this matches thecustodyAbisignature fordepositAndCreate(i.e., whether it expects a wallet/recipient param too).Also applies to: 252-271, 317-344, 404-422, 470-490, 542-568, 627-647, 699-719, 767-784
MaxMoskalenko
force-pushed
the
feat/aa-support
branch
from
9ae33ad to
4f121ac
Compare
There was a problem hiding this comment.
Actionable comments posted: 3
♻️ Duplicate comments (2)
sdk/src/client/services/NitroliteService.ts (1)
1-1: Remove unused ContractFunctionParameters import.The ContractFunctionParameters type from 'viem' is imported but not used anywhere in the file.
🔎 Apply this diff:
-import { Account, Address, PublicClient, WalletClient, Hash, zeroAddress, Hex, ContractFunctionParameters } from 'viem'; +import { Account, Address, PublicClient, WalletClient, Hash, zeroAddress, Hex } from 'viem';sdk/src/client/services/Erc20Service.ts (1)
71-82: Derive account from contractWriter.getAccount() when contractWriter is provided.When a contractWriter is injected without an explicit account parameter, line 82 only falls back to
walletClient?.accountbut doesn't attempt to derive the account fromcontractWriter.getAccount(). This means AA-based contract writers won't have their account properly initialized.Additionally, line 76 has a typo:
@ts-ignoresshould be@ts-ignore(singular).🔎 Apply this diff to fix both issues:
if (contractWriter) { this.contractWriter = contractWriter; } else if (walletClient) { this.contractWriter = new EOAContractWriter({ publicClient, - // @ts-ignores - walletClient, + // @ts-ignore - WalletClient generic mismatch + walletClient: walletClient as any, }); } this.publicClient = publicClient; - this.account = account || walletClient?.account; + this.account = account ?? this.contractWriter?.getAccount() ?? walletClient?.account;
🧹 Nitpick comments (4)
sdk/src/abis/aa/execute.ts (1)
1-78: LGTM! Consider addingas constfor stricter typing.The ABI definitions are correctly structured for AA execute functions. For improved TypeScript type inference (making the array readonly and inferring literal types), consider adding
as const:-export const AAExecuteAbi = [ +export const AAExecuteAbi = [ // ... entries ... -]; +] as const;This is optional but helps with viem's type inference when using the ABI.
sdk/test/unit/client/state.test.ts (1)
54-59: Consider adding test coverage forAccountStateSignerpath.With
participants[0]set to'0xWALLET'anddeps.account.addressset to'0xOWNER', this test exercises the fallback path wheredeps.stateSigneris used. Consider adding a test case whereparticipants[0]matchesdeps.account.addressto cover the newAccountStateSignerinstantiation path in_checkParticipantAndGetSigner.sdk/src/client/signer.ts (1)
65-99: Consider using consistent error types and reducing duplication.The implementation is correct, but two improvements could enhance consistency:
Use domain-specific error: The generic
Erroris inconsistent with the codebase's error handling pattern. Consider using a custom error fromErrors.*.Extract guard check: The
signMessagecheck is duplicated. Consider extracting it:🔎 Suggested refactor
export class AccountStateSigner implements StateSigner { private readonly account: Account; constructor(account: Account) { this.account = account; } + private ensureSignMessage(): NonNullable<Account['signMessage']> { + if (!this.account.signMessage) { + throw new Errors.AccountRequiredError('Account does not support message signing'); + } + return this.account.signMessage; + } getAddress(): Address { return this.account.address; } async signState(channelId: Hex, state: State): Promise<Hex> { - if (!this.account.signMessage) { - throw new Error('Account does not support message signing'); - } - + const signMessage = this.ensureSignMessage(); const packedState = getPackedState(channelId, state); - - return this.account.signMessage({ message: { raw: packedState } }); + return signMessage({ message: { raw: packedState } }); } async signRawMessage(message: Hex): Promise<Hex> { - if (!this.account.signMessage) { - throw new Error('Account does not support message signing'); - } - - return this.account.signMessage({ message: { raw: message } }); + const signMessage = this.ensureSignMessage(); + return signMessage({ message: { raw: message } }); } }sdk/src/client/types.ts (1)
96-149: Well-structured discriminated union for configuration variants.The refactoring to a base/variant structure with mutual exclusivity (
nevertypes) is clean and provides:
- Clear separation between legacy wallet-based and new ContractWriter-based configurations
- Proper deprecation notice on the legacy variant
- Type-safe enforcement that only one of
walletClientorcontractWritercan be providedMinor formatting nit at line 141-142 - the closing
*/could be on the same line:/** * ContractWriter instance for writing contract transactions. - */ + */ contractWriter: ContractWriter;
📜 Review details
Configuration used: defaults
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
go.work.sumis excluded by!**/*.sum
📒 Files selected for processing (20)
clearnode/signer.go(1 hunks)integration/tests/challenge_channel.test.ts(2 hunks)sdk/src/abis/aa/execute.ts(1 hunks)sdk/src/abis/aa/user_op_event.ts(1 hunks)sdk/src/client/contract_writer/aa_v06.ts(1 hunks)sdk/src/client/contract_writer/aa_v06_types.ts(1 hunks)sdk/src/client/contract_writer/eoa.ts(1 hunks)sdk/src/client/contract_writer/types.ts(1 hunks)sdk/src/client/index.ts(14 hunks)sdk/src/client/prepare.ts(0 hunks)sdk/src/client/services/Erc20Service.ts(7 hunks)sdk/src/client/services/NitroliteService.ts(25 hunks)sdk/src/client/signer.ts(2 hunks)sdk/src/client/state.ts(3 hunks)sdk/src/client/types.ts(3 hunks)sdk/src/errors.ts(2 hunks)sdk/test/unit/client/index.test.ts(14 hunks)sdk/test/unit/client/services/Erc20Service.test.ts(2 hunks)sdk/test/unit/client/services/NitroliteService.test.ts(3 hunks)sdk/test/unit/client/state.test.ts(4 hunks)
💤 Files with no reviewable changes (1)
- sdk/src/client/prepare.ts
🚧 Files skipped from review as they are similar to previous changes (5)
- sdk/src/errors.ts
- sdk/src/client/contract_writer/eoa.ts
- sdk/src/abis/aa/user_op_event.ts
- sdk/src/client/contract_writer/aa_v06.ts
- integration/tests/challenge_channel.test.ts
🧰 Additional context used
🧠 Learnings (1)
📚 Learning: 2025-11-05T10:19:06.155Z
Learnt from: dimast-x
Repo: erc7824/nitrolite PR: 411
File: clearnode/custody.go:449-465
Timestamp: 2025-11-05T10:19:06.155Z
Learning: In clearnode/custody.go's handleResized function during positive resize operations (deposits), the ledger flow is: on-chain funds first credit the channel account, are immediately debited from the channel account, then credit the unified wallet account. The channel account acts as a pass-through for deposits. The unified wallet account is where actual deposited funds are held. This is recorded as a deposit transaction from channelAccountID to walletAccountID.
Applied to files:
sdk/src/client/services/NitroliteService.ts
🧬 Code graph analysis (8)
sdk/test/unit/client/services/Erc20Service.test.ts (1)
sdk/src/errors.ts (1)
Errors(372-399)
sdk/src/client/index.ts (3)
sdk/src/client/contract_writer/types.ts (3)
ContractWriter(3-6)CallsDetails(37-39)WriteResult(41-43)sdk/src/client/contract_writer/eoa.ts (1)
EOAContractWriter(23-81)sdk/src/client/services/NitroliteService.ts (1)
NitroliteService(63-980)
sdk/test/unit/client/services/NitroliteService.test.ts (2)
sdk/src/client/services/NitroliteService.ts (1)
NitroliteService(63-980)sdk/src/errors.ts (1)
Errors(372-399)
sdk/src/client/signer.ts (4)
sdk/src/utils/state.ts (1)
getPackedState(10-35)clearnode/pkg/sign/sign.go (1)
Address(29-34)sdk/src/client/types.ts (1)
State(83-85)clearnode/pkg/rpc/error.go (1)
Error(48-50)
sdk/src/client/services/NitroliteService.ts (3)
sdk/src/client/contract_writer/types.ts (2)
ContractWriter(3-6)ContractCallParams(8-35)sdk/src/client/contract_writer/eoa.ts (1)
EOAContractWriter(23-81)sdk/src/errors.ts (1)
Errors(372-399)
sdk/src/client/services/Erc20Service.ts (3)
sdk/src/client/contract_writer/types.ts (2)
ContractWriter(3-6)ContractCallParams(8-35)sdk/src/client/contract_writer/eoa.ts (1)
EOAContractWriter(23-81)sdk/src/abis/token/index.ts (1)
Erc20Abi(6-34)
sdk/src/client/state.ts (1)
sdk/src/client/signer.ts (1)
AccountStateSigner(71-99)
sdk/src/client/types.ts (1)
sdk/src/client/contract_writer/types.ts (1)
ContractWriter(3-6)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (4)
- GitHub Check: Build and Publish (Clearnode)
- GitHub Check: Test (Integration) / Test Integration
- GitHub Check: Analyze (go)
- GitHub Check: Analyze (javascript-typescript)
🔇 Additional comments (21)
sdk/test/unit/client/state.test.ts (3)
37-44: LGTM!The
signMessagewiring on the account mock correctly supports the newAccountStateSignerimplementation which requiresaccount.signMessageto be available.
96-102: LGTM!Reformatted assertion for better readability while preserving the same test expectations.
208-214: LGTM!Reformatted assertion maintains the same test coverage with improved readability.
sdk/test/unit/client/services/Erc20Service.test.ts (2)
20-21: LGTM!The
waitForTransactionReceiptmock is properly added to support transaction confirmation flows.
124-127: Test correctly expectsContractWriterRequiredErrorwhenwalletClientis missing.The test is verified to be consistent with the implementation. The
Erc20Service.approve()method callsensureContractWriter()(line 88), which throwsContractWriterRequiredErrorwhen the contract writer is unavailable. This aligns with the test expectation.sdk/src/client/signer.ts (1)
55-55: LGTM!Minor formatting fix.
sdk/test/unit/client/services/NitroliteService.test.ts (2)
28-43: LGTM!The
mockContractWriteris correctly wired into the test setup, and theNitroliteServiceconstructor now receives it as the 5th parameter, aligning with the new ContractWriter abstraction.
159-180: LGTM!The test expectations are correctly updated to:
- Call
contractWriter.writewith batched calls format{ calls: [{ ...req, account }] }- Handle
txHashesarray response and extract the first hash- Use
ContractWriterRequiredErrorfor missing writer scenariossdk/src/client/state.ts (2)
3-5: LGTM!Import updates correctly add
AccountStateSignerand include the necessary utility function.
185-191: LGTM!Minor formatting improvement for destructuring readability.
sdk/src/client/types.ts (1)
4-4: LGTM!ContractWriter import correctly added for the new configuration variant.
sdk/src/client/contract_writer/types.ts (1)
1-43: LGTM! Well-structured type definitions for ContractWriter abstraction.The type definitions provide a clean abstraction layer for contract writing operations with proper separation of concerns. The generic ContractCallParams interface is flexible and the WriteResult/CallsDetails types support batched operations effectively.
sdk/src/client/index.ts (3)
64-85: LGTM! Constructor properly initializes contractWriter-based flow.The mutual exclusion logic correctly handles both walletClient and contractWriter initialization paths. Both services receive the contractWriter, enabling the new batched write pattern.
107-319: LGTM! Write operations consistently use contractWriter pattern.All write operations properly:
- Build CallsDetails with prepared call parameters
- Execute via contractWriter.write
- Extract the last transaction hash from batched results
The pattern is consistent and maintains proper error handling throughout.
454-460: LGTM! Clean helper for transaction hash extraction.The private helper properly validates the presence of transaction hashes before extracting the last one, with clear error messaging if the write operation returns no hashes.
sdk/src/client/contract_writer/aa_v06_types.ts (1)
1-30: LGTM! Clean type definitions for AA v0.6 bundler integration.The types properly define:
- PartialUserOperationV06 with appropriate optional fields
- GasParametersV06 with all required gas-related parameters
- BundlerClientV06 interface with essential methods for estimation, submission, and log fetching
sdk/test/unit/client/index.test.ts (2)
34-88: LGTM! Test setup properly mocks contractWriter-based flow.The test infrastructure correctly:
- Mocks contractWriter with write method returning txHashes
- Updates service mocks to use prepareXCallParams methods
- Injects mocks into the client for isolated testing
90-398: LGTM! Test cases consistently verify contractWriter integration.All test cases properly:
- Mock prepareXCallParams methods to return identifiable call objects
- Verify contractWriter.write is called with expected CallsDetails structure
- Assert returned txHash matches the mocked value ('0xTXHASH')
The test coverage validates the new batched write pattern across all operations.
sdk/src/client/services/Erc20Service.ts (2)
21-50: LGTM! executeWriteContract properly refactored to use ContractWriter.The function correctly:
- Accepts ContractWriter instead of WalletClient
- Builds a calls array with the request and account
- Executes via contractWriter.write
- Returns the last transaction hash with proper validation
152-219: LGTM! Approve methods properly use ContractWriter pattern.The new prepareApproveCallParams method and updated prepareApprove/approve methods correctly:
- Build ContractCallParams with proper parameters
- Simulate contract calls before execution
- Execute via contractWriter using the prepare/execute pattern
- Maintain proper error handling
sdk/src/client/services/NitroliteService.ts (1)
187-828: LGTM! All write operations consistently refactored to use ContractWriter.The refactoring properly:
- Adds prepareXCallParams methods for all operations (deposit, createChannel, depositAndCreateChannel, joinChannel, checkpoint, challenge, resize, close, withdraw)
- Each prepareXCallParams returns ContractCallParams for batching support
- Updates all write methods to use contractWriter via executeWriteContract
- Maintains proper ABI conversion and error handling throughout
- Provides clear JSDoc documentation for the new methods
The pattern is consistent and enables both individual and batched contract operations.
clearnode/signer.go
Outdated
| _, _, err := apitypes.TypedDataAndHash(typedData) | ||
| if err != nil { | ||
| return "", err | ||
| } | ||
|
|
||
| // 2. Fix V if needed (Ethereum uses 27/28, go-ethereum expects 0/1) | ||
| if sig[64] >= 27 { | ||
| sig[64] -= 27 | ||
| } | ||
| // if sig[64] >= 27 { | ||
| // sig[64] -= 27 | ||
| // } | ||
|
|
||
| // 3. Recover public key | ||
| pubKey, err := crypto.SigToPub(typedDataHash, sig) | ||
| if err != nil { | ||
| return "", err | ||
| } | ||
| // // 3. Recover public key | ||
| // pubKey, err := crypto.SigToPub(typedDataHash, sig) | ||
| // if err != nil { | ||
| // return "", err | ||
| // } | ||
|
|
||
| signerAddress := crypto.PubkeyToAddress(*pubKey) | ||
| // signerAddress := crypto.PubkeyToAddress(*pubKey) | ||
|
|
||
| return signerAddress.Hex(), nil | ||
| return walletAddress, nil |
There was a problem hiding this comment.
Critical: Signature verification is completely disabled, enabling impersonation attacks.
The function RecoverAddressFromEip712Signature no longer performs any cryptographic verification. It discards the typedDataHash, comments out the signature recovery logic, and simply returns the caller-provided walletAddress. This means:
- Any signature (valid or not) will be accepted
- An attacker can claim to be any wallet address without proving ownership
- The
sigparameter is now effectively unused beyond the length check
If this is intended to support Account Abstraction (where wallets use ERC-1271 isValidSignature instead of ECDSA recovery), the function should call the wallet contract to verify the signature rather than bypassing verification entirely.
🔎 Suggested approach for AA support:
For AA/smart contract wallets, you need to call isValidSignature(bytes32 hash, bytes signature) on the wallet contract per ERC-1271:
// Pseudocode for proper AA signature verification
func RecoverAddressFromEip712Signature(...) (string, error) {
_, typedDataHash, err := apitypes.TypedDataAndHash(typedData)
if err != nil {
return "", err
}
// Try ECDSA recovery first (for EOA wallets)
if sig[64] >= 27 {
sig[64] -= 27
}
pubKey, err := crypto.SigToPub(typedDataHash, sig)
if err == nil {
recoveredAddr := crypto.PubkeyToAddress(*pubKey)
if strings.EqualFold(recoveredAddr.Hex(), walletAddress) {
return walletAddress, nil
}
}
// Fall back to ERC-1271 for smart contract wallets
isValid, err := verifyERC1271Signature(walletAddress, typedDataHash, sig)
if err != nil || !isValid {
return "", fmt.Errorf("signature verification failed")
}
return walletAddress, nil
}🤖 Prompt for AI Agents
In clearnode/signer.go around lines 125-143, the EIP-712 signature verification
has been disabled (typedDataHash is discarded and signature recovery commented
out) which allows impersonation; restore proper verification by: compute
typedDataHash via apitypes.TypedDataAndHash and handle its error, normalize the
V byte (if sig[64] >= 27 subtract 27), attempt ECDSA recovery with
crypto.SigToPub and compare the recovered address to the provided walletAddress
returning on match, and if recovery fails or addresses do not match, call the
wallet contract's ERC-1271 isValidSignature(walletAddress, typedDataHash, sig)
to validate smart-contract wallets; propagate errors and only return
walletAddress when verification succeeds.
| if (contractWriter) { | ||
| this.contractWriter = contractWriter; | ||
| } else if (walletClient) { | ||
| this.contractWriter = new EOAContractWriter({ | ||
| publicClient, | ||
| // @ts-ignore | ||
| walletClient, | ||
| }); | ||
| } | ||
|
|
||
| this.publicClient = publicClient; | ||
| this.walletClient = walletClient; | ||
| this.account = account || walletClient?.account; |
There was a problem hiding this comment.
Derive account from contractWriter.getAccount() when available.
Similar to Erc20Service, line 95 doesn't attempt to derive the account from contractWriter.getAccount() when a contractWriter is provided without an explicit account. This prevents AA-based writers from properly initializing their account.
🔎 Apply this diff:
if (contractWriter) {
this.contractWriter = contractWriter;
} else if (walletClient) {
this.contractWriter = new EOAContractWriter({
publicClient,
// @ts-ignore
walletClient,
});
}
this.publicClient = publicClient;
- this.account = account || walletClient?.account;
+ this.account = account ?? this.contractWriter?.getAccount() ?? walletClient?.account;
this.addresses = addresses;📝 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.
| if (contractWriter) { | |
| this.contractWriter = contractWriter; | |
| } else if (walletClient) { | |
| this.contractWriter = new EOAContractWriter({ | |
| publicClient, | |
| // @ts-ignore | |
| walletClient, | |
| }); | |
| } | |
| this.publicClient = publicClient; | |
| this.walletClient = walletClient; | |
| this.account = account || walletClient?.account; | |
| if (contractWriter) { | |
| this.contractWriter = contractWriter; | |
| } else if (walletClient) { | |
| this.contractWriter = new EOAContractWriter({ | |
| publicClient, | |
| // @ts-ignore | |
| walletClient, | |
| }); | |
| } | |
| this.publicClient = publicClient; | |
| this.account = account ?? this.contractWriter?.getAccount() ?? walletClient?.account; | |
| this.addresses = addresses; |
🤖 Prompt for AI Agents
In sdk/src/client/services/NitroliteService.ts around lines 84 to 95, when a
contractWriter is provided the code does not derive the account from
contractWriter.getAccount(), so Account Abstraction writers don't initialize
properly; change the initialization to prefer an explicit account, then
contractWriter.getAccount() if available, then walletClient?.account — i.e. set
this.account = account || (contractWriter?.getAccount ?
contractWriter.getAccount() : walletClient?.account) and if getAccount returns a
Promise adjust the constructor/initialization to await it (or resolve it before
use) so AA-based contractWriters get their account populated.
| function _checkParticipantAndGetSigner(deps: PreparerDependencies, participant: Address): StateSigner { | ||
| let signer = deps.stateSigner; | ||
| if (participant == deps.walletClient.account.address) { | ||
| signer = new WalletStateSigner(deps.walletClient); | ||
| if (participant == deps.account.address) { | ||
| signer = new AccountStateSigner(deps.account); | ||
| } | ||
|
|
||
| return signer; | ||
| } |
There was a problem hiding this comment.
Use strict equality for address comparison.
Ethereum addresses should be compared case-insensitively since they can have different checksummed representations. Using loose equality (==) may lead to unexpected behavior:
🔎 Suggested fix
function _checkParticipantAndGetSigner(deps: PreparerDependencies, participant: Address): StateSigner {
let signer = deps.stateSigner;
- if (participant == deps.account.address) {
+ if (participant.toLowerCase() === deps.account.address.toLowerCase()) {
signer = new AccountStateSigner(deps.account);
}
return signer;
}Alternatively, viem provides isAddressEqual utility for safe address comparison:
import { isAddressEqual } from 'viem';
// ...
if (isAddressEqual(participant, deps.account.address)) {🤖 Prompt for AI Agents
In sdk/src/client/state.ts around lines 200 to 206, the address comparison uses
loose equality (==) which can mis-handle checksummed Ethereum addresses; replace
that comparison with a case-insensitive/address-safe check by importing and
using viem's isAddressEqual (import { isAddressEqual } from 'viem') and change
the if to use isAddressEqual(participant, deps.account.address) so comparisons
are correct across checksummed variants.
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (2)
sdk/src/client/services/NitroliteService.ts (1)
69-97: Account derivation incomplete for contractWriter.Similar to Erc20Service, line 95 only falls back to
walletClient?.accountwithout deriving the account from the injectedcontractWriter. Past review comments indicate this was addressed in commit 0ad95ad, but the current code still showsthis.account = account || walletClient?.account. AA-based ContractWriters won't initialize their account properly.🔎 Apply this diff to derive account from contractWriter:
this.publicClient = publicClient; - this.account = account || walletClient?.account; + this.account = account ?? this.contractWriter?.getAccount?.() ?? walletClient?.account; this.addresses = addresses;Note: The
@ts-ignoreon line 89 suppresses a type mismatch. Consider properly typing thewalletClientparameter or using an explicit type assertion with documentation.sdk/src/client/services/Erc20Service.ts (1)
61-83: Account derivation incomplete for contractWriter.Line 82 only falls back to
walletClient?.accountwithout attempting to derive the account from the injectedcontractWriter. According to past review comments, this was addressed in commits 2ba0a80 to 4f121ac, but the current code still showsthis.account = account || walletClient?.account. When an AA-based ContractWriter is provided without an explicit account parameter, it won't initialize properly.🔎 Apply this diff to derive account from contractWriter:
this.publicClient = publicClient; - this.account = account || walletClient?.account; + this.account = account ?? this.contractWriter?.getAccount?.() ?? walletClient?.account;Note: The
@ts-ignoreon line 76 suppresses a type mismatch between the bareWalletClientparameter type and theWalletClient<Transport, Chain, ParseAccount<Account>>expected byEOAContractWriter. Consider properly typing the parameter or using an explicit type assertion with a comment explaining why it's safe.
🧹 Nitpick comments (4)
sdk/src/client/state.ts (4)
202-204: Remove commented-out code rather than leaving it in the codebase.Commented-out code creates technical debt and can confuse future maintainers. Since this architectural change (removing wallet-based signer branching) appears intentional based on the PR's ContractWriter introduction, the commented lines should be deleted rather than left as comments.
If there's a possibility of reverting this logic, consider documenting the rationale in a commit message or tracking issue instead of leaving the code commented.
🔎 Apply this diff to remove commented code:
function _checkParticipantAndGetSigner(deps: PreparerDependencies, participant: Address): StateSigner { let signer = deps.stateSigner; - // if (participant == deps.account.address) { - // signer = new WalletStateSigner(deps.account); - // } return signer; }
200-206: Consider simplifying or removing the now-redundant function.Since
_checkParticipantAndGetSignernow simply returnsdeps.stateSignerwithout using theparticipantparameter, the function has become a no-op passthrough. You could either:
- Remove the function entirely and inline
deps.stateSignerat call sites (lines 60, 190), or- Keep it as a future extension point if you plan to restore participant-based logic
If keeping it for API stability or future extensibility, consider adding a comment explaining the intent.
176-183: Update documentation to reflect current behavior.The function comments claim it "provides backward compatibility" and returns a signer "depending on the user participant address," but the current implementation always returns
deps.stateSignerregardless of the participant. This mismatch between documentation and behavior can mislead developers.Either update the comments to accurately describe the current behavior or restore the participant-based branching logic if the backward compatibility is still required.
5-5: Remove unused import.
WalletStateSigneris imported but no longer used after the signer selection logic was commented out. Removing it will keep imports clean.🔎 Apply this diff to remove the unused import:
-import { StateSigner, WalletStateSigner } from './signer'; +import { StateSigner } from './signer';
📜 Review details
Configuration used: defaults
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (4)
sdk/src/client/services/Erc20Service.ts(7 hunks)sdk/src/client/services/NitroliteService.ts(25 hunks)sdk/src/client/signer.ts(1 hunks)sdk/src/client/state.ts(3 hunks)
✅ Files skipped from review due to trivial changes (1)
- sdk/src/client/signer.ts
🧰 Additional context used
🧠 Learnings (1)
📚 Learning: 2025-11-05T10:19:06.155Z
Learnt from: dimast-x
Repo: erc7824/nitrolite PR: 411
File: clearnode/custody.go:449-465
Timestamp: 2025-11-05T10:19:06.155Z
Learning: In clearnode/custody.go's handleResized function during positive resize operations (deposits), the ledger flow is: on-chain funds first credit the channel account, are immediately debited from the channel account, then credit the unified wallet account. The channel account acts as a pass-through for deposits. The unified wallet account is where actual deposited funds are held. This is recorded as a deposit transaction from channelAccountID to walletAccountID.
Applied to files:
sdk/src/client/services/NitroliteService.ts
🔇 Additional comments (15)
sdk/src/client/state.ts (2)
3-3: LGTM: Clean import consolidation.Consolidating imports from the same module improves readability without affecting behavior.
185-187: LGTM: Cleaner destructuring pattern.Nested destructuring improves code clarity without changing behavior.
sdk/src/client/services/Erc20Service.ts (5)
21-50: LGTM! ContractWriter integration implemented correctly.The refactored
executeWriteContractproperly wraps the prepared request into a batched call array, delegates tocontractWriter.write, and returns the last transaction hash with appropriate error handling.
85-91: LGTM! Guard method implemented correctly.The
ensureContractWritermethod properly validates the presence of a ContractWriter and throws an appropriate error if missing.
152-170: LGTM! Parameter preparation method well-documented.The
prepareApproveCallParamsmethod correctly constructsContractCallParamsfor ERC20 approve operations with clear documentation.
182-194: LGTM! Refactored to use the new parameter helper.The
prepareApprovemethod correctly delegates toprepareApproveCallParamsfor parameter construction.
208-219: LGTM! Write operation migrated to ContractWriter flow.The
approvemethod correctly usesensureContractWriter()and delegates to the refactoredexecuteWriteContract.sdk/src/client/services/NitroliteService.ts (8)
28-57: LGTM! Consistent ContractWriter integration.The refactored
executeWriteContractcorrectly implements the batched write flow, matching the pattern in Erc20Service.
99-105: LGTM! Guard method implemented correctly.
154-154: LGTM! Type refinement for ABI compatibility.The explicit cast to
readonly Hex[]ensures proper typing for the generated ABI's expectations.
187-206: LGTM! Deposit parameter preparation well-implemented.The method correctly constructs deposit call parameters with proper handling of native ETH vs ERC20 token deposits via the
valuefield.
215-250: LGTM! Deposit operations migrated to ContractWriter flow.Both
prepareDepositanddepositcorrectly implement the new ContractWriter-based pattern.
252-271: LGTM! Comprehensive suite of parameter preparation methods.All
prepareXCallParamsmethods (create, depositAndCreate, join, checkpoint, challenge, resize, close, withdraw) follow a consistent pattern, properly utilize ABI conversion helpers, and are well-documented. This addresses the past review concern about missing JSDoc comments.Also applies to: 317-344, 404-422, 470-490, 542-568, 627-647, 699-719, 767-784
280-292: LGTM! Simulation methods refactored consistently.All
prepareXmethods correctly delegate parameter construction to their respectiveprepareXCallParamshelpers, maintaining clean separation of concerns.Also applies to: 355-372, 432-444, 500-516, 579-596, 657-673, 729-741, 793-805
303-315: LGTM! All write operations migrated to ContractWriter.All write methods (createChannel, depositAndCreateChannel, joinChannel, checkpoint, challenge, resize, close, withdraw) consistently use
ensureContractWriter()and delegate to the refactoredexecuteWriteContract, completing the migration from WalletClient-based writes.Also applies to: 385-402, 456-468, 528-540, 608-625, 685-697, 753-765, 816-828
MaxMoskalenko
force-pushed
the
feat/aa-support
branch
2 times, most recently
from
5926956 to
4818f34
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
sdk/src/client/index.ts (1)
393-408: Update JSDoc to reflect correct error type thrown byapprove()method.The JSDoc at line 206 references
WalletClientRequiredError, but the code at line 209 callsensureContractWriter()which throwsContractWriterRequiredError. Update line 206 from:@throws {WalletClientRequiredError | AccountRequiredError} If wallet/account is missing.to:
@throws {ContractWriterRequiredError | AccountRequiredError} If contractWriter/account is missing.
♻️ Duplicate comments (3)
sdk/src/client/contract_writer/aa_v06.ts (1)
249-257: FixyParityencoding: handle0as valid value and use consistent padding size.The current implementation treats
yParity = 0as falsy (line 256), which incorrectly triggers the fallback path. Additionally, the fallback pads to 32 bytes while the truthy branch uses 1 byte. Per EIP-7702,yParityvalues of 0 and 1 are both valid.🔎 Apply this diff:
private _formatAuthorization = (authorization: SignedAuthorization) => { return { address: authorization.address, chainId: numberToHex(authorization.chainId), nonce: numberToHex(authorization.nonce), r: authorization.r ? numberToHex(BigInt(authorization.r), { size: 32 }) : pad('0x', { size: 32 }), s: authorization.s ? numberToHex(BigInt(authorization.s), { size: 32 }) : pad('0x', { size: 32 }), - yParity: authorization.yParity ? numberToHex(authorization.yParity, { size: 1 }) : pad('0x', { size: 32 }), + yParity: + authorization.yParity !== undefined + ? numberToHex(authorization.yParity, { size: 1 }) + : pad('0x', { size: 1 }), }; };sdk/src/client/services/Erc20Service.ts (1)
81-83: Account derivation should includecontractWriter.getAccount()fallback.When
contractWriteris provided without an explicitaccount, the code falls back towalletClient?.accountbut doesn't attempt to derive fromcontractWriter.getAccount(). This could causeensureAccount()to throw even when the account is available via the contract writer.🔎 Apply this diff:
this.publicClient = publicClient; - this.account = account || walletClient?.account; + this.account = account ?? this.contractWriter?.getAccount() ?? walletClient?.account; }sdk/src/client/services/NitroliteService.ts (1)
84-95: Derive account from contractWriter.getAccount() when available.The account initialization at line 95 still doesn't attempt to derive the account from
contractWriter.getAccount()when a contractWriter is provided without an explicit account. This prevents AA-based writers from properly initializing their account, as previously noted.🔎 Apply this diff:
if (contractWriter) { this.contractWriter = contractWriter; } else if (walletClient) { this.contractWriter = new EOAContractWriter({ publicClient, // @ts-ignore walletClient, }); } this.publicClient = publicClient; - this.account = account || walletClient?.account; + this.account = account ?? this.contractWriter?.getAccount() ?? walletClient?.account; this.addresses = addresses;
🧹 Nitpick comments (7)
sdk/src/abis/aa/execute.ts (1)
1-78: Add explicit type annotation for type safety.The ABI constant lacks an explicit type annotation. Adding one improves IDE support and catches errors at compile time.
🔎 Suggested fix
+import { Abi } from 'viem'; + -export const AAExecuteAbi = [ +export const AAExecuteAbi: Abi = [ { inputs: [sdk/src/client/types.ts (1)
117-149: Well-structured discriminated union for configuration.The use of
nevertypes to enforce mutual exclusivity betweenwalletClientandcontractWriteris a good TypeScript pattern. This prevents misconfiguration at compile time.Minor: Line 141 has inconsistent JSDoc formatting (
*/on same line as content).🔎 Minor formatting fix
/** * ContractWriter instance for writing contract transactions. - */ + */ contractWriter: ContractWriter;sdk/src/client/contract_writer/types.ts (1)
1-1: Remove unused importContractFunctionParameters.The
ContractFunctionParameterstype is imported but never used in this file.🔎 Apply this diff:
-import { Abi, Account, Address, Chain, ContractFunctionParameters, Hex } from 'viem'; +import { Abi, Account, Address, Chain, Hex } from 'viem';sdk/test/unit/client/index.test.ts (1)
123-137: Error expectations may need refinement for deposit failures.Lines 128 and 136 expect generic
Errorto be thrown, but the actual implementation doesn't wrap these errors inContractCallError(unlike other methods). This is inconsistent with methods likecreateChannelwhich wrap errors. Consider whetherdepositshould also wrap errors inContractCallErrorfor consistency.#!/bin/bash # Check if deposit method wraps errors like other methods rg -n "async deposit\(" sdk/src/client/index.ts -A 25 | head -30sdk/src/client/services/Erc20Service.ts (1)
204-207: Update JSDoc to reflectContractWriterRequiredError.The JSDoc mentions
WalletClientRequiredErrorbut the method now throwsContractWriterRequiredErrorviaensureContractWriter().🔎 Apply this diff:
* @throws {ContractCallError} If simulation fails. * @throws {TransactionError} If sending the transaction fails. - * @throws {WalletClientRequiredError | AccountRequiredError} If wallet/account is missing. + * @throws {ContractWriterRequiredError | AccountRequiredError} If contract writer/account is missing. */sdk/src/client/contract_writer/aa_v06.ts (2)
83-90: Consider addressing the@ts-ignorewith explicit type conversion.The
@ts-ignorehides a type mismatch betweenPartialUserOperationV06(withbigintfields) andRpcUserOperation(withHexstring fields). While the nonce is converted on line 87, other bigint fields fromgasParametersmay not be properly typed.#!/bin/bash # Check what gasParameters contains and its types rg -n "GasParametersV06" sdk/src/client/contract_writer/aa_v06_types.ts -A 10
149-150: Use the validatedchainIdvariable instead of non-null assertion.Line 142-147 validates
chainIdand returns early if undefined, but line 150 still usesthis.publicClient.chain!.id. Use the validatedchainIdvariable for consistency.🔎 Apply this diff:
const logs = await this.bundlerClient.fetchLogs( - this.publicClient.chain!.id, + chainId, [entryPoint06Address], [[toEventSelector(UserOpEventAbi)], [userOpHash]], );
📜 Review details
Configuration used: defaults
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (19)
integration/tests/challenge_channel.test.ts(2 hunks)sdk/src/abis/aa/execute.ts(1 hunks)sdk/src/abis/aa/user_op_event.ts(1 hunks)sdk/src/client/contract_writer/aa_v06.ts(1 hunks)sdk/src/client/contract_writer/aa_v06_types.ts(1 hunks)sdk/src/client/contract_writer/eoa.ts(1 hunks)sdk/src/client/contract_writer/types.ts(1 hunks)sdk/src/client/index.ts(14 hunks)sdk/src/client/prepare.ts(0 hunks)sdk/src/client/services/Erc20Service.ts(7 hunks)sdk/src/client/services/NitroliteService.ts(25 hunks)sdk/src/client/signer.ts(1 hunks)sdk/src/client/state.ts(3 hunks)sdk/src/client/types.ts(3 hunks)sdk/src/errors.ts(2 hunks)sdk/test/unit/client/index.test.ts(14 hunks)sdk/test/unit/client/services/Erc20Service.test.ts(2 hunks)sdk/test/unit/client/services/NitroliteService.test.ts(3 hunks)sdk/test/unit/client/state.test.ts(4 hunks)
💤 Files with no reviewable changes (1)
- sdk/src/client/prepare.ts
🚧 Files skipped from review as they are similar to previous changes (7)
- sdk/test/unit/client/services/Erc20Service.test.ts
- sdk/src/client/signer.ts
- sdk/test/unit/client/state.test.ts
- sdk/src/client/contract_writer/eoa.ts
- sdk/src/errors.ts
- sdk/src/client/contract_writer/aa_v06_types.ts
- integration/tests/challenge_channel.test.ts
🧰 Additional context used
🧠 Learnings (1)
📚 Learning: 2025-11-05T10:19:06.155Z
Learnt from: dimast-x
Repo: erc7824/nitrolite PR: 411
File: clearnode/custody.go:449-465
Timestamp: 2025-11-05T10:19:06.155Z
Learning: In clearnode/custody.go's handleResized function during positive resize operations (deposits), the ledger flow is: on-chain funds first credit the channel account, are immediately debited from the channel account, then credit the unified wallet account. The channel account acts as a pass-through for deposits. The unified wallet account is where actual deposited funds are held. This is recorded as a deposit transaction from channelAccountID to walletAccountID.
Applied to files:
sdk/src/client/services/NitroliteService.ts
🧬 Code graph analysis (6)
sdk/src/client/types.ts (1)
sdk/src/client/contract_writer/types.ts (1)
ContractWriter(3-6)
sdk/test/unit/client/services/NitroliteService.test.ts (2)
sdk/src/client/services/NitroliteService.ts (1)
NitroliteService(63-980)sdk/src/errors.ts (1)
Errors(372-399)
sdk/test/unit/client/index.test.ts (1)
sdk/src/client/index.ts (1)
NitroliteClient(34-461)
sdk/src/client/index.ts (2)
sdk/src/client/contract_writer/types.ts (3)
ContractWriter(3-6)CallsDetails(37-39)WriteResult(41-43)sdk/src/client/contract_writer/eoa.ts (1)
EOAContractWriter(23-81)
sdk/src/client/services/NitroliteService.ts (7)
sdk/src/client/contract_writer/types.ts (2)
ContractWriter(3-6)ContractCallParams(8-35)sdk/src/abis/types.ts (1)
ContractAddresses(6-12)sdk/src/client/contract_writer/eoa.ts (1)
EOAContractWriter(23-81)sdk/src/errors.ts (1)
Errors(372-399)sdk/src/abis/generated.ts (1)
custodyAbi(2-1567)sdk/src/abis/custody/index.ts (1)
custodyAbi(1-1)sdk/src/client/types.ts (3)
Channel(34-39)State(83-85)Signature(20-20)
sdk/src/client/contract_writer/aa_v06.ts (4)
sdk/src/client/contract_writer/aa_v06_types.ts (2)
BundlerClientV06(26-30)PartialUserOperationV06(4-15)sdk/src/errors.ts (1)
Errors(372-399)sdk/src/abis/aa/user_op_event.ts (1)
UserOpEventAbi(3-51)sdk/src/abis/aa/execute.ts (1)
AAExecuteAbi(1-78)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: Test (Integration) / Test Integration
🔇 Additional comments (17)
sdk/src/abis/aa/user_op_event.ts (1)
1-51: LGTM!The
UserOperationEventABI definition correctly matches the ERC-4337 v0.6 EntryPoint event structure with proper indexed fields (userOpHash,sender,paymaster) and non-indexed data fields.sdk/test/unit/client/services/NitroliteService.test.ts (2)
40-43: LGTM!The test setup correctly introduces
mockContractWriterwith the expectedwritemethod signature and properly injects it as a constructor dependency. This aligns with the newContractWriterabstraction.
159-180: LGTM!The execution tests are properly updated to:
- Mock
contractWriter.writereturning{ txHashes: ['0xhash'] }- Assert that
writeis called with the correct{ calls: [{ ...req, account }] }structure- Use the new
ContractWriterRequiredErrorfor error handling testssdk/src/client/types.ts (1)
96-115: LGTM!The
BaseNitroliteClientConfigcleanly extracts shared configuration fields, reducing duplication across the wallet and contract-writer variants.sdk/src/client/contract_writer/types.ts (1)
37-39: Consider makingCallsDetailsmore flexible for single-call scenarios.Currently all operations must wrap calls in an array even for single operations. This is fine, but you might consider adding a convenience type alias or allowing a single
ContractCallParamsto be passed directly in future iterations.sdk/src/client/index.ts (3)
64-76: Well-structured configuration validation for mutually exclusive paths.The constructor properly validates that either
walletClientorcontractWriteris provided (but not both implicitly through the 'in' checks), and correctly initializes thecontractWriterfield accordingly. The account derivation logic is sound.
111-127: Good batching pattern for deposit with conditional approval.The deposit method correctly accumulates calls into
CallsDetailsand conditionally adds an approve call when allowance is insufficient. This enables atomic batching for AA wallets while maintaining sequential execution for EOA.
454-460: Consistent helper for extracting the last transaction hash.The
_getLastTxHashFromWriteResulthelper provides a clean abstraction for extracting the final transaction hash from batched writes. The error handling for empty results is appropriate.sdk/test/unit/client/index.test.ts (2)
44-47: Mock setup is appropriate for the new contractWriter pattern.The mock correctly returns a
WriteResultstructure withtxHashesarray, matching the interface definition.
76-88: Good practice overriding internal services for isolated unit testing.The test properly overrides
nitroliteService,erc20Service, andcontractWriterto isolate the client logic from service implementations.sdk/src/client/services/Erc20Service.ts (1)
152-170: Well-designedprepareApproveCallParamsfor batched operations.This method cleanly separates call preparation from execution, enabling integration with the batched write pattern used throughout the client.
sdk/src/client/contract_writer/aa_v06.ts (3)
39-50: Good constructor validation and sensible defaults.The constructor properly validates required parameters and sets reasonable defaults for polling (5s interval, 120s timeout).
52-57: Clean write implementation with proper batching.The
writemethod correctly maps all calls through_prepareCalldataand executes them as a single batched UserOperation, returning the transaction hash.
136-180: Robust polling implementation with proper cleanup.The
_waitForUserOperationReceiptmethod correctly handles:
- Chain ID validation before polling
- Cleanup of interval on success, timeout, or error
- Returns after resolve to prevent continued execution
- Configurable timeout with clear error message
sdk/src/client/services/NitroliteService.ts (3)
28-57: Well-structured refactoring to support ContractWriter abstraction.The refactored
executeWriteContractfunction properly routes write operations through the ContractWriter interface. The design wraps single requests in a calls array to support future batching scenarios, and includes appropriate error handling for missing transaction hashes.
187-206: Excellent implementation of the prepareXCallParams methods.All the
prepareXCallParamsmethods follow a consistent pattern, include comprehensive JSDoc documentation (addressing previous feedback), and properly constructContractCallParamsobjects with appropriate ABI conversions and parameter handling. The methods enable batching operations through the ContractWriter abstraction while maintaining type safety.Also applies to: 252-271, 317-344, 404-422, 470-490, 542-568, 627-647, 699-719, 767-784
238-250: Consistent migration of all write operations to ContractWriter.All write operation methods have been properly refactored to use the ContractWriter abstraction. The methods consistently call
ensureContractWriter()andensureAccount(), route throughexecuteWriteContract, and maintain appropriate error handling throughout.Also applies to: 303-315, 385-402, 456-468, 528-540, 608-625, 685-697, 753-765, 816-828
MaxMoskalenko
force-pushed
the
feat/aa-support
branch
4 times, most recently
from
3d871e7 to
9493060
Compare
MaxMoskalenko
force-pushed
the
feat/aa-support
branch
from
9493060 to
0141ed0
Compare
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (7)
sdk/src/client/state.ts (1)
202-202: Use strict, case-insensitive address comparison.The loose equality operator (
==) is used for address comparison, which can mis-handle checksummed Ethereum addresses. This issue was previously flagged in past reviews.Use viem's
isAddressEqualutility for safe address comparison:🔎 Apply this fix:
Add the import at the top of the file:
-import { Address, zeroAddress } from 'viem'; +import { Address, isAddressEqual, zeroAddress } from 'viem';Then update the comparison:
- if (participant == deps.account.address && signer instanceof SessionKeyStateSigner) { + if (isAddressEqual(participant, deps.account.address) && signer instanceof SessionKeyStateSigner) {Based on past review comments.
sdk/src/client/services/Erc20Service.ts (2)
74-78: Address the@ts-ignoretype mismatch.The
walletClientparameter is typed as bareWalletClient, butEOAContractWriterexpectsWalletClient<Transport, Chain, ParseAccount<Account>>. Consider properly typing the parameter or using an explicit type assertion with a comment explaining why it's safe.
71-83: Account derivation doesn't fall back to contractWriter.getAccount() for AA path.When a
contractWriteris provided without an explicitaccount, the code falls back towalletClient?.account(line 82). For AA-based writers, there may be nowalletClient, so the account won't be set even thoughcontractWriter.getAccount()could provide it.🔎 Suggested fix:
this.publicClient = publicClient; - this.account = account || walletClient?.account; + this.account = account ?? this.contractWriter?.getAccount() ?? walletClient?.account; }sdk/src/client/services/NitroliteService.ts (2)
86-91: Address the@ts-ignoretype mismatch.Same issue as in Erc20Service: the
walletClientparameter is typed as bareWalletClient, butEOAContractWriterexpects specific generic parameters. Consider properly typing the parameter or adding an explicit type assertion with a comment.
84-96: Account derivation doesn't fall back to contractWriter.getAccount() for AA path.Similar to the issue in Erc20Service, when a
contractWriteris provided without an explicitaccount, line 95 only falls back towalletClient?.account. For AA-based writers without awalletClient, the account won't be set even thoughcontractWriter.getAccount()could provide it.🔎 Suggested fix:
this.publicClient = publicClient; - this.account = account || walletClient?.account; + this.account = account ?? this.contractWriter?.getAccount() ?? walletClient?.account; this.addresses = addresses;sdk/src/client/contract_writer/aa_v06.ts (2)
249-258: Fix yParity encoding: handle0correctly and use consistent padding.Two issues in
_formatAuthorization:
authorization.yParity ? ... : ...treats0(a valid yParity value per EIP-7702) as falsy, incorrectly triggering the fallback path.- The fallback pads to 32 bytes while the truthy branch uses 1 byte, creating inconsistent encoding.
🔎 Apply this diff:
return { address: authorization.address, chainId: numberToHex(authorization.chainId), nonce: numberToHex(authorization.nonce), r: authorization.r ? numberToHex(BigInt(authorization.r), { size: 32 }) : pad('0x', { size: 32 }), s: authorization.s ? numberToHex(BigInt(authorization.s), { size: 32 }) : pad('0x', { size: 32 }), - yParity: authorization.yParity ? numberToHex(authorization.yParity, { size: 1 }) : pad('0x', { size: 32 }), + yParity: + authorization.yParity !== undefined + ? numberToHex(authorization.yParity, { size: 1 }) + : pad('0x', { size: 1 }), };
149-153: Use the localchainIdvariable instead of non-null assertion.Line 150 uses
this.publicClient.chain!.idwith a non-null assertion, but you already extracted and validatedchainIdat line 142. Use the local variable for consistency.🔎 Apply this diff:
const logs = await this.bundlerClient.fetchLogs( - this.publicClient.chain!.id, + chainId, [entryPoint06Address], [[toEventSelector(UserOpEventAbi)], [userOpHash]], );
🧹 Nitpick comments (5)
sdk/src/client/state.ts (1)
5-5: Remove unused WalletStateSigner import.The
WalletStateSignerimport appears to be unused after the signer selection logic was updated to useAccountStateSigner(line 203). Unused imports should be removed to keep the code clean.🔎 Apply this diff to remove the unused import:
-import { AccountStateSigner, SessionKeyStateSigner, StateSigner, WalletStateSigner } from './signer'; +import { AccountStateSigner, SessionKeyStateSigner, StateSigner } from './signer';sdk/src/client/signer.ts (1)
71-99: Consider using a proper error type for missing signMessage capability.The
AccountStateSignerthrows a genericErrorwhenaccount.signMessageis unavailable (lines 84, 94). For consistency with the codebase's error handling patterns, consider using a proper error type from theErrorsmodule, such asInvalidParameterErrororMissingParameterError.🔎 Suggested improvement:
+import * as Errors from '../errors'; export class AccountStateSigner implements StateSigner { // ... async signState(channelId: Hex, state: State): Promise<Hex> { if (!this.account.signMessage) { - throw new Error('Account does not support message signing'); + throw new Errors.InvalidParameterError('Account does not support message signing'); } // ... } async signRawMessage(message: Hex): Promise<Hex> { if (!this.account.signMessage) { - throw new Error('Account does not support message signing'); + throw new Errors.InvalidParameterError('Account does not support message signing'); } // ... } }sdk/src/client/contract_writer/types.ts (1)
1-43: LGTM! Clean interface design for ContractWriter abstraction.The interface design properly supports:
- Batched contract calls via
CallsDetails- Generic typing for flexible ABI/function handling
- Multiple transaction hashes for batch operations
The commented-out parameters (lines 26, 33) might be reserved for future features. Consider adding a TODO comment if these are planned additions, or remove them if not needed.
sdk/test/unit/client/index.test.ts (1)
123-137: Error type assertions could be more specific.The tests now expect generic
Errorinstead of specific error types likeTokenErrororContractCallError. While this works, it reduces test precision. Based on the production code insdk/src/client/index.ts, thedepositmethod wraps failures inContractCallError. Consider updating to:- await expect(client.deposit(tokenAddress, 10n)).rejects.toThrow(Error); + await expect(client.deposit(tokenAddress, 10n)).rejects.toThrow(Errors.ContractCallError);sdk/src/client/contract_writer/aa_v06.ts (1)
83-90: Consider addressing the@ts-ignorewith explicit type conversion.The
@ts-ignoreat line 84 suppresses a type mismatch betweenPartialUserOperationV06(with bigint fields) andRpcUserOperation(with Hex string fields). While the runtime behavior is correct due to the nonce conversion at line 87, consider a cleaner approach:🔎 Consider extracting a conversion helper:
// Convert PartialUserOperationV06 to RpcUserOperation format const toRpcPartialUserOp = (partial: PartialUserOperationV06): Partial<RpcUserOperation<'0.6'>> => ({ ...partial, nonce: numberToHex(partial.nonce), // Add other bigint->hex conversions as needed });
📜 Review details
Configuration used: defaults
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (19)
integration/tests/challenge_channel.test.ts(2 hunks)sdk/src/abis/aa/execute.ts(1 hunks)sdk/src/abis/aa/user_op_event.ts(1 hunks)sdk/src/client/contract_writer/aa_v06.ts(1 hunks)sdk/src/client/contract_writer/aa_v06_types.ts(1 hunks)sdk/src/client/contract_writer/eoa.ts(1 hunks)sdk/src/client/contract_writer/types.ts(1 hunks)sdk/src/client/index.ts(14 hunks)sdk/src/client/prepare.ts(0 hunks)sdk/src/client/services/Erc20Service.ts(7 hunks)sdk/src/client/services/NitroliteService.ts(25 hunks)sdk/src/client/signer.ts(2 hunks)sdk/src/client/state.ts(3 hunks)sdk/src/client/types.ts(3 hunks)sdk/src/errors.ts(2 hunks)sdk/test/unit/client/index.test.ts(14 hunks)sdk/test/unit/client/services/Erc20Service.test.ts(2 hunks)sdk/test/unit/client/services/NitroliteService.test.ts(3 hunks)sdk/test/unit/client/state.test.ts(4 hunks)
💤 Files with no reviewable changes (1)
- sdk/src/client/prepare.ts
✅ Files skipped from review due to trivial changes (1)
- sdk/src/abis/aa/user_op_event.ts
🚧 Files skipped from review as they are similar to previous changes (4)
- sdk/src/errors.ts
- sdk/src/client/contract_writer/eoa.ts
- sdk/src/client/contract_writer/aa_v06_types.ts
- sdk/src/abis/aa/execute.ts
🧰 Additional context used
🧠 Learnings (1)
📚 Learning: 2025-11-05T10:19:06.155Z
Learnt from: dimast-x
Repo: erc7824/nitrolite PR: 411
File: clearnode/custody.go:449-465
Timestamp: 2025-11-05T10:19:06.155Z
Learning: In clearnode/custody.go's handleResized function during positive resize operations (deposits), the ledger flow is: on-chain funds first credit the channel account, are immediately debited from the channel account, then credit the unified wallet account. The channel account acts as a pass-through for deposits. The unified wallet account is where actual deposited funds are held. This is recorded as a deposit transaction from channelAccountID to walletAccountID.
Applied to files:
sdk/src/client/services/NitroliteService.ts
🧬 Code graph analysis (11)
sdk/test/unit/client/services/NitroliteService.test.ts (2)
sdk/src/client/services/NitroliteService.ts (1)
NitroliteService(63-980)sdk/src/errors.ts (1)
Errors(372-399)
sdk/src/client/contract_writer/types.ts (1)
clearnode/pkg/sign/sign.go (1)
Address(29-34)
sdk/test/unit/client/services/Erc20Service.test.ts (2)
sdk/src/client/services/Erc20Service.ts (1)
Erc20Service(56-220)sdk/src/errors.ts (1)
Errors(372-399)
sdk/src/client/services/Erc20Service.ts (3)
sdk/src/client/contract_writer/types.ts (2)
ContractWriter(3-6)ContractCallParams(8-35)sdk/src/client/contract_writer/eoa.ts (1)
EOAContractWriter(23-81)sdk/src/abis/token/index.ts (1)
Erc20Abi(6-34)
sdk/src/client/index.ts (3)
sdk/src/client/contract_writer/types.ts (3)
ContractWriter(3-6)CallsDetails(37-39)WriteResult(41-43)sdk/src/client/contract_writer/eoa.ts (1)
EOAContractWriter(23-81)sdk/src/client/services/Erc20Service.ts (1)
Erc20Service(56-220)
sdk/src/client/state.ts (1)
sdk/src/client/signer.ts (2)
SessionKeyStateSigner(106-128)AccountStateSigner(71-99)
sdk/src/client/types.ts (1)
sdk/src/client/contract_writer/types.ts (1)
ContractWriter(3-6)
sdk/src/client/signer.ts (3)
sdk/src/utils/state.ts (1)
getPackedState(10-35)clearnode/pkg/sign/sign.go (1)
Address(29-34)sdk/src/client/types.ts (1)
State(83-85)
integration/tests/challenge_channel.test.ts (1)
integration/common/ws.ts (1)
getChannelUpdatePredicateWithStatus(181-191)
sdk/src/client/contract_writer/aa_v06.ts (5)
sdk/src/client/contract_writer/aa_v06_types.ts (2)
BundlerClientV06(26-30)PartialUserOperationV06(4-15)sdk/src/client/contract_writer/types.ts (4)
ContractWriter(3-6)CallsDetails(37-39)WriteResult(41-43)ContractCallParams(8-35)sdk/src/errors.ts (1)
Errors(372-399)sdk/src/abis/aa/user_op_event.ts (1)
UserOpEventAbi(3-51)sdk/src/abis/aa/execute.ts (1)
AAExecuteAbi(1-78)
sdk/src/client/services/NitroliteService.ts (4)
sdk/src/client/contract_writer/types.ts (2)
ContractWriter(3-6)ContractCallParams(8-35)sdk/src/client/contract_writer/eoa.ts (1)
EOAContractWriter(23-81)sdk/src/errors.ts (1)
Errors(372-399)sdk/src/abis/generated.ts (1)
custodyAbi(2-1567)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: Test (Integration) / Test Integration
🔇 Additional comments (22)
integration/tests/challenge_channel.test.ts (2)
8-8: LGTM: Import consolidation improves readability.The consolidation of the import statement is a minor formatting improvement with no functional impact.
53-57: LGTM: Pre-challenge listener setup prevents race conditions.Setting up the WebSocket listener before invoking
challengeChannelis the correct pattern. This ensures the listener is ready to capture theChallengedstatus update as soon as it's emitted, preventing a race condition where the message could arrive before the listener is established.sdk/test/unit/client/services/Erc20Service.test.ts (2)
16-27: LGTM! Test setup correctly mocks transaction receipt waiting.The addition of
waitForTransactionReceiptmock properly supports the new ContractWriter-based write flow.
124-127: LGTM! Error handling test correctly updated.The test properly validates that
ContractWriterRequiredErroris thrown when no contractWriter is provided, aligning with the architectural shift to ContractWriter-based writes.sdk/test/unit/client/state.test.ts (2)
29-52: LGTM! Test setup properly wires Account-based signing.The addition of
signMessageto the account mock correctly enables the AccountStateSigner path introduced in this PR.
54-59: Verify participant address alignment with signing logic.The
defaultChannel.participants[0]was changed fromdeps.account.address('0xOWNER') to'0xWALLET'. This should align with the signer selection logic in_checkParticipantAndGetSigner(Line 202 in state.ts), which compares the participant todeps.account.address.Ensure this test change correctly reflects the intended behavior:
- If the participant is '0xWALLET' but
deps.account.addressis '0xOWNER', the condition on Line 202 won't match- Verify this is the intended test scenario
sdk/test/unit/client/services/NitroliteService.test.ts (2)
28-44: LGTM! Test setup correctly mocks ContractWriter.The
mockContractWriteris properly wired intoNitroliteServiceconstructor, enabling testing of the new batched write flow.
159-180: LGTM! Test assertions correctly validate ContractWriter flow.The test properly validates:
- Batched calls via
{ calls: [...] }parameter- WriteResult return shape with
{ txHashes: [...] }- Error handling with
ContractWriterRequiredErrorAll changes align with the ContractWriter interface and batched write pattern.
sdk/src/client/index.ts (4)
64-76: LGTM! Constructor properly handles walletClient vs contractWriter.The constructor correctly:
- Enforces mutual exclusivity between
walletClientandcontractWriter- Creates an
EOAContractWriterwhen using legacywalletClientpath- Validates required parameters
- Derives the account appropriately for both paths
78-85: LGTM! Service construction correctly uses ContractWriter.Both
NitroliteServiceandErc20Serviceare properly initialized with thecontractWriter, migrating away from the legacywalletClientpattern.
107-128: LGTM! Deposit method correctly implements batched writes.The method properly:
- Batches ERC20 approval (if needed) with deposit in a single operation
- Uses the
prepareXCallParamspattern for consistency- Extracts the relevant transaction hash from the batch result
454-460: LGTM! Helper correctly extracts transaction hash.The
_getLastTxHashFromWriteResulthelper appropriately:
- Validates that at least one transaction hash exists
- Returns the last hash for sequential operations
Note: For EOA-based writes (which execute sequentially), this correctly returns the final transaction. For AA batched operations that execute atomically, verify that the bundler returns the appropriate hash in the last position.
sdk/src/client/types.ts (1)
98-149: LGTM! Excellent type design for configuration variants.The restructured configuration types properly:
- Extract common configuration to
BaseNitroliteClientConfigto avoid duplication- Use discriminated union with
nevertypes to enforce mutual exclusivity betweenwalletClientandcontractWriter- Mark the legacy
walletClientpath as deprecated with clear guidance- Provide a clean public API via the
NitroliteClientConfigunion typeThis design ensures compile-time type safety and provides a clear migration path for users.
sdk/test/unit/client/index.test.ts (2)
44-47: LGTM! Mock setup correctly simulates ContractWriter behavior.The mock returns
{ txHashes: ['0xTXHASH'] }which aligns with theWriteResultinterface and the_getLastTxHashFromWriteResulthelper in the client.
168-188: LGTM! Channel creation test properly validates the new flow.The test correctly:
- Mocks
_prepareAndSignInitialStateto return channel state- Verifies
prepareCreateChannelCallParamsis called with correct arguments- Checks
contractWriter.writereceives the prepared call- Validates the returned result structure with
txHash: '0xTXHASH'sdk/src/client/services/Erc20Service.ts (2)
152-170: LGTM! Clean implementation of prepareApproveCallParams.The method correctly prepares call parameters for ERC20 approval, following the same pattern used in NitroliteService. The JSDoc documentation is adequate.
21-50: LGTM! executeWriteContract properly delegates to ContractWriter.The function correctly:
- Wraps the request in a calls array
- Invokes the writer's
writemethod- Validates that txHashes are returned
- Returns the last transaction hash
sdk/src/client/contract_writer/aa_v06.ts (2)
39-50: LGTM! Constructor properly validates required parameters.The constructor correctly validates all required configuration properties (
publicClient,smartAccount,bundlerClient) and sets sensible defaults for optional polling parameters.
182-196: LGTM! Batch encoding follows the expected smart account execute pattern.The method correctly encodes calls into the
executeBatchformat expected by the smart account.sdk/src/client/services/NitroliteService.ts (3)
187-206: LGTM! prepareDepositCallParams correctly handles ETH deposits.The method properly:
- Extracts account address from Account or Address type
- Sets
valuetoamountfor ETH deposits (when tokenAddress is zeroAddress)- Returns well-structured ContractCallParams
238-250: LGTM! deposit method follows the correct ContractWriter pattern.The method properly:
- Obtains the contractWriter and account
- Prepares the request via
prepareDeposit- Executes via
executeWriteContract- Handles errors by re-wrapping non-NitroliteError exceptions
This pattern is consistently applied across all write methods.
140-156: LGTM! State conversion correctly handles ABI type requirements.The
convertStateForABImethod properly converts mutable arrays to readonly arrays and handles thesigsfallback with correct typing.
Summary by CodeRabbit
New Features
Chores
Tests
✏️ Tip: You can customize this high-level summary in your review settings.