-
Notifications
You must be signed in to change notification settings - Fork 110
feat(breaking): Smart wallet upgrade #354
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: main
Are you sure you want to change the base?
Conversation
Graphite Automations"Request reviewers once CI passes on sdks monorepo" took an action on this PR • (04/14/25)1 reviewer was added and 1 assignee was added to this PR based on Siyu Jiang (See-You John)'s automation. |
} | ||
|
||
/** Internal methods */ | ||
|
||
protected static _encodeExecute(mode: ModeType, data: `0x${string}`): `0x${string}` { | ||
protected static _encodeERC7821Execute(mode: ModeType, data: `0x${string}`): `0x${string}` { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
imo we should be encoding calldata as a direct call to execute not through the 7821 wrapper!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
this is only supported for the legacy mode if desired, I marked it as deprecated do you think we should just remove it all together?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I mean I can't see our internal code using this codepath but I think its fair to support in the sdk !
const mode = this.getModeFromOptions(options) | ||
if(mode != ModeType.BATCHED_CALL && mode != ModeType.BATCHED_CALL_CAN_REVERT) { | ||
throw new Error(`Invalid mode: ${mode}`) | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
So yeah I think this entrypoint should just be the direct encoding, not 7821 encoding
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
so it seems like you can just remove these lines/not support a mode being passed in
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Agree that default should be via BatchedCall. I think its fine to use executeOptions here since that's what we use in other functions
Changelog
Breaking Changes
encodeCalls
method: This method has been completely removedencodeBatchedCall
is now the main method for encoding call batchesencodeCalls
(acceptscalls[]
andoptions
)MethodParameters
object type as beforeshouldRevert
in BatchedCallPlanner is nowtrue
, meaning batched calls will revert by default if any call failsAdded
BatchedCallPlanner
class to handle batched calls with revert controlencodeERC7821BatchedCall
method for legacy support of the ERC-7821 specificationencodeBatchedCall
methodMigration Guide
This update simplifies the API while maintaining backward compatibility through parameter structure. The main behavioral difference is that batched calls will now revert by default when any contained call fails, improving safety by preventing partial execution of transaction batches.
How Has This Been Tested?
[e.g. Manually, E2E tests, unit tests, Storybook]
Are there any breaking changes?
[e.g. Type definitions, API definitions]
If there are breaking changes, please ensure you bump the major version Bump the major version (by using the title
feat(breaking): ...
), post a notice in #eng-sdks, and explicitly notify all Uniswap Labs consumers of the SDK.