Conversation
kristiehuang
changed the title
Graphite Automations"Request reviewers once CI passes on sdks monorepo" took an action on this PR • (12/08/25)2 reviewers were added and 1 assignee was added to this PR based on Siyu Jiang (See-You John)'s automation. |
| it('handles calls with chainId property', () => { | ||
| const calls: Call[] = [ | ||
| { | ||
| to: '0x1111111111111111111111111111111111111111', | ||
| data: '0x1234', | ||
| value: 50n, | ||
| chainId: ChainId.SEPOLIA, | ||
| }, | ||
| ] | ||
|
|
||
| const result = SmartWallet.encodeUserOp(calls) | ||
|
|
||
| expect(result).toBeDefined() | ||
| expect(result.calldata.startsWith(EXECUTE_USER_OP_SELECTOR)).toBe(true) | ||
| expect(result.value).toBe(50n) | ||
| }) | ||
| }) |
There was a problem hiding this comment.
this test doesn't really make sense, should be "handles value properly" or check chainId somehow
There was a problem hiding this comment.
Agreed - this test doesn't add meaningful coverage since chainId isn't used in the encoding. Consider renaming to "handles value property" and removing the unused chainId, or removing the test entirely since value summation is already covered by the "sums the value of all calls" test.
| const result = SmartWallet.encodeUserOp(calls) | ||
|
|
||
| expect(result).toBeDefined() | ||
| expect(result.calldata).toBeDefined() |
There was a problem hiding this comment.
can we just add a quick test to check the to and data fields bc the other tests only check revertOnFailure not these other fields
There was a problem hiding this comment.
Agreed - the first test at line 16-31 (and this one) only check the selector and value. Adding assertions that decode and verify the to and data fields would make these tests more comprehensive, similar to how the revertOnFailure tests decode the full structure.
🤖 Claude Code Review
SummaryThis PR adds a new ReviewThe implementation looks correct:
Feedback on Existing Comments@zhongeric raised two valid points about test coverage that haven't been addressed in the current code:
These are reasonable improvements but not blocking issues. 💡 Want a fresh review? Add a comment containing |
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
There was a problem hiding this comment.
📋 Review verdict: APPROVE
👆 The main review comment above is the source of truth for this PR review. It is automatically updated on each review cycle, so always refer to it for the most current feedback.
This formal review submission is for the verdict only.
3 participants
Description
needed new encodeUserOp function in the smart wallet SDK for userops
How Has This Been Tested?
manually ported code over in trading api & successfully encoded a swap userop that was executed on chain: https://etherscan.io/tx/0x5b22264fa784e2bbd15f97b9f7a4aa9261cf09627465e992e1a4e8c11d421e3a
unit tests
Are there any breaking changes?
no breaking changes, just adding a new fn
✨ Claude-Generated Content
Description
needed new encodeUserOp function in the smart wallet SDK for userops
How Has This Been Tested?
Are there any breaking changes?
no breaking changes, just adding a new fn
✨ Claude-Generated Content
Description
Adds a new
encodeUserOpstatic method to theSmartWalletclass for encoding UserOperation calldata compatible with ERC-4337 EntryPoint v0.7.0 and v0.8.0. This enables the smart wallet SDK to generate calldata for account abstraction user operations.Changes
SmartWallet.encodeUserOp(calls, options)method that:Callobjects and optionalExecuteOptionsCallPlannerandBatchedCallPlannerto encode the callsexecuteUserOpselector (0x8dd7712f) to the encoded dataMethodParameterswith calldata and aggregated valuerevertOnFailureoption (true/false/default)How Has This Been Tested?
Are there any breaking changes?
No - this is a purely additive change that adds a new method without modifying existing functionality.