diff --git a/packages/snap/.env.example b/packages/snap/.env.example index e4dd785..99862ed 100644 --- a/packages/snap/.env.example +++ b/packages/snap/.env.example @@ -74,4 +74,10 @@ SECURITY_ALERTS_API_BASE_URL=https://security-alerts.api.cx.metamask.io #MAX_FEE_THRESHOLD_IN_XLM= # Maximum background reschedules for the track-transaction cron job while Horizon has not -#TRACK_TRANSACTION_MAX_RESCHEDULES=10 \ No newline at end of file +#TRACK_TRANSACTION_MAX_RESCHEDULES=10 + +# Maximum number of reconcile attempts for a pending transaction +#MAX_RECONCILE_ATTEMPTS=5 + +# Maximum age of a pending transaction in milliseconds +#MAX_PENDING_TRANSACTION_AGE=30000 \ No newline at end of file diff --git a/packages/snap/snap.config.ts b/packages/snap/snap.config.ts index 65816dd..8d1fc1e 100644 --- a/packages/snap/snap.config.ts +++ b/packages/snap/snap.config.ts @@ -44,6 +44,8 @@ const config: SnapConfig = { BASE_FEE_MULTIPLIER: process.env.BASE_FEE_MULTIPLIER ?? '', SIMULATION_FEE_MULTIPLIER: process.env.SIMULATION_FEE_MULTIPLIER ?? '', MAX_FEE_THRESHOLD_IN_XLM: process.env.MAX_FEE_THRESHOLD_IN_XLM ?? '', + MAX_RECONCILE_ATTEMPTS: process.env.MAX_RECONCILE_ATTEMPTS ?? '', + MAX_PENDING_TRANSACTION_AGE: process.env.MAX_PENDING_TRANSACTION_AGE ?? '', }, polyfills: true, }; diff --git a/packages/snap/snap.manifest.json b/packages/snap/snap.manifest.json index 9f57f31..6500e89 100644 --- a/packages/snap/snap.manifest.json +++ b/packages/snap/snap.manifest.json @@ -7,7 +7,7 @@ "url": "https://github.com/MetaMask/snap-stellar-wallet.git" }, "source": { - "shasum": "fcXiAJRViO4Lb+5+PZ8Nhi9yA6B3/kafroMv9FYTe3A=", + "shasum": "9+dwiv652krRNthWB+CWrTYrwYJP3ngwel8BTZucQeg=", "location": { "npm": { "filePath": "dist/bundle.js", diff --git a/packages/snap/src/config.ts b/packages/snap/src/config.ts index 1b281b3..312ec05 100644 --- a/packages/snap/src/config.ts +++ b/packages/snap/src/config.ts @@ -110,6 +110,20 @@ const ConfigStruct = object({ * The maximum fee threshold in XLM for the Stellar network. */ maxFeeThresholdInXLM: parseFloatStruct(1, 1), + /** + * The maximum number of Horizon not-found reconcile attempts for a pending transaction. + * Used with `maxPendingTransactionAge` to evict stale pending txs from snap state; + * both limits must be exceeded before a pending tx is dropped. + * Minimum value is 2 to avoid dropping the pending transaction too early. + */ + maxReconcileAttempts: parseIntegerStruct(2, 5), + /** + * The maximum age of a pending transaction in milliseconds. + * Used with `maxReconcileAttempts` to evict stale pending txs from snap state; + * both limits must be exceeded before a pending tx is dropped. + * Minimum value is 15000 to avoid dropping the pending transaction too early. + */ + maxPendingTransactionAge: parseIntegerStruct(15000, 30000), }), api: object({ tokenApi: object({ @@ -185,6 +199,8 @@ export const AppConfig = create( maxFeeThresholdInXLM: process.env.MAX_FEE_THRESHOLD_IN_XLM, trackTransactionMaxReschedules: process.env.TRACK_TRANSACTION_MAX_RESCHEDULES, + maxReconcileAttempts: process.env.MAX_RECONCILE_ATTEMPTS, + maxPendingTransactionAge: process.env.MAX_PENDING_TRANSACTION_AGE, }, api: { tokenApi: { diff --git a/packages/snap/src/handlers/cronjob/refreshConfirmationContext/handler.test.ts b/packages/snap/src/handlers/cronjob/refreshConfirmationContext/handler.test.ts index 9bcc323..22fdaae 100644 --- a/packages/snap/src/handlers/cronjob/refreshConfirmationContext/handler.test.ts +++ b/packages/snap/src/handlers/cronjob/refreshConfirmationContext/handler.test.ts @@ -321,68 +321,6 @@ describe('RefreshConfirmationContextHandler', () => { ); }); - it('runs the transaction refresher first and feeds its patch to the remaining refreshers', async () => { - jest - .mocked(getInterfaceContextIfExists) - .mockResolvedValueOnce(baseContext) - .mockResolvedValueOnce(baseContext); - - const transactionRefresher = createMockRefresher( - ConfirmationContextRefresherKey.Transaction, - { - refresh: jest.fn().mockResolvedValue({ - result: { securityScanRequest: { transaction: 'FRESH_XDR' } }, - reschedule: false, - }), - }, - ); - const scanRefresher = createMockRefresher( - ConfirmationContextRefresherKey.Scan, - { - refresh: jest.fn().mockResolvedValue({ - result: { scanFetchStatus: FetchStatus.Fetched }, - reschedule: true, - }), - }, - ); - - // Register scan first to prove ordering is driven by key, not array order. - const { handler, updateConfirmation } = setup([ - scanRefresher, - transactionRefresher, - ]); - - await handler.handle({ - jsonrpc: '2.0', - id: '1', - method: BackgroundEventMethod.RefreshConfirmationContext, - params: { - ...confirmationContextRequestParams, - refresherKeys: [ - ConfirmationContextRefresherKey.Scan, - ConfirmationContextRefresherKey.Transaction, - ], - }, - }); - - // The transaction refresher sees the original context... - expect(transactionRefresher.refresh).toHaveBeenCalledWith(baseContext); - // ...and the scan refresher sees it already patched with the rebuilt envelope. - expect(scanRefresher.refresh).toHaveBeenCalledWith( - expect.objectContaining({ - securityScanRequest: { transaction: 'FRESH_XDR' }, - }), - ); - expect(updateConfirmation).toHaveBeenCalledWith( - expect.objectContaining({ - updatedContext: expect.objectContaining({ - securityScanRequest: { transaction: 'FRESH_XDR' }, - scanFetchStatus: FetchStatus.Fetched, - }), - }), - ); - }); - it('does not run a refresher when its key is omitted from refresherKeys', async () => { jest.mocked(getInterfaceContextIfExists).mockResolvedValue(baseContext); diff --git a/packages/snap/src/handlers/cronjob/refreshConfirmationContext/handler.ts b/packages/snap/src/handlers/cronjob/refreshConfirmationContext/handler.ts index f1cd9ff..6be1ebd 100644 --- a/packages/snap/src/handlers/cronjob/refreshConfirmationContext/handler.ts +++ b/packages/snap/src/handlers/cronjob/refreshConfirmationContext/handler.ts @@ -1,9 +1,9 @@ import type { Json } from '@metamask/utils'; -import { ConfirmationContextRefresherKey } from './api'; import type { ConfirmationContextRefreshResult, ConfirmationContextRefreshers, + ConfirmationContextRefresherKey, ConfirmationDataContext, IConfirmationContextRefresher, } from './api'; @@ -170,17 +170,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 results: ConfirmationContextRefreshResult[] = []; - let workingContext = ctx; - - if (transactionRefresher) { - const transactionResult = await this.#settleRefresher( - transactionRefresher, - workingContext, - ); - results.push(transactionResult); - // Merge the transaction refresher's patch into the context the remaining - // refreshers see, so they work off the renewed envelope (latest fee, - // account sequence, extended expiry) rather than the original snapshot. - if (transactionResult?.result) { - workingContext = { ...workingContext, ...transactionResult.result }; - } - } - - const remainingResults = await Promise.all( - remainingRefreshers.map(async (refresher) => - this.#settleRefresher(refresher, workingContext), + const settled = await Promise.allSettled( + activeRefreshers.map(async (refresher) => + this.#runRefresher(refresher, ctx), ), ); - return [...results, ...remainingResults]; - } - - /** - * Runs a single refresher and converts an unexpected rejection into `null`, - * so a failing refresher never blocks the others. - * - * @param refresher - The refresher to run. - * @param ctx - The context passed to the refresher. - * @returns The refresher result, or `null` when it rejected. - */ - async #settleRefresher( - refresher: IConfirmationContextRefresher, - ctx: ConfirmationDataContext, - ): Promise { - try { - return await this.#runRefresher(refresher, ctx); - } catch (error) { + return settled.map((outcome, index) => { + if (outcome.status === 'fulfilled') { + return outcome.value; + } this.logger.error( - `Refresher "${refresher.key}" rejected unexpectedly`, - error, + `Refresher "${activeRefreshers[index]?.key}" rejected unexpectedly`, + outcome.reason, ); return null; - } + }); } async #runRefresher( diff --git a/packages/snap/src/handlers/cronjob/refreshConfirmationContext/transactionRefresher.test.ts b/packages/snap/src/handlers/cronjob/refreshConfirmationContext/transactionRefresher.test.ts index f00d841..6b6961a 100644 --- a/packages/snap/src/handlers/cronjob/refreshConfirmationContext/transactionRefresher.test.ts +++ b/packages/snap/src/handlers/cronjob/refreshConfirmationContext/transactionRefresher.test.ts @@ -6,7 +6,6 @@ import { ConfirmationTransactionRefresher } from './transactionRefresher'; import { KnownCaip2ChainId } from '../../../api'; import type { AssetMetadataService } from '../../../services/asset-metadata'; import type { TransactionService } 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'; import { getSlip44AssetId } from '../../../utils'; @@ -22,27 +21,17 @@ describe('ConfirmationTransactionRefresher', () => { const accountId = '11111111-1111-4111-8111-111111111111'; const toAddress = 'GDPMFLKUGASUTWBN2XGYYKD27QGHCYH4BUFUTER4L23INYQ4JHDWFOIE'; - const paymentOperations: MockClassicOperation[] = [ - { - type: 'payment', - params: { destination: toAddress, asset: 'native', amount: '1' }, - }, - ]; - - const transaction = buildMockClassicTransaction(paymentOperations, { - networkPassphrase: Networks.TESTNET, - }); + const transaction = buildMockClassicTransaction( + [ + { + type: 'payment', + params: { destination: toAddress, asset: 'native', amount: '1' }, + }, + ], + { networkPassphrase: Networks.TESTNET }, + ); const transactionXdr = transaction.getRaw().toXDR(); - // The envelope previously held in the security-scan request. Each refresh - // cycle rebuilds the transaction and swaps this for the freshly rebuilt one. - const scanTransactionXdr = buildMockClassicTransaction(paymentOperations, { - networkPassphrase: Networks.TESTNET, - timeout: 600, - }) - .getRaw() - .toXDR(); - const sendRequest = { jsonrpc: '2.0' as const, id: 1, @@ -124,7 +113,6 @@ describe('ConfirmationTransactionRefresher', () => { transactionsFetchStatus: FetchStatus.Fetched, accountId, scope, - origin: 'https://metamask.io', request: sendRequest, ...overrides, }); @@ -135,7 +123,7 @@ describe('ConfirmationTransactionRefresher', () => { expect(refresher.key).toBe(ConfirmationContextRefresherKey.Transaction); }); - it('re-validates the send transaction and propagates the rebuilt envelope to security scan', async () => { + it('re-validates the send transaction and returns no patch on success', async () => { const { refresher, transactionService } = setup(); const result = await refresher.refresh(createTransactionContext()); @@ -149,17 +137,7 @@ describe('ConfirmationTransactionRefresher', () => { destination: toAddress, amount: expect.anything(), }); - expect(result).toStrictEqual({ - result: { - securityScanRequest: { - accountAddress: accountId, - origin: 'https://metamask.io', - scope, - transaction: transactionXdr, - }, - }, - reschedule: false, - }); + expect(result).toBeNull(); }); it('marks the transaction invalid when re-validation throws', async () => { @@ -194,17 +172,7 @@ describe('ConfirmationTransactionRefresher', () => { expect( transactionService.createValidatedSendTransaction, ).not.toHaveBeenCalled(); - expect(result).toStrictEqual({ - result: { - securityScanRequest: { - accountAddress: accountId, - origin: 'https://metamask.io', - scope, - transaction: transactionXdr, - }, - }, - reschedule: false, - }); + expect(result).toBeNull(); }); it('re-validates a change-trust opt-out transaction with a zero limit', async () => { @@ -224,59 +192,9 @@ describe('ConfirmationTransactionRefresher', () => { }); }); - it('renews the security-scan transaction with the rebuilt envelope', async () => { - const { refresher } = setup(); - const securityScanRequest = { - accountAddress: toAddress, - origin: 'https://dapp.example', - scope, - transaction: scanTransactionXdr, - }; - - const result = await refresher.refresh( - createTransactionContext({ securityScanRequest }), - ); - - expect(result).toStrictEqual({ - result: { - securityScanRequest: { - ...securityScanRequest, - transaction: transactionXdr, - }, - }, - reschedule: false, - }); - }); - - it('renews the security-scan transaction for change-trust flows', async () => { - const { refresher } = setup(); - const securityScanRequest = { - accountAddress: toAddress, - origin: 'https://dapp.example', - scope, - transaction: scanTransactionXdr, - }; - - const result = await refresher.refresh( - createTransactionContext({ - request: changeTrustAddRequest, - securityScanRequest, - }), - ); - - expect(result).toStrictEqual({ - result: { - securityScanRequest: { - ...securityScanRequest, - transaction: transactionXdr, - }, - }, - reschedule: false, - }); - }); - - it('rebuilds when the stored envelope has expired', async () => { + it('marks the transaction invalid when the original envelope has expired', async () => { const { refresher, transactionService } = setup(); + // The stored XDR being signed is expired, even though the rebuilt draft would be valid. const mockNow = 1_700_000_000_000; jest.useFakeTimers(); jest.setSystemTime(mockNow); @@ -301,16 +219,9 @@ describe('ConfirmationTransactionRefresher', () => { expect( transactionService.createValidatedSendTransaction, - ).toHaveBeenCalled(); + ).not.toHaveBeenCalled(); expect(result).toStrictEqual({ - result: { - securityScanRequest: { - accountAddress: accountId, - origin: 'https://metamask.io', - scope, - transaction: transactionXdr, - }, - }, + result: { transactionsFetchStatus: FetchStatus.Error }, reschedule: false, }); } finally { diff --git a/packages/snap/src/handlers/cronjob/refreshConfirmationContext/transactionRefresher.ts b/packages/snap/src/handlers/cronjob/refreshConfirmationContext/transactionRefresher.ts index cd6785e..e54b137 100644 --- a/packages/snap/src/handlers/cronjob/refreshConfirmationContext/transactionRefresher.ts +++ b/packages/snap/src/handlers/cronjob/refreshConfirmationContext/transactionRefresher.ts @@ -8,14 +8,12 @@ import { type IConfirmationContextRefresher, } from './api'; import type { AssetMetadataService } from '../../../services/asset-metadata'; -import type { +import { Transaction, - TransactionService, + type TransactionService, } from '../../../services/transaction'; -import type { - ContextWithSecurityScan, - ContextWithTransactionValidation, -} from '../../../ui/confirmation/api'; +import { assertTransactionTimeBound } from '../../../services/transaction/utils'; +import type { ContextWithTransactionValidation } from '../../../ui/confirmation/api'; import { ContextWithTransactionValidationStruct, FetchStatus, @@ -31,18 +29,12 @@ import { } from '../../clientRequest/api'; type TransactionValidationContext = ConfirmationDataContext & - ContextWithTransactionValidation & - Partial & { - // origin is always present on the rendered confirmation context - // (ConfirmationBaseProps.origin), but isn't part of the validation struct. - origin?: string; - }; + ContextWithTransactionValidation; /** * Re-validates the pending transaction while the sign confirmation dialog is open. - * Transaction slice of the confirmation context refresh pipeline; on every cycle - * it rebuilds against the latest on-chain account state and propagates the fresh - * envelope to the security-scan request for the scan refresher. + * Transaction slice of the confirmation context refresh pipeline; periodically + * checks time bounds, fees, and balance against the latest on-chain account state. */ export class ConfirmationTransactionRefresher implements IConfirmationContextRefresher { readonly key = ConfirmationContextRefresherKey.Transaction; @@ -103,7 +95,12 @@ export class ConfirmationTransactionRefresher implements IConfirmationContextRef ): Promise { const validationCtx = ctx as TransactionValidationContext; try { - const { request, accountId, scope } = validationCtx; + const { + request, + accountId, + scope, + transaction: transactionXdr, + } = validationCtx; // Load the sender from the network so validation uses current sequence and balances. const { onChainAccount } = await this.#accountResolver.resolveAccount({ @@ -118,11 +115,19 @@ export class ConfirmationTransactionRefresher implements IConfirmationContextRef }, }); + // Deserialize the envelope awaiting signature and assert its own time bound. + // The draft rebuilt below gets a fresh timeout, so validating that draft would + // miss expiry of the transaction the user is actually looking at. + const transaction = Transaction.fromXdr({ + xdr: transactionXdr, + scope, + }); + assertTransactionTimeBound(transaction); + // 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. - let rebuiltTransaction: Transaction; switch (request.method) { case ClientRequestMethod.ConfirmSend: { const assetMetadata = await this.#assetMetadataService.resolve( @@ -134,53 +139,34 @@ export class ConfirmationTransactionRefresher implements IConfirmationContextRef decimals, ); // Throws on insufficient balance, inactive destination, or fee estimate failure. - rebuiltTransaction = - await this.#transactionService.createValidatedSendTransaction({ - onChainAccount, - scope, - assetId: request.params.assetId, - destination: request.params.toAddress, - amount, - }); + await this.#transactionService.createValidatedSendTransaction({ + onChainAccount, + scope, + assetId: request.params.assetId, + destination: request.params.toAddress, + amount, + }); break; } case ClientRequestMethod.ChangeTrustOpt: // Throws when change-trust limits or account state are no longer valid. - rebuiltTransaction = - await this.#transactionService.createValidatedChangeTrustTransaction( - { - onChainAccount, - scope, - assetId: request.params.assetId, - limit: - request.params.action === ChangeTrustOptAction.Delete - ? '0' - : request.params.limit, - }, - ); + await this.#transactionService.createValidatedChangeTrustTransaction({ + onChainAccount, + scope, + assetId: request.params.assetId, + limit: + request.params.action === ChangeTrustOptAction.Delete + ? '0' + : request.params.limit, + }); break; default: 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, - origin: securityScanRequest?.origin ?? origin ?? '', - scope, - transaction: rebuiltTransactionXdr, - }, - }, - reschedule: false, - }; + // Still valid: nothing to write. The status stays Fetched and we don't drive a + // reschedule ourselves (other refreshers keep the cron alive while the dialog is open). + return null; } catch (error) { this.#logger.error( 'Error re-validating confirmation transaction:', diff --git a/packages/snap/src/services/network/exceptions.ts b/packages/snap/src/services/network/exceptions.ts index 8b62261..c17b4c6 100644 --- a/packages/snap/src/services/network/exceptions.ts +++ b/packages/snap/src/services/network/exceptions.ts @@ -73,8 +73,11 @@ export class TransactionRetryableException extends TransactionSendException {} /** Thrown when a transaction is not found. */ export class TransactionNotFoundException extends NetworkServiceException { + readonly transactionHash: string; + constructor(transactionHash: string, options?: StellarSnapExceptionOptions) { super(`Transaction ${transactionHash} not found`, options); + this.transactionHash = transactionHash; } } diff --git a/packages/snap/src/services/transaction/TransactionRepository.test.ts b/packages/snap/src/services/transaction/TransactionRepository.test.ts new file mode 100644 index 0000000..89dd7d8 --- /dev/null +++ b/packages/snap/src/services/transaction/TransactionRepository.test.ts @@ -0,0 +1,203 @@ +import { TransactionStatus } from '@metamask/keyring-api'; + +import { KnownCaip2ChainId } from '../../api'; +import { AppConfig } from '../../config'; +import { generateMockTransactions } from './__mocks__/transaction.fixtures'; +import type { StellarKeyringTransaction } from './api'; +import type { TransactionStateValue } from './TransactionRepository'; +import { TransactionRepository } from './TransactionRepository'; +import { getSnapProvider } from '../../utils/snap'; +import { State } from '../state/State'; + +jest.mock('../../utils/snap'); + +describe('TransactionRepository', () => { + const scope = KnownCaip2ChainId.Mainnet; + const accountId = 'account-1'; + + const defaultState: TransactionStateValue = { + transactions: {}, + lastScanTokens: {}, + }; + + const recentTimestampSeconds = () => Math.floor(Date.now() / 1000); + + const expiredTimestampSeconds = () => + Math.floor( + (Date.now() - AppConfig.transaction.maxPendingTransactionAge - 1000) / + 1000, + ); + + let mockState: TransactionStateValue; + + const createRepository = () => + new TransactionRepository( + new State({ + encrypted: false, + defaultState, + }), + ); + + beforeEach(() => { + mockState = structuredClone(defaultState); + const snapProvider = getSnapProvider() as { request: jest.Mock }; + snapProvider.request.mockImplementation(async ({ method, params }) => { + if (method === 'snap_getState') { + if (params.key) { + return mockState[params.key as keyof TransactionStateValue]; + } + return mockState; + } + + if (method === 'snap_manageState' && params.operation === 'update') { + mockState = params.newState as TransactionStateValue; + } + + return null; + }); + }); + + afterEach(() => { + (getSnapProvider() as { request: jest.Mock }).request.mockReset(); + }); + + it('removes confirmed incoming transactions from snap state', async () => { + const repository = createRepository(); + const pendingTransaction = generateMockTransactions(1, { + id: 'tx-hash-1', + account: accountId, + scope, + status: TransactionStatus.Unconfirmed, + })[0] as StellarKeyringTransaction; + + await repository.saveMany([pendingTransaction]); + + const confirmedTransaction = generateMockTransactions(1, { + id: 'tx-hash-1', + account: accountId, + scope, + status: TransactionStatus.Confirmed, + })[0] as StellarKeyringTransaction; + + await repository.saveMany([confirmedTransaction]); + + const stored = await repository.findStellarTransactionsByAccountIds([ + accountId, + ]); + expect(stored).toStrictEqual([]); + }); + + it('keeps incoming pending over existing when reconcileAttemptCount is higher', async () => { + const repository = createRepository(); + const pendingTransaction = generateMockTransactions(1, { + id: 'tx-hash-1', + account: accountId, + scope, + status: TransactionStatus.Unconfirmed, + timestamp: 100, + })[0] as StellarKeyringTransaction; + + await repository.saveMany([ + { + ...pendingTransaction, + reconcileAttemptCount: 1, + } as StellarKeyringTransaction, + ]); + + await repository.saveMany([ + { + ...pendingTransaction, + reconcileAttemptCount: 2, + } as StellarKeyringTransaction, + ]); + + const stored = await repository.findStellarTransactionsByAccountIds([ + accountId, + ]); + expect(stored).toStrictEqual([ + expect.objectContaining({ + id: 'tx-hash-1', + reconcileAttemptCount: 2, + }), + ]); + }); + + it('drops pending transactions when reconcile attempts and max age are both exceeded', async () => { + const repository = createRepository(); + const pendingTransaction = generateMockTransactions(1, { + id: 'tx-hash-1', + account: accountId, + scope, + status: TransactionStatus.Unconfirmed, + timestamp: expiredTimestampSeconds(), + })[0] as StellarKeyringTransaction; + + await repository.saveMany([ + { + ...pendingTransaction, + reconcileAttemptCount: 5, + } as StellarKeyringTransaction, + ]); + + const stored = await repository.findStellarTransactionsByAccountIds([ + accountId, + ]); + expect(stored).toStrictEqual([]); + }); + + it('keeps pending transactions when reconcile attempts exceeded but max age is not', async () => { + const repository = createRepository(); + const pendingTransaction = generateMockTransactions(1, { + id: 'tx-hash-1', + account: accountId, + scope, + status: TransactionStatus.Unconfirmed, + timestamp: recentTimestampSeconds(), + })[0] as StellarKeyringTransaction; + + await repository.saveMany([ + { + ...pendingTransaction, + reconcileAttemptCount: 5, + } as StellarKeyringTransaction, + ]); + + const stored = await repository.findStellarTransactionsByAccountIds([ + accountId, + ]); + expect(stored).toStrictEqual([ + expect.objectContaining({ + id: 'tx-hash-1', + reconcileAttemptCount: 5, + }), + ]); + }); + + it('keeps pending transactions when max age exceeded but reconcile attempts are not', async () => { + const repository = createRepository(); + const pendingTransaction = generateMockTransactions(1, { + id: 'tx-hash-1', + account: accountId, + scope, + status: TransactionStatus.Unconfirmed, + timestamp: expiredTimestampSeconds(), + })[0] as StellarKeyringTransaction; + + await repository.saveMany([ + { + ...pendingTransaction, + reconcileAttemptCount: 1, + } as StellarKeyringTransaction, + ]); + + const stored = await repository.findStellarTransactionsByAccountIds([ + accountId, + ]); + expect(stored).toStrictEqual([ + expect.objectContaining({ + id: 'tx-hash-1', + reconcileAttemptCount: 1, + }), + ]); + }); +}); diff --git a/packages/snap/src/services/transaction/TransactionRepository.ts b/packages/snap/src/services/transaction/TransactionRepository.ts index fdb7381..db5d815 100644 --- a/packages/snap/src/services/transaction/TransactionRepository.ts +++ b/packages/snap/src/services/transaction/TransactionRepository.ts @@ -3,12 +3,17 @@ import { groupBy } from 'lodash'; import sortBy from 'lodash/sortBy'; import uniqBy from 'lodash/uniqBy'; -import { isPendingTransactionStatus } from './utils'; +import type { StellarKeyringTransaction } from './api'; +import { + isPendingTransactionStatus, + shouldDropPendingTransaction, + toKeyringTransactions, +} from './utils'; import type { KnownCaip2ChainId } from '../../api'; import type { State } from '../state/State'; export type TransactionStateValue = { - transactions: Record; + transactions: Record; lastScanTokens: Record>; }; @@ -28,18 +33,29 @@ export class TransactionRepository { TransactionStateValue['transactions'] >(this.#stateKey); - return Object.values(transactionsByAccount ?? {}).flat(); + return toKeyringTransactions( + Object.values(transactionsByAccount ?? {}).flat(), + ); } async findByAccountIds( accountIds: string[], scope?: KnownCaip2ChainId, ): Promise { + return toKeyringTransactions( + await this.findStellarTransactionsByAccountIds(accountIds, scope), + ); + } + + async findStellarTransactionsByAccountIds( + accountIds: string[], + scope?: KnownCaip2ChainId, + ): Promise { const transactionsByAccount = await this.#state.getKey< TransactionStateValue['transactions'] >(this.#stateKey); - const transactions: KeyringTransaction[] = []; + const transactions: StellarKeyringTransaction[] = []; for (const accountId of accountIds) { const accountTransactions = transactionsByAccount?.[accountId] ?? []; transactions.push( @@ -58,7 +74,7 @@ export class TransactionRepository { TransactionStateValue['transactions'] >(this.#stateKey); - return transactionsByAccount?.[accountId] ?? []; + return toKeyringTransactions(transactionsByAccount?.[accountId] ?? []); } async findLastScanTokenByAccountIds( @@ -78,7 +94,7 @@ export class TransactionRepository { return lastScanTokenByAccountId; } - async save(transaction: KeyringTransaction): Promise { + async save(transaction: StellarKeyringTransaction): Promise { await this.saveMany([transaction]); } @@ -87,13 +103,14 @@ export class TransactionRepository { * * Submitted transactions are upserted locally. Confirmed and failed transactions * remove any matching id from snap state — durable history lives in the controller - * after AccountTransactionsUpdated is emitted. + * after AccountTransactionsUpdated is emitted. Pending transactions that exceed both + * `maxReconcileAttempts` and `maxPendingTransactionAge` are also evicted. * * @param transactions - Transactions to reconcile into snap state. * @param lastScanTokens - Optional scan cursors to persist per account and scope. */ async saveMany( - transactions: KeyringTransaction[], + transactions: StellarKeyringTransaction[], lastScanTokens?: Record>, ): Promise { if (transactions.length === 0 && !lastScanTokens) { @@ -142,16 +159,31 @@ export class TransactionRepository { } #applyTransactionUpdate( - existingTransactions: KeyringTransaction[], - incomingTransactions: KeyringTransaction[], - ): KeyringTransaction[] { + existingTransactions: StellarKeyringTransaction[], + incomingTransactions: StellarKeyringTransaction[], + ): StellarKeyringTransaction[] { const merged = [...incomingTransactions, ...existingTransactions]; - // Merge the transactions based on transaction id and keep the latest one + // Merge the transactions based on transaction id and choose the incoming one if there are duplicates // Sort it by timestamp in descending order - // Filter out the completed transactions + // Filter out completed transactions and pending txs eligible for eviction return sortBy( uniqBy(merged, 'id'), (item) => -(item.timestamp ?? 0), - ).filter((transaction) => isPendingTransactionStatus(transaction.status)); + ).filter((transaction) => this.#canSave(transaction)); + } + + /** + * Checks if the transaction can be saved to snap state. + * + * Keeps pending transactions unless both reconcile attempts and max age are exceeded. + * + * @param transaction - The transaction to check. + * @returns Whether the transaction can be saved to snap state. + */ + #canSave(transaction: StellarKeyringTransaction): boolean { + return ( + isPendingTransactionStatus(transaction.status) && + !shouldDropPendingTransaction(transaction) + ); } } diff --git a/packages/snap/src/services/transaction/TransactionSynchronizeService.test.ts b/packages/snap/src/services/transaction/TransactionSynchronizeService.test.ts index 3585e5d..2e99da9 100644 --- a/packages/snap/src/services/transaction/TransactionSynchronizeService.test.ts +++ b/packages/snap/src/services/transaction/TransactionSynchronizeService.test.ts @@ -14,6 +14,7 @@ import { buildMockClassicTransaction, generateMockTransactions, } from './__mocks__/transaction.fixtures'; +import type { StellarKeyringTransaction } from './api'; import { KeyringTransactionBuilder } from './KeyringTransactionBuilder'; import { Transaction } from './Transaction'; import { TransactionMapper } from './TransactionMapper'; @@ -27,7 +28,7 @@ import type { AccountService } from '../account/AccountService'; import type { StellarAssetMetadata } from '../asset-metadata/api'; import { toStellarAssetMetadata } from '../asset-metadata/utils'; import { createMemoryCache } from '../cache/__mocks__/cache.fixtures'; -import { NetworkService } from '../network'; +import { NetworkService, TransactionNotFoundException } from '../network'; import { createMockAccountWithBalances, DEFAULT_MOCK_ACCOUNT_WITH_BALANCES, @@ -140,9 +141,9 @@ describe('TransactionSynchronizeService', () => { accountService, getTransactionsSpy: jest.spyOn(networkService, 'getTransactions'), getTransactionSpy: jest.spyOn(networkService, 'getTransaction'), - findByAccountIdsSpy: jest.spyOn( + findStellarTransactionsByAccountIdsSpy: jest.spyOn( transactionRepository, - 'findByAccountIds', + 'findStellarTransactionsByAccountIds', ), findLastScanTokenSpy: jest.spyOn( transactionRepository, @@ -204,7 +205,7 @@ describe('TransactionSynchronizeService', () => { const { service, getTransactionsSpy, - findByAccountIdsSpy, + findStellarTransactionsByAccountIdsSpy, findLastScanTokenSpy, saveManySpy, emitSnapKeyringEventSpy, @@ -219,7 +220,7 @@ describe('TransactionSynchronizeService', () => { amount: '3', }); - findByAccountIdsSpy.mockResolvedValue([]); + findStellarTransactionsByAccountIdsSpy.mockResolvedValue([]); findLastScanTokenSpy.mockResolvedValue({ [keyringAccount.id]: null, }); @@ -270,7 +271,7 @@ describe('TransactionSynchronizeService', () => { service, getTransactionsSpy, getTransactionSpy, - findByAccountIdsSpy, + findStellarTransactionsByAccountIdsSpy, findLastScanTokenSpy, saveManySpy, } = setup(); @@ -290,7 +291,7 @@ describe('TransactionSynchronizeService', () => { status: TransactionStatus.Unconfirmed, }); - findByAccountIdsSpy.mockResolvedValue([ + findStellarTransactionsByAccountIdsSpy.mockResolvedValue([ pendingTransaction as KeyringTransaction, ]); findLastScanTokenSpy.mockResolvedValue({ @@ -343,7 +344,7 @@ describe('TransactionSynchronizeService', () => { const { service, getTransactionsSpy, - findByAccountIdsSpy, + findStellarTransactionsByAccountIdsSpy, findLastScanTokenSpy, saveManySpy, emitSnapKeyringEventSpy, @@ -359,7 +360,7 @@ describe('TransactionSynchronizeService', () => { scope, }); - findByAccountIdsSpy.mockResolvedValue([]); + findStellarTransactionsByAccountIdsSpy.mockResolvedValue([]); findLastScanTokenSpy.mockResolvedValue({ [senderAccount.id]: null, }); @@ -438,7 +439,7 @@ describe('TransactionSynchronizeService', () => { const { service, getTransactionsSpy, - findByAccountIdsSpy, + findStellarTransactionsByAccountIdsSpy, findLastScanTokenSpy, emitSnapKeyringEventSpy, } = setup({ @@ -456,7 +457,7 @@ describe('TransactionSynchronizeService', () => { scope, }); - findByAccountIdsSpy.mockResolvedValue([]); + findStellarTransactionsByAccountIdsSpy.mockResolvedValue([]); findLastScanTokenSpy.mockResolvedValue({ [senderAccount.id]: null, }); @@ -485,4 +486,107 @@ describe('TransactionSynchronizeService', () => { `transactions.${recipientAccount.id}`, ); }); + + it('increments reconcile attempt when pending transaction is not found on Horizon', async () => { + const { + service, + getTransactionsSpy, + getTransactionSpy, + findStellarTransactionsByAccountIdsSpy, + findLastScanTokenSpy, + saveManySpy, + } = setup(); + const { keyringAccount, activatedAccountPair } = buildActivatedPair( + 'account-sync-attempt', + 'GA7UCNSASSOPQYTRGJ2NC7TDBSXHMWK6JHS7AO6X2ZQAIQSTB5ELNFSO', + ); + const [pendingTransaction] = generateMockTransactions(1, { + id: 'missing-tx-hash', + account: keyringAccount.id, + scope, + status: TransactionStatus.Unconfirmed, + }); + + findStellarTransactionsByAccountIdsSpy.mockResolvedValue([ + { + ...pendingTransaction, + reconcileAttemptCount: 1, + } as StellarKeyringTransaction, + ]); + findLastScanTokenSpy.mockResolvedValue({ + [keyringAccount.id]: 'existing-token', + }); + getTransactionsSpy.mockResolvedValue([]); + getTransactionSpy.mockRejectedValue( + new TransactionNotFoundException('missing-tx-hash'), + ); + + await service.synchronize([activatedAccountPair], scope, []); + + expect(getTransactionSpy).toHaveBeenCalledWith('missing-tx-hash', scope); + expect(saveManySpy).toHaveBeenCalledWith( + [ + expect.objectContaining({ + id: 'missing-tx-hash', + reconcileAttemptCount: 2, + }), + ], + { + [keyringAccount.id]: { + [scope]: 'existing-token', + }, + }, + ); + }); + + it('increments reconcile attempt to max when one below limit', async () => { + const { + service, + getTransactionsSpy, + getTransactionSpy, + findStellarTransactionsByAccountIdsSpy, + findLastScanTokenSpy, + saveManySpy, + } = setup(); + const { keyringAccount, activatedAccountPair } = buildActivatedPair( + 'account-sync-drop', + 'GA7UCNSASSOPQYTRGJ2NC7TDBSXHMWK6JHS7AO6X2ZQAIQSTB5ELNFSO', + ); + const [pendingTransaction] = generateMockTransactions(1, { + id: 'missing-tx-hash-drop', + account: keyringAccount.id, + scope, + status: TransactionStatus.Unconfirmed, + }); + + findStellarTransactionsByAccountIdsSpy.mockResolvedValue([ + { + ...pendingTransaction, + reconcileAttemptCount: 4, + } as StellarKeyringTransaction, + ]); + findLastScanTokenSpy.mockResolvedValue({ + [keyringAccount.id]: 'existing-token', + }); + getTransactionsSpy.mockResolvedValue([]); + getTransactionSpy.mockRejectedValue( + new TransactionNotFoundException('missing-tx-hash-drop'), + ); + + await service.synchronize([activatedAccountPair], scope, []); + + expect(saveManySpy).toHaveBeenCalledWith( + [ + expect.objectContaining({ + id: 'missing-tx-hash-drop', + reconcileAttemptCount: 5, + }), + ], + { + [keyringAccount.id]: { + [scope]: 'existing-token', + }, + }, + ); + }); }); diff --git a/packages/snap/src/services/transaction/TransactionSynchronizeService.ts b/packages/snap/src/services/transaction/TransactionSynchronizeService.ts index f8f9068..aa86c32 100644 --- a/packages/snap/src/services/transaction/TransactionSynchronizeService.ts +++ b/packages/snap/src/services/transaction/TransactionSynchronizeService.ts @@ -29,21 +29,20 @@ import type { KeyringAccountId, StellarKeyringAccount, } from '../account'; +import type { StellarKeyringTransaction } from './api'; import { TransactionOrder } from './api'; import type { Transaction } from './Transaction'; import type { TransactionMapper } from './TransactionMapper'; import type { TransactionRepository } from './TransactionRepository'; import type { StellarAssetMetadata } from '../asset-metadata'; -import type { NetworkService } from '../network'; -import { isPendingTransactionStatus } from './utils'; +import { TransactionNotFoundException, type NetworkService } from '../network'; +import { + isPendingTransactionStatus, + shouldDropPendingTransaction, + toKeyringTransactions, +} from './utils'; import type { ActivatedAccountPair } from '../sync/api'; -/** Pending snap-state txs keyed by account, then by on-chain transaction hash. */ -type PendingTransactionsByAccount = Map< - KeyringAccountId, - Map ->; - /** * Mutable state shared across fetch, map, and persist steps within one sync run. */ @@ -53,10 +52,10 @@ type SyncContext = { keyringAccountsById: Map; /** All snap-managed accounts on `scope`, keyed by address for SEP-41 receive mapping. */ allAccountsByAddress: Map; - pendingByAccount: PendingTransactionsByAccount; + pendingTransactionsById: Map; lastScanTokenByAccountId: Record; /** Mapped keyring txs collected during steps 2–3; persisted and emitted in step 4. */ - transactionsToSave: Record; + transactionsToSave: Record; sep41AssetsMetadata: Record; }; @@ -151,6 +150,7 @@ export class TransactionSynchronizeService { }); } + // #region -------------------- Context Creation -------------------- async #createSyncContext( activatedAccountPairs: ActivatedAccountPair[], scope: KnownCaip2ChainId, @@ -169,12 +169,15 @@ export class TransactionSynchronizeService { } // Use Promise.all (not allSettled): if state cannot be loaded, sync cannot continue. - const [pendingByAccount, lastScanTokenByAccountId, allAccountsByAddress] = - await Promise.all([ - this.#loadPendingTransactionsFromState(keyringAccountIds, scope), - this.#fetchLastScanTokens(keyringAccountIds, scope), - this.#loadAllAccountsByAddress(scope), - ]); + const [ + pendingTransactionsById, + lastScanTokenByAccountId, + allAccountsByAddress, + ] = await Promise.all([ + this.#loadPendingTransactionsFromState(keyringAccountIds, scope), + this.#fetchLastScanTokens(keyringAccountIds, scope), + this.#loadAllAccountsByAddress(scope), + ]); const sep41AssetsMetadata = this.#toSep41AssetsMetadata(sep41Assets); return { @@ -182,7 +185,7 @@ export class TransactionSynchronizeService { keyringAccounts, keyringAccountsById, allAccountsByAddress, - pendingByAccount, + pendingTransactionsById, lastScanTokenByAccountId, transactionsToSave: {}, sep41AssetsMetadata, @@ -230,31 +233,37 @@ export class TransactionSynchronizeService { async #loadPendingTransactionsFromState( keyringAccountIds: KeyringAccountId[], scope: KnownCaip2ChainId, - ): Promise { - const transactions = await this.#transactionRepository.findByAccountIds( - keyringAccountIds, - scope, - ); + ): Promise> { + const transactions = + await this.#transactionRepository.findStellarTransactionsByAccountIds( + keyringAccountIds, + scope, + ); - const pendingByAccount: PendingTransactionsByAccount = new Map(); + const pendingTransactionsById: Map< + TransactionId, + StellarKeyringTransaction + > = new Map(); - // Transaction ids can be shared across accounts (send vs receive), so index by account first. + // Pending txs are unique and belong to the submitting account only. + // Hence we dont need to group by account id. for (const transaction of transactions) { if (isPendingTransactionStatus(transaction.status)) { - this.#setPendingInState(pendingByAccount, transaction); + pendingTransactionsById.set(transaction.id, transaction); } } this.#logger.debug('Pending transactions loaded from snap state', { noOfAccounts: keyringAccountIds.length, noOfStoredTransactions: transactions.length, - noOfPendingTransactions: - this.#getPendingTransactionCount(pendingByAccount), + noOfPendingTransactions: pendingTransactionsById.size, }); - return pendingByAccount; + return pendingTransactionsById; } + // #endregion + // #region -------------------- Main Step to scan transactions -------------------- async #scanAccountTransactionsAndMap(context: SyncContext): Promise { // We use promise.allSettled here because we want to continue the sync even if some accounts fail to fetch. const fetchResults = await batchesAllSettled( @@ -288,26 +297,23 @@ export class TransactionSynchronizeService { let noOfResolved = 0; for (const transaction of transactions) { - const pendingFromState = this.#getPendingFromState( - context.pendingByAccount, - keyringAccount.id, + const pendingTransaction = context.pendingTransactionsById.get( transaction.id, ); + const pendingForAccount = + pendingTransaction?.account === keyringAccount.id + ? pendingTransaction + : undefined; - // Pending tx seen in this scan — skip the reconcile pass below. - if (pendingFromState) { - this.#deletePendingFromState( - context.pendingByAccount, - keyringAccount.id, - transaction.id, - ); + if (pendingForAccount) { + context.pendingTransactionsById.delete(transaction.id); noOfResolved += 1; } this.#appendMappedTransaction(context, { keyringAccount, onChainTransaction: transaction, - pendingFromState, + pendingTransaction: pendingForAccount, }); } @@ -327,110 +333,105 @@ export class TransactionSynchronizeService { async #reconcilePendingTransactionsAndMap( context: SyncContext, ): Promise { - const pendingCountBeforeReconcile = this.#getPendingTransactionCount( - context.pendingByAccount, - ); + const pendingCountBeforeReconcile = context.pendingTransactionsById.size; if (pendingCountBeforeReconcile === 0) { return; } - const pendingByTransactionId: Record = - {}; - - // Group by transaction id so each on-chain hash is fetched once when shared across accounts. - for (const pendingById of context.pendingByAccount.values()) { - for (const pendingFromState of pendingById.values()) { - pushToRecordArray( - pendingByTransactionId, - pendingFromState.id, - pendingFromState, - ); - } - } - - const transactionIdsToFetch = Object.keys(pendingByTransactionId); + const transactionIdsToFetch = Array.from( + context.pendingTransactionsById.keys(), + ); this.#logger.debug('Reconciling pending transactions', { transactionIdsToFetch, }); - // TODO: adding a max reconcile limit to avoid the sync running forever. const fetchResults = await batchesAllSettled( transactionIdsToFetch, 10, - async (transactionId) => ({ - transactionId, - onChainTransaction: await this.#fetchOnChainTransaction( - transactionId, - context.scope, - ), - }), + async (transactionId) => + this.#fetchOnChainTransaction(transactionId, context.scope), ); for (const fetchResult of fetchResults) { if (fetchResult.status === 'rejected') { + const error = fetchResult.reason; + // if the error is a TransactionNotFoundException, + // increment the reconcile attempt and continue. + if (error instanceof TransactionNotFoundException) { + this.#incrementReconcileAttempt(context, error.transactionHash); + continue; + } + // for other errors, log and continue. this.#logger.warn('Failed to fetch on-chain transaction', { error: fetchResult.reason, }); continue; } - const { transactionId, onChainTransaction } = fetchResult.value; - - // Map the on-chain transaction for every account that still has it pending. - for (const pendingFromState of pendingByTransactionId[transactionId] ?? - []) { - const keyringAccount = context.keyringAccountsById.get( - pendingFromState.account, - ); - - if (!keyringAccount) { - this.#logger.warn( - 'Failed to find keyring account for pending transaction', - { transactionId, accountId: pendingFromState.account }, - ); - continue; - } + const onChainTransaction = fetchResult.value; - this.#appendMappedTransaction(context, { - keyringAccount, - onChainTransaction, - pendingFromState, + // Get the pending transaction from the sync context. + // OnChain Transaction Id should be the same as the Pending Transaction Id. + const pendingTransaction = context.pendingTransactionsById.get( + onChainTransaction.id, + ); + if (!pendingTransaction) { + this.#logger.warn('Pending transaction not found after fetch', { + transactionId: onChainTransaction.id, }); + continue; + } - this.#deletePendingFromState( - context.pendingByAccount, - pendingFromState.account, - transactionId, + // Find the keyring account that the pending transaction belongs to. + // The pending transaction should only belong to one single account. + const keyringAccount = context.keyringAccountsById.get( + pendingTransaction.account, + ); + if (!keyringAccount) { + this.#logger.warn( + 'Failed to find keyring account for pending transaction', + { + transactionId: pendingTransaction.id, + accountId: pendingTransaction.account, + }, ); + continue; } + + this.#appendMappedTransaction(context, { + keyringAccount, + onChainTransaction, + pendingTransaction, + }); + + context.pendingTransactionsById.delete(pendingTransaction.id); } - const remainingPendingCount = this.#getPendingTransactionCount( - context.pendingByAccount, - ); + const remainingPendingCount = context.pendingTransactionsById.size; this.#logger.debug('Pending transactions reconciled', { noOfResolved: pendingCountBeforeReconcile - remainingPendingCount, noOfRemaining: remainingPendingCount, }); } + // #endregion #appendMappedTransaction( context: SyncContext, params: { keyringAccount: StellarKeyringAccount; onChainTransaction: Transaction; - pendingFromState?: KeyringTransaction; + pendingTransaction?: KeyringTransaction; }, ): void { - const { keyringAccount, onChainTransaction, pendingFromState } = params; + const { keyringAccount, onChainTransaction, pendingTransaction } = params; const mappedTransaction = this.#transactionMapper.mapTransactionSafe({ transaction: onChainTransaction, keyringAccount, - transactionFromState: pendingFromState, + transactionFromState: pendingTransaction, assetMetadata: context.sep41AssetsMetadata, }); @@ -583,6 +584,9 @@ export class TransactionSynchronizeService { async #saveTransactions(context: SyncContext): Promise { this.#logger.debug('Saving transactions'); const transactions = Object.values(context.transactionsToSave).flat(); + const pendingTransactions = Array.from( + context.pendingTransactionsById.values(), + ); const lastScanTokensByAccountIdWithScope: Record< KeyringAccountId, @@ -598,7 +602,7 @@ export class TransactionSynchronizeService { } await this.#transactionRepository.saveMany( - transactions, + transactions.concat(pendingTransactions), lastScanTokensByAccountIdWithScope, ); this.#logger.debug('Transactions saved'); @@ -612,60 +616,20 @@ export class TransactionSynchronizeService { getSnapProvider(), KeyringEvent.AccountTransactionsUpdated, { - transactions: context.transactionsToSave, + // Strip snap-internal reconcile metadata before keyring emit. + transactions: Object.fromEntries( + Object.entries(context.transactionsToSave).map( + ([accountId, transactions]) => [ + accountId, + toKeyringTransactions(transactions), + ], + ), + ), }, ); this.#logger.debug('Transactions updated event emitted'); } - #getPendingTransactionCount( - pendingByAccount: PendingTransactionsByAccount, - ): number { - let count = 0; - for (const pendingById of pendingByAccount.values()) { - count += pendingById.size; - } - return count; - } - - #getPendingFromState( - pendingByAccount: PendingTransactionsByAccount, - accountId: KeyringAccountId, - transactionId: TransactionId, - ): KeyringTransaction | undefined { - return pendingByAccount.get(accountId)?.get(transactionId); - } - - #deletePendingFromState( - pendingByAccount: PendingTransactionsByAccount, - accountId: KeyringAccountId, - transactionId: TransactionId, - ): void { - const pendingById = pendingByAccount.get(accountId); - if (!pendingById) { - return; - } - - pendingById.delete(transactionId); - - if (pendingById.size === 0) { - pendingByAccount.delete(accountId); - } - } - - #setPendingInState( - pendingByAccount: PendingTransactionsByAccount, - transaction: KeyringTransaction, - ): void { - let pendingById = pendingByAccount.get(transaction.account); - if (!pendingById) { - pendingById = new Map(); - pendingByAccount.set(transaction.account, pendingById); - } - - pendingById.set(transaction.id, transaction); - } - /** * Returns the recipient address when `transaction` is a confirmed SEP-41 send from * `senderKeyringAccount`. @@ -705,4 +669,36 @@ export class TransactionSynchronizeService { return to.address; } + + /** + * Records a Horizon not-found reconcile attempt on a pending transaction. + * Eviction is deferred to {@link TransactionRepository.saveMany} when both + * `maxReconcileAttempts` and `maxPendingTransactionAge` are exceeded. + * + * @param context - Mutable sync run state including pending transactions. + * @param transactionId - Pending transaction hash that Horizon did not return. + */ + #incrementReconcileAttempt( + context: SyncContext, + transactionId: TransactionId, + ): void { + const transaction = context.pendingTransactionsById.get(transactionId); + if (!transaction) { + return; + } + + const reconcileAttemptCount = (transaction.reconcileAttemptCount ?? 0) + 1; + transaction.reconcileAttemptCount = reconcileAttemptCount; + + if (shouldDropPendingTransaction(transaction)) { + this.#logger.debug( + 'Pending transaction eligible for eviction on save after max reconcile attempts and max age are exceeded', + { + transactionId, + accountId: transaction.account, + reconcileAttemptCount, + }, + ); + } + } } diff --git a/packages/snap/src/services/transaction/api.ts b/packages/snap/src/services/transaction/api.ts index 30c982a..b23892b 100644 --- a/packages/snap/src/services/transaction/api.ts +++ b/packages/snap/src/services/transaction/api.ts @@ -1,3 +1,13 @@ +import type { Transaction as KeyringTransaction } from '@metamask/keyring-api'; + +/** + * Snap-state transaction with optional reconcile metadata. + * `reconcileAttemptCount` is internal-only and must be stripped before keyring emit/read APIs. + */ +export type StellarKeyringTransaction = KeyringTransaction & { + reconcileAttemptCount?: number; +}; + /** * The order of onChain transactions to fetch. */ diff --git a/packages/snap/src/services/transaction/utils.ts b/packages/snap/src/services/transaction/utils.ts index 35a2ccd..71bcd28 100644 --- a/packages/snap/src/services/transaction/utils.ts +++ b/packages/snap/src/services/transaction/utils.ts @@ -7,7 +7,7 @@ import type { Operation } from '@stellar/stellar-sdk'; import { Asset } from '@stellar/stellar-sdk'; import { BigNumber } from 'bignumber.js'; -import { StellarOperationType } from './api'; +import { StellarOperationType, type StellarKeyringTransaction } from './api'; import { InvalidInvokeContractStructureException, RequiresMemoException, @@ -25,6 +25,7 @@ import type { KnownCaip2ChainId, } from '../../api'; import { SwapTransactionXdrStruct } from '../../api'; +import { AppConfig } from '../../config'; import { DUST_XLM_AMOUNT } from '../../constants'; import { getSlip44AssetId, @@ -576,3 +577,85 @@ export function isCompletedTransactionStatus( status === `${TransactionStatus.Confirmed}` ); } + +/** + * Strips snap-internal reconcile metadata before exposing a transaction via keyring APIs. + * + * @param transaction - Snap-state transaction that may include `reconcileAttemptCount`. + * @returns A pure keyring transaction. + */ +export function toKeyringTransaction( + transaction: StellarKeyringTransaction, +): KeyringTransaction { + const { + reconcileAttemptCount: _reconcileAttemptCount, + ...keyringTransaction + } = transaction; + return keyringTransaction; +} + +/** + * Converts a list of Stellar transactions to a list of pure keyring transactions. + * + * @param transactions - Snap-state transactions that may include `reconcileAttemptCount`. + * @returns Pure keyring transactions. + */ +export function toKeyringTransactions( + transactions: StellarKeyringTransaction[], +): KeyringTransaction[] { + return transactions.map(toKeyringTransaction); +} + +/** + * Checks if the pending transaction has exceeded the max reconcile attempts. + * + * @param reconcileAttemptCount - Number of Horizon not-found reconcile attempts for a pending tx. + * @returns Whether the reconcile attempt limit has been reached. + */ +export function isReconcileAttemptExceeded( + reconcileAttemptCount: number = 0, +): boolean { + return reconcileAttemptCount >= AppConfig.transaction.maxReconcileAttempts; +} + +/** + * Checks if the pending transaction has exceeded the max pending transaction age. + * + * @param timestamp - Transaction creation time in seconds since epoch. + * @returns Whether the pending transaction is older than the configured max age. + */ +export function isExceedMaxPendingTransactionAge( + timestamp?: number | null, +): boolean { + if (timestamp === undefined || timestamp === null) { + return false; + } + + return ( + Date.now() - timestamp * 1000 > + AppConfig.transaction.maxPendingTransactionAge + ); +} + +/** + * Whether a pending transaction should be evicted from snap state. + * + * Both reconcile attempts and max age must be exceeded so frequent account-switch + * syncs cannot drop a pending tx before it has had enough wall-clock time. + * + * @param transaction - Pending transaction with optional reconcile metadata. + * @param transaction.reconcileAttemptCount - Horizon not-found reconcile attempts for this pending tx. + * @param transaction.timestamp - Transaction creation time in seconds since epoch. + * @returns Whether the pending tx should be dropped from snap state. + */ +export function shouldDropPendingTransaction( + transaction: Pick< + StellarKeyringTransaction, + 'reconcileAttemptCount' | 'timestamp' + >, +): boolean { + return ( + isReconcileAttemptExceeded(transaction.reconcileAttemptCount ?? 0) && + isExceedMaxPendingTransactionAge(transaction.timestamp) + ); +}