Skip to content
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.

Sign up for GitHub

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

Jump to bottom

feat: add swap+send STX typings #4634

Open
wants to merge 10 commits into
base: main
Choose a base branch
from
+139 −3
Open

feat: add swap+send STX typings #4634

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
10 commits
Select commit Hold shift + click to select a range
108cc0d
feat: enable approval pairing for transactions
BZahory Aug 21, 2024
c5d7ca7
chore: add `swapAndSendApproval` tx type
BZahory Aug 21, 2024
fad5bb5
test: update failing tests
BZahory Aug 21, 2024
6322aab
test: add testing case
BZahory Aug 21, 2024
d3de602
chore: remove unused fn name
BZahory Aug 22, 2024
e9d466a
test: remove asyncs
BZahory Aug 22, 2024
2af578a
style: order properties
BZahory Aug 22, 2024
747ea32
refactor: rename `approvalTxParams` to `approvalTx`
BZahory Aug 22, 2024
71ca4b6
chore: update fn name
BZahory Aug 22, 2024
1db56f5
test: update for refactors
BZahory Aug 22, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
Next Next commit
feat: enable approval pairing for transactions
  • Loading branch information
@BZahory
BZahory committed Aug 22, 2024
commit 108cc0d66b429685c1a3bb067a450073be098b90
11 changes: 11 additions & 0 deletions packages/transaction-controller/src/types.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -398,6 +398,17 @@ type TransactionMetaBase = {
*/
txParams: TransactionParams;

approvalTxParams?: {
matthewwalsh0 marked this conversation as resolved.
Show resolved Hide resolved
data?: TransactionParams;
params?: {
requireApproval?: boolean;
type?: TransactionType;
swaps?: {
meta?: Partial<TransactionMeta>;
};
};
};

/**
* Transaction receipt.
*/
Expand Down
3 changes: 3 additions & 0 deletions packages/transaction-controller/src/utils/swaps.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -357,6 +357,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(
Copy link
Member

@matthewwalsh0 matthewwalsh0 Aug 22, 2024 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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)

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If the user closes the extension, then I believe we automatically reject the transaction.

Expand All @@ -367,6 +368,7 @@ function updateSwapAndSendTransaction(
destinationTokenAmount,
destinationTokenDecimals,
destinationTokenSymbol,
approvalTxParams,
estimatedBaseFee,
sourceTokenAddress,
sourceTokenAmount,
Expand All @@ -386,6 +388,7 @@ function updateSwapAndSendTransaction(
destinationTokenAmount,
destinationTokenDecimals,
destinationTokenSymbol,
approvalTxParams,
estimatedBaseFee,
sourceTokenAddress,
sourceTokenAmount,
Expand Down
11 changes: 11 additions & 0 deletions packages/user-operation-controller/src/UserOperationController.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -146,6 +146,16 @@ export type AddUserOperationSwapOptions = {
destinationTokenAmount?: string;
destinationTokenDecimals?: number;
destinationTokenSymbol?: string;
approvalTxParams?: {
data: TransactionParams;
params?: {
requireApproval: boolean;
type: TransactionType;
swaps?: {
meta?: Partial<TransactionMeta>;
};
};
};
estimatedBaseFee?: string;
sourceTokenAddress?: string;
sourceTokenAmount?: string;
Expand Down Expand Up @@ -451,6 +461,7 @@ export class UserOperationController extends BaseController<
destinationTokenAmount: swaps.destinationTokenAmount ?? null,
destinationTokenDecimals: swaps.destinationTokenDecimals ?? null,
destinationTokenSymbol: swaps.destinationTokenSymbol ?? null,
approvalTxParams: swaps.approvalTxParams ?? null,
BZahory marked this conversation as resolved.
Show resolved Hide resolved
estimatedBaseFee: swaps.estimatedBaseFee ?? null,
sourceTokenAddress: swaps.sourceTokenAddress ?? null,
sourceTokenAmount: swaps.sourceTokenAmount ?? null,
Expand Down
12 changes: 12 additions & 0 deletions packages/user-operation-controller/src/types.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,4 +1,5 @@
import type {
TransactionMeta,
TransactionParams,
TransactionType,
UserFeeLevel,
Expand Down Expand Up @@ -348,6 +349,17 @@ export type SwapsMetadata = {
/** Symbol of the destination token. */
destinationTokenSymbol: string | null;

approvalTxParams?: {
Copy link
Member

@matthewwalsh0 matthewwalsh0 Aug 22, 2024 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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)

Copy link
Member

@matthewwalsh0 matthewwalsh0 Aug 23, 2024 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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?

Copy link
Contributor Author

@BZahory BZahory Aug 23, 2024 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

data: TransactionParams;
params?: {
requireApproval?: boolean;
type?: TransactionType;
swaps?: {
meta?: Partial<TransactionMeta>;
};
};
} | null;

BZahory marked this conversation as resolved.
Show resolved Hide resolved
/** Amount of the destination token. */
destinationTokenAmount: string | null;

Expand Down
1 change: 1 addition & 0 deletions packages/user-operation-controller/src/utils/transaction.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -106,6 +106,7 @@ export function getTransactionMetadata(

const swaps = {
approvalTxId: swapsMetadata?.approvalTxId ?? undefined,
approvalTxParams: swapsMetadata?.approvalTxParams ?? undefined,
destinationTokenAddress:
swapsMetadata?.destinationTokenAddress ?? undefined,
destinationTokenAmount: swapsMetadata?.destinationTokenAmount ?? undefined,
Expand Down
1 change: 1 addition & 0 deletions packages/user-operation-controller/src/utils/validation.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -68,6 +68,7 @@ export function validateAddUserOperationOptions(
swaps: optional(
object({
approvalTxId: optional(string()),
approvalTxParams: optional(object()),
destinationTokenAddress: optional(string()),
destinationTokenDecimals: optional(number()),
destinationTokenSymbol: optional(string()),
Expand Down