feat: use eth_sendTransactionBatch for swaps requiring allowance bump #122
Conversation
meeh0w
force-pushed
the
feat/batch-approvals
branch
from
9436ddc to
2b8853c
Compare
| // Only wallets that provide us signed transactions without additional approvals | ||
| // can be used to sign transaction batches (so for example hardware wallets or accounts | ||
| // that connect through WalletConnect protocol would not work, since users have to approve | ||
| // each transaction one by one anyways. |
There was a problem hiding this comment.
I think we should add a follow up story and make the other other wallets capable of something similar.
i.e: After the user presses approve all, the transactions would display one after the other on the hw wallet without UI interaction. The Ledger dialog could display, "Approve transaction 1/2 ... (and thisplay the details)"
There was a problem hiding this comment.
| action: Action | MultiTxAction, | ||
| ): action is MultiTxAction => | ||
| action && | ||
| 'signingRequests' in action && |
There was a problem hiding this comment.
Should we use some sort of type or enum for this instead?
| displayData: DisplayData; | ||
| }; | ||
|
|
||
| export type MultiTxAction = Omit<Action, 'signingData' | 'displayData'> & { |
There was a problem hiding this comment.
Are regular transactions a batch of one? Do we need to keep two separate types? Should we just use the one that is capable of handling more and on the UI we just display the single vs batch UI based on the number of transactions needed to be approved?
There was a problem hiding this comment.
I'm open to doing that as a follow-up PR, as it would involve revamping the current (single) Action model on each handler we have, which would blow up this branch tremendously.
To be completely honest, I would like to revisit the Action model as a whole and require it to always be typed (for internal request handlers too) -- especially the data we use on the UI side.
| display: 'flex', | ||
| width: 320, | ||
| flex: '0 0 auto', | ||
| backgroundColor: 'grey.850', | ||
| ml: isFirst ? 3.5 : 0, | ||
| mr: isLast ? 3.5 : 0, | ||
| position: 'relative', |
There was a problem hiding this comment.
I know designs are like this so no action just a note. We are getting sooo far off from the component library.
| export const isBatchApprovalParams = ( | ||
| params: ApprovalParams | BatchApprovalParams, | ||
| ): params is BatchApprovalParams => { | ||
| return 'signingRequests' in params; |
There was a problem hiding this comment.
same here regarding using an enum or type instead.
| const isOneClickSwapSupported = | ||
| walletDetails?.type === SecretType.Mnemonic || | ||
| walletDetails?.type === SecretType.Seedless || | ||
| walletDetails?.type === SecretType.PrivateKey; | ||
|
|
||
| if (isOneClickSwapEnabled && isOneClickSwapSupported) { | ||
| return oneClickSwap(params); | ||
| } |
There was a problem hiding this comment.
Please add tests for the new on-click swap path as well.
Description
Notes for reviewers
ApprovalController.requestBatchApprovalmethod).useFeeCustomizer()hook so that it's able to update the fee settings on the entire transaction batch. + some small updates to UI components.SwapProvider- things like params validation, response validation, building transactions.oneClickSwap()method in theSwapProvider(using those previously extracted shared utils), hides it behind a feature flag and makes it so it's only called when the feature flag is enabled.TxBatchApprovalScreenand related components.Testing
Screenshots:
Screen.Recording.2025-01-17.at.12.39.15.mov
Checklist for the author