MetaMask / MetaMask/core

Derive transaction status-change events from state in `#updateTransactionInternal`

Open
#9,412 0 comments 0 reactions 1 assignee Claimed by @matthewwalsh0 View on GitHub
team-confirmations
Dominant language
TypeScript
Stars
413
Forks
308
Avg merge
1d 4h
Merged PRs (30d)
253

Description

## What

Emit transaction status-change events by diffing `status` inside `#updateTransactionInternal`, instead of relying on callers to manually invoke `#onTransactionStatusChange` (and the status-specific `transactionFailed` / `transactionConfirmed` / `transactionDropped` / `transactionFinished` publishes).

## Why

Today these events are fired manually at ~8 separate call sites. That makes emitting an event a "conscious decision" the author has to remember, rather than something derived from the state, which should be the source of truth. Any code path that mutates a transaction's `status` without also calling the emit helpers silently skips the events.

This is exactly what caused https://github.com/MetaMask/core/pull/9400: `SmartTransactionsController` marked a transaction failed via the plain `updateTransaction` action, which doesn't emit `transactionStatusUpdated` / `transactionFailed`, so the bridge status controller was never notified and the transaction stayed stuck as pending.

#9400 fixed the symptom with a targeted `failTransaction` action, but the underlying fragility remains.

## Proposal

- Diff previous vs new `status` inside `#updateTransactionInternal` and publish the appropriate event(s) automatically after the update.
- Remove the ~8 manual `#onTransactionStatusChange` call sites once the diff covers them.
- Consider whether the public `failTransaction` action (added in #9400) can then be removed.

## Risk / notes

Not just `transactionStatusUpdated` — the status-specific events (`transactionFailed`, `transactionConfirmed`, `transactionDropped`, `transactionFinished`) also need to be derived from the diff, and every existing manual emit must be removed to avoid double-firing. This touches every add/approve/sign/submit/confirm/drop/fail path, so it needs careful review and test coverage. Raised as a follow-up from #9400 rather than folded into that fix.

Context: review discussion on https://github.com/MetaMask/core/pull/9400

Contributor guide

No contributing guide indexed for this repository

Assessment

This issue has not been assessed yet.

Get new issues in your inbox

A short digest of beginner-friendly GitHub issues.