diff --git a/packages/stellar-wallet-snap/CHANGELOG.md b/packages/stellar-wallet-snap/CHANGELOG.md index 06ce9dd52..da836fdcf 100644 --- a/packages/stellar-wallet-snap/CHANGELOG.md +++ b/packages/stellar-wallet-snap/CHANGELOG.md @@ -26,6 +26,10 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0 ### Fixed +- Stop the confirmation refresh cron when transaction re-validation fails ([#282](https://github.com/MetaMask/internal-snaps/pull/282)) + - Show the mapped transaction error banner + - Skip the security scan for the invalid transaction + - Do not reschedule further refresh cycles - Fill contract-based receive transactions in history instead of marking them as unknown ([#255](https://github.com/MetaMask/internal-snaps/pull/255)) ## [0.1.0] diff --git a/packages/stellar-wallet-snap/docs/use-cases/cron-job/refreshConfirmationContext.md b/packages/stellar-wallet-snap/docs/use-cases/cron-job/refreshConfirmationContext.md index 92a66ad30..e877296e6 100644 --- a/packages/stellar-wallet-snap/docs/use-cases/cron-job/refreshConfirmationContext.md +++ b/packages/stellar-wallet-snap/docs/use-cases/cron-job/refreshConfirmationContext.md @@ -36,16 +36,14 @@ Fetches / updates token spot prices shown on the confirmation (fee asset, send a ### Security scan -Runs (or refreshes) the remote security scan on the current transaction envelope in context. Uses the **latest** envelope when the transaction refresher has already patched it this cycle. +Runs (or refreshes) the remote security scan on `securityScanRequest` in context. Uses the **rebuilt** envelope when the transaction refresher already patched it this cycle. ### Transaction rebuild -Runs **first** when enabled: +Runs **first** when enabled. Rebuilds from the original request against a live on-chain account (fresh fee, sequence, time bounds, destination activation). Confirm-time send / change-trust rebuilds again before signing; this cycle does not patch the stored confirmation `transaction` XDR. -1. Resolve live on-chain account. -2. Rebuild the pending send / change-trust envelope (fresh fee, sequence, time bounds). -3. Re-validate locally; update fee / validation status in context. -4. Write the rebuilt XDR into the security-scan request so the scan refresher does not scan a stale snapshot. +- **Success** — write the rebuilt XDR into `securityScanRequest` so scan does not use a stale snapshot. +- **Failure** — set `transactionsFetchStatus` to error, set mapped `errorMessage` for the confirmation banner, and set `scanFetchStatus` to error, so scan is skipped. ## Step-by-step (one cycle) @@ -72,7 +70,7 @@ sequenceDiagram else still open opt Transaction in refresherKeys Cron->>TxR: rebuild + validate (live) - TxR-->>Cron: patch (xdr, fee, status) + TxR-->>Cron: patch (scan xdr, or error banner) end par Cron->>Price: refresh spot prices diff --git a/packages/stellar-wallet-snap/src/handlers/clientRequest/utils.ts b/packages/stellar-wallet-snap/src/handlers/clientRequest/utils.ts index 1c437e583..3e0e48d56 100644 --- a/packages/stellar-wallet-snap/src/handlers/clientRequest/utils.ts +++ b/packages/stellar-wallet-snap/src/handlers/clientRequest/utils.ts @@ -56,7 +56,7 @@ export function assertRefreshedTransactionFeeNotHigher(params: { * @returns The localized message key for the banner subtitle. */ export function getTxnErrorMessageKey( - error: TransactionValidationException, + error: unknown, senderAddress: string, ): LocalizedMessage { if (error instanceof InsufficientBalanceException) { diff --git a/packages/stellar-wallet-snap/src/handlers/cronjob/refreshConfirmationContext/api.ts b/packages/stellar-wallet-snap/src/handlers/cronjob/refreshConfirmationContext/api.ts index eed3851f1..6c1a487dd 100644 --- a/packages/stellar-wallet-snap/src/handlers/cronjob/refreshConfirmationContext/api.ts +++ b/packages/stellar-wallet-snap/src/handlers/cronjob/refreshConfirmationContext/api.ts @@ -25,7 +25,13 @@ export type ConfirmationDataContext = Record & ContextWithPrices; /** Outcome of one refresher cycle. `null` means no work was needed. */ export type ConfirmationContextRefreshResult = { result: Record; + /** Vote to schedule another refresh cycle. */ reschedule: boolean; + /** + * When true, the handler does not reschedule after this cycle. + * But other refreshers may still run (e.g. prices). + */ + halt?: boolean; } | null; /** diff --git a/packages/stellar-wallet-snap/src/handlers/cronjob/refreshConfirmationContext/handler.test.ts b/packages/stellar-wallet-snap/src/handlers/cronjob/refreshConfirmationContext/handler.test.ts index 2e36b4493..2661512d6 100644 --- a/packages/stellar-wallet-snap/src/handlers/cronjob/refreshConfirmationContext/handler.test.ts +++ b/packages/stellar-wallet-snap/src/handlers/cronjob/refreshConfirmationContext/handler.test.ts @@ -332,7 +332,7 @@ describe('RefreshConfirmationContextHandler', () => { { refresh: jest.fn().mockResolvedValue({ result: { securityScanRequest: { transaction: 'FRESH_XDR' } }, - reschedule: false, + reschedule: true, }), }, ); @@ -366,8 +366,10 @@ describe('RefreshConfirmationContextHandler', () => { }); // The transaction refresher sees the original context... + expect(transactionRefresher.refresh).toHaveBeenCalledTimes(1); expect(transactionRefresher.refresh).toHaveBeenCalledWith(baseContext); // ...and the scan refresher sees it already patched with the rebuilt envelope. + expect(scanRefresher.refresh).toHaveBeenCalledTimes(1); expect(scanRefresher.refresh).toHaveBeenCalledWith( expect.objectContaining({ securityScanRequest: { transaction: 'FRESH_XDR' }, @@ -426,4 +428,78 @@ describe('RefreshConfirmationContextHandler', () => { expect(scanRefresher.isValidContext).not.toHaveBeenCalled(); expect(updateConfirmation).toHaveBeenCalled(); }); + + it('skips the security scan and does not reschedule when a refresher halts', async () => { + jest + .mocked(getInterfaceContextIfExists) + .mockResolvedValueOnce(baseContext) + .mockResolvedValueOnce(baseContext); + + const transactionRefresher = createMockRefresher( + ConfirmationContextRefresherKey.Transaction, + { + refresh: jest.fn().mockResolvedValue({ + result: { + transactionsFetchStatus: FetchStatus.Error, + scanFetchStatus: FetchStatus.Error, + }, + reschedule: false, + halt: true, + }), + }, + ); + const pricesRefresher = createMockRefresher( + ConfirmationContextRefresherKey.Prices, + { + refresh: jest.fn().mockResolvedValue({ + result: { tokenPricesFetchStatus: FetchStatus.Fetched }, + reschedule: true, + }), + }, + ); + const scanRefresher = createMockRefresher( + ConfirmationContextRefresherKey.Scan, + { + refresh: jest.fn().mockResolvedValue({ + result: { scanFetchStatus: FetchStatus.Fetched }, + reschedule: true, + }), + }, + ); + + const { handler, updateConfirmation } = setup([ + transactionRefresher, + pricesRefresher, + scanRefresher, + ]); + + await handler.handle({ + jsonrpc: '2.0', + id: '1', + method: BackgroundEventMethod.RefreshConfirmationContext, + params: { + ...confirmationContextRequestParams, + refresherKeys: [ + ConfirmationContextRefresherKey.Transaction, + ConfirmationContextRefresherKey.Prices, + ConfirmationContextRefresherKey.Scan, + ], + }, + }); + + expect(transactionRefresher.refresh).toHaveBeenCalledTimes(1); + expect(pricesRefresher.refresh).toHaveBeenCalledTimes(1); + expect(scanRefresher.refresh).not.toHaveBeenCalled(); + expect(scanRefresher.isValidContext).toHaveBeenCalled(); + expect(updateConfirmation).toHaveBeenCalledWith( + expect.objectContaining({ + updatedContext: expect.objectContaining({ + transactionsFetchStatus: FetchStatus.Error, + scanFetchStatus: FetchStatus.Error, + tokenPricesFetchStatus: FetchStatus.Fetched, + }), + }), + ); + expect(scheduleBackgroundEvent).not.toHaveBeenCalled(); + }); }); diff --git a/packages/stellar-wallet-snap/src/handlers/cronjob/refreshConfirmationContext/handler.ts b/packages/stellar-wallet-snap/src/handlers/cronjob/refreshConfirmationContext/handler.ts index ac2bd6ffb..de88ce9c7 100644 --- a/packages/stellar-wallet-snap/src/handlers/cronjob/refreshConfirmationContext/handler.ts +++ b/packages/stellar-wallet-snap/src/handlers/cronjob/refreshConfirmationContext/handler.ts @@ -140,6 +140,13 @@ export class RefreshConfirmationContextHandler extends CronjobBaseHandler result?.halt)) { + this.logger.info( + 'Confirmation refresh halted; cron will not be rescheduled', + ); + return; + } + if (results.some((result) => result?.reschedule)) { await RefreshConfirmationContextHandler.scheduleBackgroundEvent({ scope, @@ -177,6 +184,8 @@ export class RefreshConfirmationContextHandler extends CronjobBaseHandler { - const transactionRefresher = activeRefreshers.find( - (refresher) => - refresher.key === ConfirmationContextRefresherKey.Transaction, - ); - const remainingRefreshers = activeRefreshers.filter( - (refresher) => - refresher.key !== ConfirmationContextRefresherKey.Transaction, + const remainingRefreshers = new Map( + activeRefreshers.map((refresher) => [refresher.key, refresher]), ); const results: ConfirmationContextRefreshResult[] = []; let workingContext = ctx; + const transactionRefresher = remainingRefreshers.get( + ConfirmationContextRefresherKey.Transaction, + ); if (transactionRefresher) { + remainingRefreshers.delete(ConfirmationContextRefresherKey.Transaction); + const transactionResult = await this.#settleRefresher( transactionRefresher, workingContext, @@ -212,10 +221,15 @@ export class RefreshConfirmationContextHandler extends CronjobBaseHandler + [...remainingRefreshers.values()].map(async (refresher) => this.#settleRefresher(refresher, workingContext), ), ); diff --git a/packages/stellar-wallet-snap/src/handlers/cronjob/refreshConfirmationContext/transactionRefresher.test.ts b/packages/stellar-wallet-snap/src/handlers/cronjob/refreshConfirmationContext/transactionRefresher.test.ts index 403458be1..831dec05a 100644 --- a/packages/stellar-wallet-snap/src/handlers/cronjob/refreshConfirmationContext/transactionRefresher.test.ts +++ b/packages/stellar-wallet-snap/src/handlers/cronjob/refreshConfirmationContext/transactionRefresher.test.ts @@ -3,6 +3,21 @@ import { Networks } from '@stellar/stellar-sdk'; import { KnownCaip2ChainId } from '../../../api'; import type { AssetMetadataService } from '../../../services/asset-metadata'; import type { TransactionService } from '../../../services/transaction'; +import { + InsufficientBalanceException, + InsufficientBalanceToCoverBaseReserveException, + InsufficientBalanceToCoverFeeException, + InvalidAmountForCreateAccountException, + InvalidAssetForCreateAccountException, + RemoveTrustlineWithNonZeroBalanceException, + RequiresMemoException, + TransactionExpireException, + TransactionValidationException, + TrustlineExceedLimitException, + TrustlineNotAuthorizedException, + TrustlineNotFoundException, + UpdateTrustlineException, +} from '../../../services/transaction'; import type { MockClassicOperation } from '../../../services/transaction/__mocks__/transaction.fixtures'; import { buildMockClassicTransaction } from '../../../services/transaction/__mocks__/transaction.fixtures'; import { FetchStatus } from '../../../ui/confirmation/api'; @@ -160,23 +175,93 @@ describe('ConfirmationTransactionRefresher', () => { transaction: transactionXdr, }, }, - reschedule: false, + reschedule: true, }); }); - it('marks the transaction invalid when re-validation throws', async () => { - const { refresher, transactionService } = setup(); - transactionService.createValidatedSendTransaction.mockRejectedValueOnce( - new Error('insufficient balance'), - ); + it.each([ + { + error: new InsufficientBalanceException('1', '2'), + errorMessage: 'confirmation.txnError.insufficientBalance', + }, + { + error: new InsufficientBalanceToCoverFeeException('1', '2'), + errorMessage: 'confirmation.txnError.insufficientBalanceToCoverFee', + }, + { + error: new InsufficientBalanceToCoverBaseReserveException('1', '2'), + errorMessage: + 'confirmation.txnError.insufficientBalanceToCoverBaseReserve', + }, + { + error: new RequiresMemoException(toAddress), + errorMessage: 'confirmation.txnError.requiresMemo', + }, + { + error: new InvalidAmountForCreateAccountException('0.5'), + errorMessage: 'confirmation.txnError.invalidCreateAccountAmount', + }, + { + error: new InvalidAssetForCreateAccountException(classicAssetId), + errorMessage: 'confirmation.txnError.invalidCreateAccountAsset', + }, + { + error: new TrustlineNotAuthorizedException(classicAssetId, accountId), + errorMessage: 'confirmation.txnError.trustlineNotAuthorized', + }, + { + error: new TrustlineNotFoundException(classicAssetId, accountId), + errorMessage: 'confirmation.txnError.trustlineNotFoundOnAccount', + }, + { + error: new TrustlineNotFoundException(classicAssetId, toAddress), + errorMessage: 'confirmation.txnError.trustlineNotFound', + }, + { + error: new TrustlineExceedLimitException(classicAssetId), + errorMessage: 'confirmation.txnError.trustlineExceedLimit', + }, + { + error: new RemoveTrustlineWithNonZeroBalanceException('nonzero'), + errorMessage: 'confirmation.txnError.trustlineNonZeroBalance', + }, + { + error: new UpdateTrustlineException('limit'), + errorMessage: 'confirmation.txnError.updateTrustlineLimit', + }, + { + error: new TransactionExpireException(1), + errorMessage: 'confirmation.txnError.expired', + }, + { + error: new TransactionValidationException('unknown'), + errorMessage: 'confirmation.txnError.generic', + }, + { + error: new Error('unknown'), + errorMessage: 'confirmation.txnError.generic', + }, + ])( + 'marks the transaction invalid when re-validation throws ($errorMessage)', + async ({ error, errorMessage }) => { + const { refresher, transactionService } = setup(); + transactionService.createValidatedSendTransaction.mockRejectedValueOnce( + error, + ); - const result = await refresher.refresh(createTransactionContext()); + const result = await refresher.refresh(createTransactionContext()); - expect(result).toStrictEqual({ - result: { transactionsFetchStatus: FetchStatus.Error }, - reschedule: false, - }); - }); + expect(result).toStrictEqual({ + result: { + transactionsFetchStatus: FetchStatus.Error, + errorMessage, + scanFetchStatus: FetchStatus.Error, + }, + reschedule: false, + halt: true, + }); + }, + ); it('re-validates a change-trust opt-in transaction', async () => { const { refresher, transactionService } = setup(); @@ -205,7 +290,7 @@ describe('ConfirmationTransactionRefresher', () => { transaction: transactionXdr, }, }, - reschedule: false, + reschedule: true, }); }); @@ -246,7 +331,7 @@ describe('ConfirmationTransactionRefresher', () => { transaction: transactionXdr, }, }, - reschedule: false, + reschedule: true, }); }); @@ -273,7 +358,7 @@ describe('ConfirmationTransactionRefresher', () => { transaction: transactionXdr, }, }, - reschedule: false, + reschedule: true, }); }); @@ -313,7 +398,7 @@ describe('ConfirmationTransactionRefresher', () => { transaction: transactionXdr, }, }, - reschedule: false, + reschedule: true, }); } finally { jest.useRealTimers(); diff --git a/packages/stellar-wallet-snap/src/handlers/cronjob/refreshConfirmationContext/transactionRefresher.ts b/packages/stellar-wallet-snap/src/handlers/cronjob/refreshConfirmationContext/transactionRefresher.ts index f13a158b2..dc6013726 100644 --- a/packages/stellar-wallet-snap/src/handlers/cronjob/refreshConfirmationContext/transactionRefresher.ts +++ b/packages/stellar-wallet-snap/src/handlers/cronjob/refreshConfirmationContext/transactionRefresher.ts @@ -22,6 +22,7 @@ import { ChangeTrustOptAction, ClientRequestMethod, } from '../../clientRequest/api'; +import { getTxnErrorMessageKey } from '../../clientRequest/utils'; import { ConfirmationContextRefresherKey } from './api'; import type { ConfirmationContextRefreshResult, @@ -98,9 +99,12 @@ export class ConfirmationTransactionRefresher implements IConfirmationContextRef ctx: ConfirmationDataContext, ): Promise { const validationCtx = ctx as TransactionValidationContext; - try { - const { request, accountId, scope } = validationCtx; + const { request, accountId, scope, securityScanRequest, origin } = + validationCtx; + // Use the scan request address as Default if it is present. + let accountAddress = securityScanRequest?.accountAddress ?? ''; + try { // Load the sender from the network so validation uses current sequence and balances. const { onChainAccount } = await this.#accountResolver.resolveAccount({ accountId, @@ -113,11 +117,9 @@ export class ConfirmationTransactionRefresher implements IConfirmationContextRef wallet: false, }, }); + accountAddress = onChainAccount.accountId; - // TODO(follow-up): this validates a rebuilt draft as a proxy for the stored - // envelope. It can miss divergence (payment vs createAccount on a deactivated - // destination, stale Soroban footprint). Seq drift is covered by the submit-time - // txBadSeq retry. For full fidelity, validate the stored envelope itself. + // Always rebuild the transaction, to make sure the transaction is up to date with the latest account state. let rebuiltTransaction: Transaction; switch (request.method) { case ClientRequestMethod.ConfirmSend: { @@ -159,32 +161,34 @@ export class ConfirmationTransactionRefresher implements IConfirmationContextRef throw new Error('Unsupported request method for transaction refresh'); } - const { securityScanRequest, origin } = validationCtx; const rebuiltTransactionXdr = rebuiltTransaction.getRaw().toXDR(); - // Always feed the rebuilt envelope to the scan refresher. The user-facing - // `transaction` field is intentionally left untouched; the signable envelope - // is rebuilt again at confirm time. return { result: { securityScanRequest: { accountAddress: - securityScanRequest?.accountAddress ?? onChainAccount.accountId, + securityScanRequest?.accountAddress ?? accountAddress, origin: securityScanRequest?.origin ?? origin ?? '', scope, transaction: rebuiltTransactionXdr, }, }, - reschedule: false, + reschedule: true, }; - } catch (error) { + } catch (error: unknown) { this.#logger.error( 'Error re-validating confirmation transaction:', error, ); return { - result: { transactionsFetchStatus: FetchStatus.Error }, + result: { + transactionsFetchStatus: FetchStatus.Error, + errorMessage: getTxnErrorMessageKey(error, accountAddress), + // Clear the scan loading state in the confirmation UI + skip the security scan request. + scanFetchStatus: FetchStatus.Error, + }, reschedule: false, + halt: true, }; } }