From d7d2151e18a0141c19f73804b84c553a39decf01 Mon Sep 17 00:00:00 2001 From: khanti42 Date: Mon, 25 Nov 2024 12:09:28 +0100 Subject: [PATCH] fix: improve Fee Token Selection UX with Disclaimer and Error Messaging (#433) * chore: add unit test for input controller * chore: remove unused mock * chore: update test * chore: update mock test * chore: update type * fix: improve readability on fee token selection and fallback mechanism * chore: handle pr comment review * fix: rollback request state in case of error in input-event-controller * chore: fix comments --------- Co-authored-by: stanleyyuen <102275989+stanleyyconsensys@users.noreply.github.com> --- .../starknet-snap/src/__tests__/helper.ts | 4 +++ .../user-input-event-controller.test.ts | 33 ++++++++++++------- .../user-input-event-controller.ts | 26 ++++++++++++--- .../src/ui/fragments/FeeTokenSelector.tsx | 5 +++ 4 files changed, 52 insertions(+), 16 deletions(-) diff --git a/packages/starknet-snap/src/__tests__/helper.ts b/packages/starknet-snap/src/__tests__/helper.ts index f0b6f319..6e63fc8b 100644 --- a/packages/starknet-snap/src/__tests__/helper.ts +++ b/packages/starknet-snap/src/__tests__/helper.ts @@ -22,6 +22,7 @@ import { v4 as uuidv4 } from 'uuid'; import { FeeToken } from '../types/snapApi'; import type { AccContract, + Erc20Token, Transaction, TransactionRequest, } from '../types/snapState'; @@ -298,11 +299,13 @@ export function generateTransactions({ export function generateTransactionRequests({ chainId, address, + selectedFeeToken, contractAddresses = PRELOADED_TOKENS.map((token) => token.address), cnt = 1, }: { chainId: constants.StarknetChainId | string; address: string; + selectedFeeToken?: Erc20Token; contractAddresses?: string[]; cnt?: number; }): TransactionRequest[] { @@ -330,6 +333,7 @@ export function generateTransactionRequests({ addressIndex: 0, maxFee: '100', selectedFeeToken: + selectedFeeToken?.symbol ?? feeTokens[Math.floor(generateRandomValue() * feeTokens.length)].symbol, calls: [ { diff --git a/packages/starknet-snap/src/ui/controllers/user-input-event-controller.test.ts b/packages/starknet-snap/src/ui/controllers/user-input-event-controller.test.ts index 17270a35..18209137 100644 --- a/packages/starknet-snap/src/ui/controllers/user-input-event-controller.test.ts +++ b/packages/starknet-snap/src/ui/controllers/user-input-event-controller.test.ts @@ -317,6 +317,7 @@ describe('UserInputEventController', () => { describe('handleFeeTokenChange', () => { const prepareHandleFeeTokenChange = async ( feeToken: FeeToken = FeeToken.STRK, + selectedFeeToken?: Erc20Token, ) => { const network = STARKNET_TESTNET_NETWORK; const { chainId } = network; @@ -325,6 +326,7 @@ describe('UserInputEventController', () => { const [transactionRequest] = generateTransactionRequests({ chainId, address: account.address, + selectedFeeToken, }); const event = generateInputEvent({ @@ -353,6 +355,10 @@ describe('UserInputEventController', () => { 'updates the transaction request with the updated estimated fee: feeToken - %symbol', async (token: Erc20Token) => { const feeToken = FeeToken[token.symbol]; + const selectedFeeToken = + token.symbol === FeeToken.ETH + ? STRK_SEPOLIA_TESTNET + : ETHER_SEPOLIA_TESTNET; const { event, account, @@ -363,7 +369,7 @@ describe('UserInputEventController', () => { mockGetEstimatedFeesResponse, upsertTransactionRequestSpy, transactionRequest, - } = await prepareHandleFeeTokenChange(feeToken); + } = await prepareHandleFeeTokenChange(feeToken, selectedFeeToken); const feeTokenAddress = token.address; const { signer, calls } = transactionRequest; const { publicKey, privateKey, address } = account; @@ -404,9 +410,10 @@ describe('UserInputEventController', () => { controller.eventId, transactionRequest, ); - expect(upsertTransactionRequestSpy).toHaveBeenCalledWith( - transactionRequest, - ); + expect(upsertTransactionRequestSpy).toHaveBeenCalledWith({ + ...transactionRequest, + selectedFeeToken: feeToken, + }); }, ); @@ -430,33 +437,37 @@ describe('UserInputEventController', () => { transactionRequest, { errors: { - fees: `Not enough ${feeToken} to pay for fee`, + fees: `Not enough ${feeToken} to pay for fee, switching back to ${transactionRequest.selectedFeeToken}`, }, }, ); }); - it('updates the transaction request with an general error message if other error was thrown.', async () => { + it('rollback the transaction request and show a general error message if other error was thrown.', async () => { const { event, - hasSufficientFundsForFeeSpy, transactionRequest, updateExecuteTxnFlowSpy, upsertTransactionRequestSpy, } = await prepareHandleFeeTokenChange(); // Simulate an error thrown to test the error handling - hasSufficientFundsForFeeSpy.mockRejectedValue(false); + upsertTransactionRequestSpy.mockRejectedValue(new Error('Failed!')); + const rollbackSnapshot = { + maxFee: transactionRequest.maxFee, + selectedFeeToken: transactionRequest.selectedFeeToken, + includeDeploy: transactionRequest.includeDeploy, + resourceBounds: [...transactionRequest.resourceBounds], + }; const controller = createMockController(event); await controller.handleFeeTokenChange(); - expect(upsertTransactionRequestSpy).not.toHaveBeenCalled(); expect(updateExecuteTxnFlowSpy).toHaveBeenCalledWith( controller.eventId, - transactionRequest, + { ...transactionRequest, ...rollbackSnapshot }, { errors: { - fees: `Fail to calculate the fees`, + fees: `Failed to calculate the fees, switching back to ${transactionRequest.selectedFeeToken}`, }, }, ); diff --git a/packages/starknet-snap/src/ui/controllers/user-input-event-controller.ts b/packages/starknet-snap/src/ui/controllers/user-input-event-controller.ts index 191f48fc..28520697 100644 --- a/packages/starknet-snap/src/ui/controllers/user-input-event-controller.ts +++ b/packages/starknet-snap/src/ui/controllers/user-input-event-controller.ts @@ -130,8 +130,20 @@ export class UserInputEventController { return token.address; } + protected createRollbackSnapshot( + request: TransactionRequest, + ): Partial { + return { + maxFee: request.maxFee, + selectedFeeToken: request.selectedFeeToken, + includeDeploy: request.includeDeploy, + resourceBounds: [...request.resourceBounds], + }; + } + protected async handleFeeTokenChange() { const request = this.context?.request as TransactionRequest; + const rollbackSnapshot = this.createRollbackSnapshot(request); const { addressIndex, calls, signer, chainId } = request; const feeToken = (this.event as InputChangeEvent) .value as unknown as FeeToken; @@ -192,13 +204,17 @@ export class UserInputEventController { } catch (error) { const errorMessage = error instanceof InsufficientFundsError - ? `Not enough ${feeToken} to pay for fee` - : 'Fail to calculate the fees'; + ? `Not enough ${feeToken} to pay for fee, switching back to ${request.selectedFeeToken}` + : `Failed to calculate the fees, switching back to ${request.selectedFeeToken}`; // On failure, display ExecuteTxnUI with an error message - await updateExecuteTxnFlow(this.eventId, request, { - errors: { fees: errorMessage }, - }); + await updateExecuteTxnFlow( + this.eventId, + { ...request, ...rollbackSnapshot }, + { + errors: { fees: errorMessage }, + }, + ); } } } diff --git a/packages/starknet-snap/src/ui/fragments/FeeTokenSelector.tsx b/packages/starknet-snap/src/ui/fragments/FeeTokenSelector.tsx index c8aca08f..8adcefff 100644 --- a/packages/starknet-snap/src/ui/fragments/FeeTokenSelector.tsx +++ b/packages/starknet-snap/src/ui/fragments/FeeTokenSelector.tsx @@ -3,6 +3,7 @@ import { Field, Form, Selector, + Text, SelectorOption, type SnapComponent, } from '@metamask/snaps-sdk/jsx'; @@ -47,6 +48,10 @@ export const FeeTokenSelector: SnapComponent = ({ + + If the chosen token has no funds, the system will automatically switch + to a funded token. + ); };