feat: Add pending transaction reconcile max attempt and max age in TransactionService#136
feat: Add pending transaction reconcile max attempt and max age in TransactionService#136stanleyyconsensys wants to merge 3 commits into
Conversation
There was a problem hiding this comment.
Pull request overview
Adds bounded eviction for stale pending transactions in snap state by tracking reconcile “not found” attempts and enforcing an age threshold, while ensuring internal-only reconcile metadata is never exposed through keyring APIs.
Changes:
- Introduces
reconcileAttemptCountas snap-internal metadata and strips it on keyring emits/reads. - Updates transaction synchronization to increment reconcile attempts when Horizon returns “not found” and persists updated pending state.
- Evicts pending transactions from snap state only when both max reconcile attempts and max pending age are exceeded (configurable via env/config).
Reviewed changes
Copilot reviewed 10 out of 10 changed files in this pull request and generated 2 comments.
Show a summary per file
| File | Description |
|---|---|
| packages/snap/src/services/transaction/utils.ts | Adds helpers for stripping reconcile metadata and determining when pending txs should be dropped. |
| packages/snap/src/services/transaction/TransactionSynchronizeService.ts | Tracks pending txs by id, increments reconcile attempts on Horizon “not found”, strips internal metadata before keyring emit, and persists pending txs. |
| packages/snap/src/services/transaction/TransactionSynchronizeService.test.ts | Adds coverage for reconcile-attempt increment behavior on “not found”. |
| packages/snap/src/services/transaction/TransactionRepository.ts | Stores StellarKeyringTransaction in snap state, strips metadata on read APIs, and evicts stale pending txs in saveMany. |
| packages/snap/src/services/transaction/TransactionRepository.test.ts | Adds eviction/merge behavior tests for reconcile attempts + max age gating. |
| packages/snap/src/services/transaction/api.ts | Introduces StellarKeyringTransaction type (KeyringTransaction + optional reconcile metadata). |
| packages/snap/src/services/network/exceptions.ts | Extends TransactionNotFoundException to carry the transaction hash for callers. |
| packages/snap/src/config.ts | Adds maxReconcileAttempts and maxPendingTransactionAge configuration knobs. |
| packages/snap/snap.config.ts | Wires new env vars into snap build config. |
| packages/snap/.env.example | Documents the new env vars. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| accountIds: string[], | ||
| scope?: KnownCaip2ChainId, | ||
| ): Promise<KeyringTransaction[]> { | ||
| return toKeyringTransactions( |
There was a problem hiding this comment.
most of the method, we will re-sharp the result to KeyringTransaction
with that we can avoid too much change on handler side
| ); | ||
| } | ||
|
|
||
| async findStellarTransactionsByAccountIds( |
There was a problem hiding this comment.
this is the only method, we return in StellarKeyringTransaction sharp, becoz there is a use case to read the attempt count for a txn
| /** All snap-managed accounts on `scope`, keyed by address for SEP-41 receive mapping. */ | ||
| allAccountsByAddress: Map<StellarAddress, StellarKeyringAccount>; | ||
| pendingByAccount: PendingTransactionsByAccount; | ||
| pendingTransactionsById: Map<TransactionId, StellarKeyringTransaction>; |
There was a problem hiding this comment.
horizon will not return txn that in pending state
Hence pending txn only live when txn send from MM wallet
previously we do make it key by account id, but actually it is not necessary
as each pending txn can only have 1 sender
| {}; | ||
|
|
||
| // Group by transaction id so each on-chain hash is fetched once when shared across accounts. | ||
| for (const pendingById of context.pendingByAccount.values()) { |
There was a problem hiding this comment.
this is no longer need, as we simplify the structure as a single level MAP
| transactionIdsToFetch, | ||
| 10, | ||
| async (transactionId) => ({ | ||
| transactionId, |
There was a problem hiding this comment.
this is no longer need, as the enquiry transactionId should match the transactionId in onChainTransaction
| continue; | ||
| } | ||
|
|
||
| this.#deletePendingFromState( |
There was a problem hiding this comment.
no longer need, we can use the Map delete native feature
|
|
||
| await this.#transactionRepository.saveMany( | ||
| transactions, | ||
| transactions.concat(pendingTransactions), |
There was a problem hiding this comment.
Pass both success txn and pending txn to save
it is not going to happen if a txn in both pending and success, so we should safe to not deduplicate
and when we save, we remove the success txn in state + pending txn that is expired (age and max attempt exceed)
| this.#logger.debug('Transactions updated event emitted'); | ||
| } | ||
|
|
||
| #getPendingTransactionCount( |
There was a problem hiding this comment.
since we simplify the data structure of pending txn , they no longer needed
Explanation
References
Checklist