AI PR #69
AI PR #69
Conversation
WalkthroughThis update introduces consistent and structured error handling across the SDK by replacing generic errors with a custom Changes
Sequence Diagram(s)sequenceDiagram
participant User
participant SessionKeyValidator
participant ErrorHandler
participant API/KeyStore
User->>SessionKeyValidator: enableSessionKey/rotateSessionKey/disableSessionKey(...)
SessionKeyValidator->>SessionKeyValidator: Validate inputs
alt Invalid input
SessionKeyValidator->>ErrorHandler: throw ErrorHandler(code=1, message)
ErrorHandler-->>User: Error thrown
else Valid input
SessionKeyValidator->>API/KeyStore: Fetch context or generate session key data
API/KeyStore-->>SessionKeyValidator: Response / Data
alt API/KeyStore error
SessionKeyValidator->>ErrorHandler: throw ErrorHandler(code=1, message)
ErrorHandler-->>User: Error thrown
else Success
SessionKeyValidator-->>User: Return session key data
end
end
Suggested reviewers
Poem
Warning There were issues while running some tools. Please review the errors and either fix the tool's configuration or disable the tool if it's a critical failure. 🔧 ESLint
src/sdk/common/getInitData.tsOops! Something went wrong! :( ESLint: 8.28.0 ReferenceError: Cannot read config file: /.eslintrc.js src/sdk/SessionKeyValidator/SessionKeyValidator.tsOops! Something went wrong! :( ESLint: 8.28.0 ReferenceError: Cannot read config file: /.eslintrc.js src/sdk/base/BaseAccountAPI.tsOops! Something went wrong! :( ESLint: 8.28.0 ReferenceError: Cannot read config file: /.eslintrc.js
✨ Finishing Touches
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
SupportNeed help? Create a ticket on our support page for assistance with any issues or questions. Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
|
Yooo! You forgot to bump the version in package.json! |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🔭 Outside diff range comments (1)
src/sdk/SessionKeyValidator/SessionKeyValidator.ts (1)
186-263: Breaking change: Parameter order modifiedThe
rotateSessionKeymethod has been simplified and the parameter order changed witholdSessionKeymoved to the first position. This is a breaking change that will require updates to any existing code using this method.The simplification (removing user operation handling) improves separation of concerns.
🧹 Nitpick comments (4)
src/sdk/common/service.ts (1)
68-78: Consider using a Set for better performance with many subscriptions.While the duplicate prevention logic is correct, using
includes()has O(n) complexity for each subscription check. For services with many subscriptions, this could impact performance.Consider using a Set to track subscriptions for O(1) lookup:
- private subscriptions: Subscription[] = []; + private subscriptions = new Set<Subscription>(); protected addSubscriptions(...subscriptions: Subscription[]): void { for (const sub of subscriptions) { - if (sub && !this.subscriptions.includes(sub)) { - this.subscriptions.push(sub); + if (sub && !this.subscriptions.has(sub)) { + this.subscriptions.add(sub); } } } protected removeSubscriptions(): void { - for (const subscription of this.subscriptions) { + for (const subscription of this.subscriptions) { subscription.unsubscribe(); } - this.subscriptions = []; + this.subscriptions.clear(); }src/sdk/common/utils/deep-compare.ts (1)
58-60: Consider providing more context in the error message.The error handling is good, but the message could be more descriptive for debugging.
} else { - throw new Error('deepCompare: Mismatched array/object types'); + throw new Error(`deepCompare: Type mismatch - comparing ${aIsArray ? 'array' : 'object'} with ${bIsArray ? 'array' : 'object'}`); }src/sdk/common/getInitData.ts (1)
49-54: Consider adding runtime type validation for the decoded arguments.While the error handling is comprehensive, the type assertions could potentially hide runtime type errors.
Consider validating the types before assertion:
return { - validators: initCallDataArgs[0] as Module[], - executors: initCallDataArgs[1] as Module[], - hooks: [initCallDataArgs[2]] as Module[], - fallbacks: initCallDataArgs[3] as Module[], + validators: Array.isArray(initCallDataArgs[0]) ? initCallDataArgs[0] as Module[] : throw new ErrorHandler('Invalid validators format', 1), + executors: Array.isArray(initCallDataArgs[1]) ? initCallDataArgs[1] as Module[] : throw new ErrorHandler('Invalid executors format', 1), + hooks: [initCallDataArgs[2]] as Module[], + fallbacks: Array.isArray(initCallDataArgs[3]) ? initCallDataArgs[3] as Module[] : throw new ErrorHandler('Invalid fallbacks format', 1), }Alternatively, create a validation helper function to keep the code clean.
src/sdk/base/HttpRpcClient.ts (1)
73-73: Apply optional chaining for safer property accessThe static analysis tool correctly identifies that optional chaining should be used here for safer property access.
Apply this fix:
- if (body && body?.details && body?.code) { + if (body?.details && body?.code) {
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (12)
src/sdk/SessionKeyValidator/SessionKeyValidator.ts(12 hunks)src/sdk/base/BaseAccountAPI.ts(3 hunks)src/sdk/base/EtherspotWalletAPI.ts(4 hunks)src/sdk/base/HttpRpcClient.ts(1 hunks)src/sdk/common/OperationUtils.ts(2 hunks)src/sdk/common/getInitData.ts(1 hunks)src/sdk/common/rxjs/error.subject.ts(1 hunks)src/sdk/common/service.ts(3 hunks)src/sdk/common/utils/deep-compare.ts(3 hunks)src/sdk/common/utils/hashing-utils.ts(1 hunks)src/sdk/sdk.ts(4 hunks)src/sdk/wallet/providers/key.wallet-provider.ts(4 hunks)
🧰 Additional context used
🧬 Code Graph Analysis (9)
src/sdk/common/OperationUtils.ts (1)
src/sdk/types/user-operation-types.ts (1)
BaseAccountUserOperationStruct(6-16)
src/sdk/base/BaseAccountAPI.ts (1)
src/sdk/errorHandler/errorHandler.service.ts (1)
ErrorHandler(3-17)
src/sdk/common/getInitData.ts (1)
src/sdk/errorHandler/errorHandler.service.ts (1)
ErrorHandler(3-17)
src/sdk/sdk.ts (3)
src/sdk/index.ts (1)
ModularSdk(11-11)src/sdk/errorHandler/errorHandler.service.ts (1)
ErrorHandler(3-17)src/sdk/common/exceptions/validation.exception.ts (1)
ValidationException(4-17)
src/sdk/common/service.ts (1)
src/sdk/context.ts (1)
Context(5-33)
src/sdk/common/utils/deep-compare.ts (1)
src/sdk/types/bignumber.ts (1)
BigNumber(49-303)
src/sdk/base/HttpRpcClient.ts (1)
src/sdk/errorHandler/errorHandler.service.ts (1)
ErrorHandler(3-17)
src/sdk/common/utils/hashing-utils.ts (1)
src/sdk/common/utils/viem-utils.ts (1)
isAddress(140-158)
src/sdk/SessionKeyValidator/SessionKeyValidator.ts (4)
src/sdk/errorHandler/errorHandler.service.ts (1)
ErrorHandler(3-17)src/sdk/SessionKeyValidator/interfaces.ts (1)
SessionKeyResponse(19-22)src/sdk/network/network.service.ts (1)
chainId(47-49)src/sdk/SessionKeyValidator/constants.ts (1)
PERMISSIONS_URL(1-1)
🪛 Biome (1.9.4)
src/sdk/common/OperationUtils.ts
[error] 26-26: Avoid the use of spread (...) syntax on accumulators.
Spread syntax should be avoided on accumulators (like those in .reduce) because it causes a time complexity of O(n^2).
Consider methods such as .splice or .push instead.
(lint/performance/noAccumulatingSpread)
src/sdk/base/HttpRpcClient.ts
[error] 73-73: Change to an optional chain.
Unsafe fix: Change to an optional chain.
(lint/complexity/useOptionalChain)
🔇 Additional comments (31)
src/sdk/common/service.ts (1)
4-17: Documentation improvements look good!The JSDoc comments clearly describe the Service class purpose and the idempotent behavior of lifecycle methods.
src/sdk/common/OperationUtils.ts (1)
6-12: Type safety improvements are excellent!The JSDoc documentation and specific return type
Promise<Record<string, string>>provide better clarity and type safety.src/sdk/base/BaseAccountAPI.ts (1)
252-276: Ensure robust extraction ofsenderfrom simulateContract errorsWe only found this single instance of
simulateContracterror handling insrc/sdk/base/BaseAccountAPI.ts, and no other patterns or examples in the codebase. To guard against differences in error shapes across RPC providers or viem versions, add defensive fallbacks when extractingsender:• File to update:
- src/sdk/base/BaseAccountAPI.ts (method
getCounterFactualAddress, lines 252–276)• Suggested diff:
} catch (e: any) { - if (e?.errorArgs?.sender) { - return e.errorArgs.sender; + // Handle different error payloads from simulateContract reverts + const sender = + e?.errorArgs?.sender ?? + e?.data?.args?.sender ?? + e?.error?.data?.args?.sender; + if (sender) { + return sender; + } throw new ErrorHandler( `Failed to get counterfactual address: ${e instanceof Error ? e.message : String(e)}`, 1 ); }Please verify against your target RPC providers and viem version that these fallback paths correctly capture the reverted
senderfield.src/sdk/common/utils/deep-compare.ts (1)
5-11: Excellent type safety improvements!The change from
anytounknownparameters and comprehensive JSDoc documentation enhance type safety and API clarity.src/sdk/sdk.ts (4)
24-24: LGTM: Proper import for enhanced validationThe import of
ValidationExceptionsupports the new batch validation logic in theestimatemethod.
44-47: LGTM: Well-defined batch size limitThe
MAX_BATCH_SIZEconstant provides a clear limit for user operation batches. The value of 50 seems reasonable for preventing resource exhaustion while allowing sufficient batch operations.
176-180: LGTM: Improved documentationThe enhanced JSDoc clearly describes the method's behavior and failure conditions, improving developer experience.
193-198: LGTM: Comprehensive batch validation logicThe validation improvements are excellent:
- Prevents processing batches exceeding
MAX_BATCH_SIZE- Ensures array length consistency between
to,data, andvaluearrays- Uses appropriate error types (
ErrorHandlerfor size limits,ValidationExceptionfor structural issues)The validation logic is robust and follows the SDK's error handling patterns.
src/sdk/wallet/providers/key.wallet-provider.ts (5)
5-5: LGTM: Proper import for standardized error handlingThe
ErrorHandlerimport supports the consistent error handling pattern being implemented across the SDK.
7-9: LGTM: Enhanced documentationThe improved JSDoc comments provide clear information about the class purpose and constructor parameters.
Also applies to: 17-22
24-26: LGTM: Important security validationThe private key validation prevents initialization with invalid keys, improving security and error handling. The check for both existence and string type is appropriate.
34-36: LGTM: Consistent error handling and useful propertyThe account validation uses
ErrorHandlerconsistently, and theaccountAddressproperty provides a convenient reference to the wallet address.Also applies to: 41-41
45-45: LGTM: Comprehensive error handling standardizationAll methods now use
ErrorHandlerwith appropriate error codes (1) instead of genericErrorobjects. The error messages are clear and consistent across all validation points:
- Account validation
- Required parameter validation (validatorAddress, factoryAddress, initCode)
This provides better error tracking and user experience.
Also applies to: 50-52, 68-68, 76-78, 101-101, 109-109, 119-119
src/sdk/common/rxjs/error.subject.ts (4)
6-6: LGTM: Improved type safetyChanging from
Subject<any>toSubject<Error>enforces type safety and ensures only Error instances are emitted.
18-25: LGTM: Robust error wrapping in next methodThe enhanced
nextmethod ensures type safety by:
- Only emitting actual Error instances
- Wrapping non-Error values in Error objects
- Maintaining backward compatibility
This prevents type-related issues in error handling.
31-31: LGTM: Enhanced error handling in wrap methodThe improvements include:
- Updated return type to
T | nullfor clarity- Consistent error wrapping for both synchronous and asynchronous errors
- Type checks before wrapping to preserve Error instances
The logic correctly handles Promise rejections and synchronous exceptions.
Also applies to: 36-39, 42-42
64-64: LGTM: Consistent error wrapping in catch methodThe
catchmethod now applies the same error wrapping logic as other methods, ensuring all emitted errors are proper Error instances.Also applies to: 73-73
src/sdk/common/utils/hashing-utils.ts (4)
6-11: LGTM: Comprehensive documentationThe enhanced JSDoc clearly describes the supported input types and return value, improving developer experience.
20-20: LGTM: Improved type safety and error handlingThe enhancements include:
- Using
isAHexfrom viem for better hex validation- Restricting object inputs to
Uint8ArrayandBufferonly- Clear error messages for unsupported types
- Warning before throwing for better debugging
The stricter validation prevents potential runtime errors and provides better feedback.
Also applies to: 27-41
47-55: LGTM: Simplified and more reliable hex validationThe updated
isHexfunction leverages the importedisAHexhelper, which is more reliable than custom validation logic.
63-92: LGTM: Enhanced robustness in toHexFromBytesLikeThe improvements include:
- Address-specific handling using
isAddress- Better string-to-bytes conversion with
stringToBytes- Stricter object type validation
- More descriptive error messages
The logic correctly handles different input types while maintaining type safety.
src/sdk/base/HttpRpcClient.ts (2)
67-78: LGTM: Standardized error handlingThe method now uses
ErrorHandlerconsistently with the SDK's error handling pattern. The fallback to error code 1 provides a reasonable default.
80-117: LGTM: Comprehensive error parsing improvementsThe enhanced error parsing logic is excellent:
- Handles viem-specific error types (BaseError, ContractFunctionExecutionError)
- Provides structured error objects with code, details, and data
- Includes fallback handling for unknown error types
- Maintains backward compatibility
This significantly improves error handling robustness across the RPC client.
src/sdk/base/EtherspotWalletAPI.ts (4)
10-10: LGTM!The import is correctly added as part of the SDK-wide error handling standardization.
302-348: Good error handling improvements!The enhanced error handling with specific error messages and the addition of try-catch for contract calls improves robustness. The JSDoc documentation is also helpful.
351-403: Excellent input validation and type handling!The added parameter validation and comprehensive BigNumberish type handling significantly improve the method's robustness. The explicit handling of all possible types with a fallback ensures proper value conversion.
414-480: Well-implemented batch encoding method!The new
encodeBatchmethod includes comprehensive validation:
- Proper null/undefined checks
- Array length equality validation
- Non-empty array validation
- Per-element type validation
The implementation correctly handles batch operations and aligns with the SDK's batch processing enhancements.
src/sdk/SessionKeyValidator/SessionKeyValidator.ts (4)
8-8: LGTM!ErrorHandler import aligns with SDK-wide error handling standardization.
38-131: Good error handling standardization!The method now uses ErrorHandler consistently with proper error messages and includes a well-structured error re-throw pattern that preserves ErrorHandler instances.
133-183: Verify the session key disabling logicThe new
disableSessionKeymethod follows good patterns for validation and error handling. However, it callsgenerateSessionKeyDatawith empty values for token, functionSelector, and zero values for timestamps.Please verify that passing empty/zero values to the backend API actually disables the session key as intended.
364-556: Consistent error handling improvementsAll private methods now use ErrorHandler consistently with error code 1, completing the error handling standardization across the SessionKeyValidator class.
| .reduce( | ||
| (set, [k, v]) => ({ | ||
| ...set, | ||
| [k]: v, | ||
| }), | ||
| {}, | ||
| {} as Record<string, string>, | ||
| ), |
There was a problem hiding this comment.
Fix O(n²) complexity issue with spread syntax in reduce.
The spread syntax in the reduce accumulator creates a new object on each iteration, resulting in O(n²) time complexity.
Use Object.fromEntries for better performance:
export async function toJSON(op: Partial<BaseAccountUserOperationStruct>): Promise<Record<string, string>> {
return resolveProperties(op).then((userOp) =>
- Object.keys(userOp)
- .map((key) => {
- let val = (userOp as any)[key];
- if (typeof val === 'object' && BigNumber.isBigNumber(val)) {
- val = val.toHexString()
- }
- else if (typeof val !== 'string' || !val.startsWith('0x')) {
- val = toHex(val);
- }
- return [key, val as string];
- })
- .reduce(
- (set, [k, v]) => ({
- ...set,
- [k]: v,
- }),
- {} as Record<string, string>,
- ),
+ Object.fromEntries(
+ Object.keys(userOp).map((key) => {
+ let val = (userOp as any)[key];
+ if (typeof val === 'object' && BigNumber.isBigNumber(val)) {
+ val = val.toHexString()
+ }
+ else if (typeof val !== 'string' || !val.startsWith('0x')) {
+ val = toHex(val);
+ }
+ return [key, val as string];
+ })
+ ),
);
}📝 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.
| .reduce( | |
| (set, [k, v]) => ({ | |
| ...set, | |
| [k]: v, | |
| }), | |
| {}, | |
| {} as Record<string, string>, | |
| ), | |
| export async function toJSON(op: Partial<BaseAccountUserOperationStruct>): Promise<Record<string, string>> { | |
| return resolveProperties(op).then((userOp) => | |
| Object.fromEntries( | |
| Object.keys(userOp).map((key) => { | |
| let val = (userOp as any)[key]; | |
| if (typeof val === 'object' && BigNumber.isBigNumber(val)) { | |
| val = val.toHexString(); | |
| } else if (typeof val !== 'string' || !val.startsWith('0x')) { | |
| val = toHex(val); | |
| } | |
| return [key, val as string]; | |
| }) | |
| ), | |
| ); | |
| } |
🧰 Tools
🪛 Biome (1.9.4)
[error] 26-26: Avoid the use of spread (...) syntax on accumulators.
Spread syntax should be avoided on accumulators (like those in .reduce) because it causes a time complexity of O(n^2).
Consider methods such as .splice or .push instead.
(lint/performance/noAccumulatingSpread)
🤖 Prompt for AI Agents
In src/sdk/common/OperationUtils.ts around lines 24 to 30, the use of spread
syntax inside the reduce function causes O(n²) complexity by creating a new
object on each iteration. Replace the entire reduce call with a single call to
Object.fromEntries on the original array to convert it directly into a
Record<string, string>, which improves performance to O(n).
Description
Types of changes
What types of changes does your code introduce?
Further comments (optional)
Summary by CodeRabbit
New Features
Bug Fixes
Refactor
Documentation