feat: add swap+send STX typings #4634
Conversation
packages/user-operation-controller/src/UserOperationController.ts
Outdated
Show resolved
Hide resolved
| @@ -348,6 +349,17 @@ export type SwapsMetadata = { | |||
| /** Symbol of the destination token. */ | |||
| destinationTokenSymbol: string | null; | |||
|
|
|||
| approvalTxParams?: { | |||
There was a problem hiding this comment.
Same issue here, we already store the transactionParams and transactionType in the UserOperationMetadata so don't want to duplicate any state.
Plus won't the meta property be a duplicate of the other properties in this type?
Or a approvalUserOperationId if it's ultimately concerning an alternate user operation?
There was a problem hiding this comment.
The idea is that we are bundling the approval TX with the trade TX, so there wouldn't be an approval TX ID to reference in this case, since the user would be shown that approval as well. The goal is to have one confirmation screen for the trade that triggers the approval right before the trade is submitted – very similar to a callback/hook pattern. This thread provides good context towards the end: https://consensys.slack.com/archives/C068SFX90PN/p1720616402512989
As for duplication, when creating the transaction at the surface level (i.e., in the send ducks file), we pass the approvalTx as swaps metadata, and it is ultimately converted into transaction metadata in getTransactionMetadata() (core)
There was a problem hiding this comment.
I understand the goal from the client perspective is to have a single confirmation that triggers both the approval and trade transactions.
But from the core perspective, it still has to deal with them one at a time, meaning it still requires separate calls to addTransaction, so there will eventually be two transactions in the state.
So my query is why we need to couple the controller itself to this metadata, if it's ultimately the client orchestrating the process meaning the client can have state in the confirmation that knows it needs two transactions.
Ultimately, we don't want to couple the controller unnecessarily to client problems, so I'm not sure what benefit this new metadata would have?
There was a problem hiding this comment.
Ultimately, we don't want to couple the controller unnecessarily to client problems, so I'm not sure what benefit this new metadata would have?
The issue is that the controller manages the transaction that the confirmations component would handle, so this PR is a direct result of deep coupling. I do believe this is well within the scope of the controller, as this directly relates to how transactions are handled and presented to the confirmations component.
So my query is why we need to couple the controller itself to this metadata, if it's ultimately the client orchestrating the process meaning the client can have state in the confirmation that knows it needs two transactions.
This goes back to the point of keeping these coupled to mitigate the risk of any bug or edge cases where the approval is either cleared a) after the trade or b) not at all. Keeping this bundled until we are actually submitting the transactions prevents a lot of the race conditions that transactions are prone to.
There is an approach where we accomplish the same on the client side, but this would require a ton of confirmation refactors that would (rightfully) blow up the review/testing scope of the extension PR.
| @@ -357,6 +372,7 @@ function updateSwapTransaction( | |||
| * @param propsToUpdate.swapMetaData - Metadata of the swap | |||
| * @param propsToUpdate.swapTokenValue - Value of the token to be swapped – possibly the same as sourceTokenAmount; included for consistency | |||
| * @param propsToUpdate.type - Type of the transaction | |||
| * @param propsToUpdate.approvalTxParams - The parameters of the approval transaction | |||
| * @returns The updated transaction meta object. | |||
| */ | |||
| function updateSwapAndSendTransaction( | |||
There was a problem hiding this comment.
If the purpose of the approvalTxParams is ultimately to be included in this event, could we instead just include them in the addTransaction method options and pass them to updateSwapsTransaction then here and then include them as an alternate property in the event?
That way the event has the info, but we don't have to persist anything new?
There was a problem hiding this comment.
We need the approval TX for the confirmations screen in extension, not just this method. The idea is that we ensure it's always bundled with the tx, so that we don't lose it if the send state in extension is lost (ie the user closes the extension and reopens it)
There was a problem hiding this comment.
If the user closes the extension, then I believe we automatically reject the transaction.
more accurate since it contains both tx params and metadata
You need to link the branch in MetaMask/core#4634, so build and replace my name w your name, then once matt walsh is done yapping it up, get that merged and deployed, then finally replace all the linking with the latest npm version for the user-operations-controller and transaction-controller.
Explanation
Swap+Send currently bypasses both the confirmations and STX flow for the Extension, due to an inability to use a single confirmation page. This adds typing and handling for both a swap+send approval event and a new transaction property that tracks data for an approval that should be executed right before the transaction.
Both the transaction-controller and user-controller had to be updated here, as both contain typing relating to the transactions.
References
Relates to MetaMask/metamask-extension#25734
Changelog
@metamask/transaction-controllerSwapAndSendApprovaltyping and eventapprovalTxParamstransaction property and handling@metamask/user-operation-controllerapprovalTxParamstransaction property and handlingChecklist