fix: WaitTx timeout no longer overwrites a rendered success screen (closes #155)
Some checks failed
check / check (push) Has been cancelled
Some checks failed
check / check (push) Has been cancelled
A poll tick that found a receipt called showSuccess() and then fell through to the elapsed check, so on the tick crossing the 60-second deadline the "Transaction Confirmed" screen was immediately replaced by "not confirmed within 60 seconds" — the user is told a confirmed transaction failed. The wait now has an explicit lifecycle. A wait id is bumped by endWait(), which is called on receipt, on timeout, when a new wait starts and when the user navigates away; every timer callback and every post-await continuation checks it, so exactly one outcome can be rendered per wait and no stale timer or in-flight receipt lookup can touch a view it no longer owns. A receipt lookup that throws is treated as "no answer this tick" rather than "no receipt": the poll returns before the deadline check and keeps running, so one transient RPC failure cannot declare a timeout. This matters most on a resumed wait, whose first poll is immediate and may already be past the deadline, where a single error would otherwise be terminal. Retrying is bounded: six consecutive failed lookups — 60 seconds at the poll cadence, the same patience the confirmation deadline gets — end the wait and report that the network could not be reached, pointing at the RPC URL in Settings. That is a different fact from the timeout, because the chain was never asked, and it says so rather than claiming the transaction did not confirm. Any lookup that answers, with a receipt or with null, resets the count. An unbounded retry would be worse than the bug it avoids: the wait is persisted, so a mistyped RPC URL would leave a wait that every popup open resumes and nothing ever ends, on a view with no exit control of its own. The wait is also persisted (state.viewData.pendingWait) and "wait-tx" is now restorable: reopening the popup resumes the poll with the elapsed counter and the deadline still measured from the original broadcast, instead of silently abandoning the wait. restoreWait() validates every field startWait() goes on to use, not just the presence of the containers — hash, a non-array object txInfo carrying a string to and a string amount, and a finite numeric broadcastTime — and returns false otherwise. txInfo.to reaches addressTitle(), which calls address.toLowerCase(), so a payload merely missing that one field would throw a TypeError out of restoreView(), which init() does not guard: the rest of popup init is skipped and wait-tx stays on screen with no back control. A non-numeric broadcastTime leaves an unexitable wait counting "NaNs". Polling stays in the popup rather than moving to the background, which would depend on setInterval surviving in an MV3 service worker. "wait-tx" is added to src/popup/restorableViews.js, and a test pins its membership. restoreView() refuses any view outside that set, so dropping the entry would kill the resume feature silently — the other tests call restoreWait() directly and never read the set. The 60-second threshold and the timeout copy are unchanged.
This commit is contained in:
440
tests/txStatus.test.js
Normal file
440
tests/txStatus.test.js
Normal file
@@ -0,0 +1,440 @@
|
||||
// Lifecycle tests for the post-broadcast transaction status views
|
||||
// (src/popup/views/txStatus.js).
|
||||
//
|
||||
// The bug these pin down: the receipt poll rendered both outcomes on the tick
|
||||
// that crossed the 60-second deadline, so a confirmed transaction was replaced
|
||||
// by "not confirmed within 60 seconds" — the user is told their transaction
|
||||
// failed when it succeeded. The same shape applies to any callback that
|
||||
// outlives its wait: a receipt lookup still in flight when the view is left
|
||||
// must not render over whatever replaced it.
|
||||
//
|
||||
// Fake timers make the race deterministic: the receipt promise is already
|
||||
// resolved when the deadline tick runs, so in the unfixed code showSuccess()
|
||||
// is always followed by showError() on that tick.
|
||||
//
|
||||
// No network: getProvider is mocked at the module boundary and there is no
|
||||
// jsdom in this repo, so the handful of DOM calls these views make are served
|
||||
// by the stub below.
|
||||
|
||||
jest.mock("../src/shared/log", () => ({
|
||||
log: {
|
||||
debugf: () => {},
|
||||
infof: () => {},
|
||||
warnf: () => {},
|
||||
errorf: () => {},
|
||||
},
|
||||
debugFetch: jest.fn(),
|
||||
setRuntimeDebug: () => {},
|
||||
isDebug: () => false,
|
||||
}));
|
||||
|
||||
const mockReceiptLookup = jest.fn();
|
||||
jest.mock("../src/shared/balances", () => ({
|
||||
getProvider: () => ({ getTransactionReceipt: mockReceiptLookup }),
|
||||
refreshBalances: jest.fn(),
|
||||
}));
|
||||
|
||||
global.fetch = jest.fn(() => {
|
||||
throw new Error("tests must not perform network requests");
|
||||
});
|
||||
|
||||
// ---------------------------------------------------------------------------
|
||||
// Minimal DOM. Every element is created on demand and remembered by id, so a
|
||||
// test can read back what a view wrote into it.
|
||||
// ---------------------------------------------------------------------------
|
||||
|
||||
const elements = new Map();
|
||||
|
||||
function makeElement(id) {
|
||||
const classes = new Set(["view", "hidden"]);
|
||||
const el = {
|
||||
id,
|
||||
textContent: "",
|
||||
innerHTML: "",
|
||||
style: {},
|
||||
classList: {
|
||||
add: (c) => classes.add(c),
|
||||
remove: (c) => classes.delete(c),
|
||||
contains: (c) => classes.has(c),
|
||||
toggle: (c, on) => (on ? classes.add(c) : classes.delete(c)),
|
||||
},
|
||||
addEventListener: () => {},
|
||||
querySelectorAll: () => [],
|
||||
remove: () => {},
|
||||
prepend: () => {},
|
||||
};
|
||||
// Views reach for .parentElement to hide whole sections.
|
||||
Object.defineProperty(el, "parentElement", {
|
||||
get: () => getElement(id + "-parent"),
|
||||
});
|
||||
return el;
|
||||
}
|
||||
|
||||
function getElement(id) {
|
||||
if (!elements.has(id)) elements.set(id, makeElement(id));
|
||||
return elements.get(id);
|
||||
}
|
||||
|
||||
global.document = {
|
||||
getElementById: (id) => getElement(id),
|
||||
// escapeHtml() builds a detached div; textContent in, escaped HTML out.
|
||||
createElement: () => {
|
||||
const el = { innerHTML: "" };
|
||||
Object.defineProperty(el, "textContent", {
|
||||
set(v) {
|
||||
el.innerHTML = String(v)
|
||||
.replace(/&/g, "&")
|
||||
.replace(/</g, "<")
|
||||
.replace(/>/g, ">");
|
||||
},
|
||||
});
|
||||
return el;
|
||||
},
|
||||
body: { prepend: () => {} },
|
||||
addEventListener: () => {},
|
||||
};
|
||||
|
||||
global.window = { location: { search: "" } };
|
||||
|
||||
const stored = {};
|
||||
global.chrome = {
|
||||
storage: {
|
||||
local: {
|
||||
set: (obj) => {
|
||||
Object.assign(stored, obj);
|
||||
return Promise.resolve();
|
||||
},
|
||||
get: () => Promise.resolve(stored),
|
||||
},
|
||||
},
|
||||
};
|
||||
|
||||
const txStatus = require("../src/popup/views/txStatus");
|
||||
const { state } = require("../src/shared/state");
|
||||
const { RESTORABLE_VIEWS } = require("../src/popup/restorableViews");
|
||||
|
||||
const TX_HASH =
|
||||
"0x85215772ed26ea8b39c2b3b18779030487efbe0b5fd7e882592b2f62b837be84";
|
||||
const RECIPIENT = "0x66133E8ea0f5D1d612D2502a968757D1048c214a";
|
||||
const TX_INFO = {
|
||||
to: RECIPIENT,
|
||||
amount: "0.0050",
|
||||
token: "ETH",
|
||||
tokenSymbol: null,
|
||||
};
|
||||
|
||||
// True when a view element is not hidden.
|
||||
function visible(view) {
|
||||
return !getElement("view-" + view).classList.contains("hidden");
|
||||
}
|
||||
|
||||
function waitStatusText() {
|
||||
return getElement("wait-tx-status").textContent;
|
||||
}
|
||||
|
||||
beforeEach(() => {
|
||||
jest.useFakeTimers();
|
||||
jest.setSystemTime(new Date("2026-08-11T12:00:00Z"));
|
||||
elements.clear();
|
||||
mockReceiptLookup.mockReset();
|
||||
state.wallets = [];
|
||||
state.viewData = {};
|
||||
state.viewStack = [];
|
||||
state.currentView = null;
|
||||
txStatus.init({ doRefreshAndRender: jest.fn() });
|
||||
});
|
||||
|
||||
afterEach(() => {
|
||||
txStatus.endWait();
|
||||
jest.useRealTimers();
|
||||
});
|
||||
|
||||
describe("WaitTx receipt/timeout race", () => {
|
||||
test("a receipt arriving on the deadline tick leaves the user on SuccessTx", async () => {
|
||||
// No receipt for the first five polls; the sixth — the tick at
|
||||
// t=60s, which is also the timeout deadline — returns one.
|
||||
mockReceiptLookup
|
||||
.mockResolvedValueOnce(null)
|
||||
.mockResolvedValueOnce(null)
|
||||
.mockResolvedValueOnce(null)
|
||||
.mockResolvedValueOnce(null)
|
||||
.mockResolvedValueOnce(null)
|
||||
.mockResolvedValue({ blockNumber: 21000000 });
|
||||
|
||||
txStatus.showWait(TX_INFO, TX_HASH);
|
||||
expect(visible("wait-tx")).toBe(true);
|
||||
|
||||
await jest.advanceTimersByTimeAsync(60000);
|
||||
|
||||
expect(visible("success-tx")).toBe(true);
|
||||
expect(visible("error-tx")).toBe(false);
|
||||
expect(state.currentView).toBe("success-tx");
|
||||
expect(state.viewData.blockNumber).toBe(21000000);
|
||||
expect(state.viewData.message).toBeUndefined();
|
||||
|
||||
// And nothing is left running to undo it.
|
||||
expect(jest.getTimerCount()).toBe(0);
|
||||
await jest.advanceTimersByTimeAsync(300000);
|
||||
expect(state.currentView).toBe("success-tx");
|
||||
expect(mockReceiptLookup).toHaveBeenCalledTimes(6);
|
||||
});
|
||||
|
||||
test("a genuine timeout still shows ErrorTx with the hash", async () => {
|
||||
mockReceiptLookup.mockResolvedValue(null);
|
||||
|
||||
txStatus.showWait(TX_INFO, TX_HASH);
|
||||
await jest.advanceTimersByTimeAsync(60000);
|
||||
|
||||
expect(visible("error-tx")).toBe(true);
|
||||
expect(state.currentView).toBe("error-tx");
|
||||
expect(state.viewData.message).toMatch(
|
||||
/not confirmed within 60 seconds/,
|
||||
);
|
||||
expect(state.viewData.hash).toBe(TX_HASH);
|
||||
// The hash section carries the hash and the etherscan link.
|
||||
expect(getElement("error-tx-hash").innerHTML).toContain(TX_HASH);
|
||||
expect(getElement("error-tx-hash").innerHTML).toContain(
|
||||
"/tx/" + TX_HASH,
|
||||
);
|
||||
expect(jest.getTimerCount()).toBe(0);
|
||||
});
|
||||
|
||||
test("a receipt still in flight when the view is left does not render over it", async () => {
|
||||
let resolveReceipt;
|
||||
mockReceiptLookup.mockReturnValue(
|
||||
new Promise((r) => {
|
||||
resolveReceipt = r;
|
||||
}),
|
||||
);
|
||||
|
||||
txStatus.showWait(TX_INFO, TX_HASH);
|
||||
await jest.advanceTimersByTimeAsync(10000);
|
||||
expect(mockReceiptLookup).toHaveBeenCalledTimes(1);
|
||||
|
||||
// User leaves the wait (popup navigation / teardown) while the
|
||||
// lookup is outstanding, then the lookup finally answers.
|
||||
txStatus.endWait();
|
||||
state.currentView = "main";
|
||||
resolveReceipt({ blockNumber: 21000000 });
|
||||
await Promise.resolve();
|
||||
await Promise.resolve();
|
||||
|
||||
expect(state.currentView).toBe("main");
|
||||
expect(visible("success-tx")).toBe(false);
|
||||
});
|
||||
|
||||
test("no timer survives the view being left", async () => {
|
||||
mockReceiptLookup.mockResolvedValue(null);
|
||||
|
||||
txStatus.showWait(TX_INFO, TX_HASH);
|
||||
expect(jest.getTimerCount()).toBeGreaterThan(0);
|
||||
|
||||
txStatus.endWait();
|
||||
expect(jest.getTimerCount()).toBe(0);
|
||||
|
||||
await jest.advanceTimersByTimeAsync(120000);
|
||||
expect(mockReceiptLookup).not.toHaveBeenCalled();
|
||||
});
|
||||
});
|
||||
|
||||
describe("WaitTx persistence across popup close", () => {
|
||||
test("restoreWait resumes the poll with the deadline running from broadcast", async () => {
|
||||
mockReceiptLookup.mockResolvedValue(null);
|
||||
|
||||
txStatus.showWait(TX_INFO, TX_HASH);
|
||||
expect(state.viewData.pendingWait.hash).toBe(TX_HASH);
|
||||
const persisted = JSON.parse(JSON.stringify(state.viewData));
|
||||
|
||||
// Popup closes: timers die with the page.
|
||||
txStatus.endWait();
|
||||
|
||||
// 45 seconds pass with the popup shut, then it is reopened.
|
||||
jest.advanceTimersByTime(45000);
|
||||
state.viewData = persisted;
|
||||
expect(txStatus.restoreWait()).toBe(true);
|
||||
|
||||
expect(visible("wait-tx")).toBe(true);
|
||||
// Elapsed is counted from the broadcast, not from the reopen.
|
||||
expect(waitStatusText()).toBe("Waiting for confirmation... 45s");
|
||||
// The immediate poll on resume has already run.
|
||||
await Promise.resolve();
|
||||
expect(mockReceiptLookup).toHaveBeenCalledTimes(1);
|
||||
|
||||
// The deadline is 15 seconds away, not 60.
|
||||
await jest.advanceTimersByTimeAsync(20000);
|
||||
expect(state.currentView).toBe("error-tx");
|
||||
});
|
||||
|
||||
test("a rejected lookup on the resume poll keeps waiting instead of reporting failure", async () => {
|
||||
// A wait resumed after the deadline has already passed: the first
|
||||
// poll is immediate and past 60s, so a thrown lookup must not be
|
||||
// read as "no receipt". It means "no answer this tick" — keep
|
||||
// polling, because the transaction may well have confirmed.
|
||||
mockReceiptLookup.mockResolvedValue(null);
|
||||
txStatus.showWait(TX_INFO, TX_HASH);
|
||||
const persisted = JSON.parse(JSON.stringify(state.viewData));
|
||||
txStatus.endWait();
|
||||
|
||||
// Ten minutes with the popup shut, then it is reopened and the
|
||||
// first receipt lookup fails transiently.
|
||||
jest.advanceTimersByTime(600000);
|
||||
mockReceiptLookup.mockReset();
|
||||
mockReceiptLookup
|
||||
.mockRejectedValueOnce(new Error("rpc unavailable"))
|
||||
.mockResolvedValue({ blockNumber: 21000000 });
|
||||
|
||||
state.viewData = persisted;
|
||||
expect(txStatus.restoreWait()).toBe(true);
|
||||
await jest.advanceTimersByTimeAsync(0);
|
||||
|
||||
// The wait is still alive: no timeout was declared off one error.
|
||||
expect(visible("wait-tx")).toBe(true);
|
||||
expect(visible("error-tx")).toBe(false);
|
||||
expect(state.currentView).toBe("wait-tx");
|
||||
expect(jest.getTimerCount()).toBeGreaterThan(0);
|
||||
|
||||
// And the next tick answers, so the confirmed transaction is
|
||||
// reported as confirmed.
|
||||
await jest.advanceTimersByTimeAsync(10000);
|
||||
expect(state.currentView).toBe("success-tx");
|
||||
expect(state.viewData.blockNumber).toBe(21000000);
|
||||
});
|
||||
|
||||
test("a lookup returning null past the deadline still times out", async () => {
|
||||
// The counterpart to the test above: the deadline must still fire
|
||||
// when the lookup actually answers "no receipt".
|
||||
mockReceiptLookup.mockResolvedValue(null);
|
||||
txStatus.showWait(TX_INFO, TX_HASH);
|
||||
const persisted = JSON.parse(JSON.stringify(state.viewData));
|
||||
txStatus.endWait();
|
||||
|
||||
jest.advanceTimersByTime(600000);
|
||||
state.viewData = persisted;
|
||||
expect(txStatus.restoreWait()).toBe(true);
|
||||
await jest.advanceTimersByTimeAsync(0);
|
||||
|
||||
expect(state.currentView).toBe("error-tx");
|
||||
expect(state.viewData.message).toMatch(
|
||||
/not confirmed within 60 seconds/,
|
||||
);
|
||||
});
|
||||
|
||||
test("restoreWait reports nothing to resume when no wait is persisted", () => {
|
||||
state.viewData = {};
|
||||
expect(txStatus.restoreWait()).toBe(false);
|
||||
expect(jest.getTimerCount()).toBe(0);
|
||||
});
|
||||
|
||||
test("restoreWait rejects a persisted wait missing its txInfo or broadcast time", () => {
|
||||
for (const bad of [
|
||||
{ hash: TX_HASH, broadcastTime: Date.now() },
|
||||
{ hash: TX_HASH, txInfo: TX_INFO },
|
||||
{ hash: TX_HASH, txInfo: TX_INFO, broadcastTime: "soon" },
|
||||
{ hash: TX_HASH, txInfo: TX_INFO, broadcastTime: NaN },
|
||||
{ hash: TX_HASH, txInfo: "nope", broadcastTime: Date.now() },
|
||||
// An object that merely lacks a field startWait() dereferences
|
||||
// is the shape that actually escaped: txInfo.to reaches
|
||||
// addressTitle(), which calls address.toLowerCase(). typeof []
|
||||
// is "object", so an array passes an object check.
|
||||
{ hash: TX_HASH, txInfo: {}, broadcastTime: Date.now() },
|
||||
{ hash: TX_HASH, txInfo: [], broadcastTime: Date.now() },
|
||||
{ hash: TX_HASH, txInfo: { to: 42 }, broadcastTime: Date.now() },
|
||||
{
|
||||
hash: TX_HASH,
|
||||
txInfo: { to: RECIPIENT },
|
||||
broadcastTime: Date.now(),
|
||||
},
|
||||
]) {
|
||||
state.viewData = { pendingWait: bad };
|
||||
expect(txStatus.restoreWait()).toBe(false);
|
||||
expect(jest.getTimerCount()).toBe(0);
|
||||
}
|
||||
});
|
||||
});
|
||||
|
||||
describe("WaitTx against an RPC that never answers", () => {
|
||||
test("a permanently failing lookup ends the wait instead of polling forever", async () => {
|
||||
mockReceiptLookup.mockRejectedValue(new Error("rpc unavailable"));
|
||||
|
||||
txStatus.showWait(TX_INFO, TX_HASH);
|
||||
|
||||
// Six consecutive failures is 60 seconds at the 10s cadence — the
|
||||
// same patience as the confirmation deadline.
|
||||
await jest.advanceTimersByTimeAsync(60000);
|
||||
|
||||
expect(state.currentView).toBe("error-tx");
|
||||
expect(visible("wait-tx")).toBe(false);
|
||||
// The user is told what actually happened: the lookup failed. It is
|
||||
// not the same fact as "the transaction did not confirm".
|
||||
expect(state.viewData.message).toMatch(/could not be reached/i);
|
||||
expect(state.viewData.message).not.toMatch(/not confirmed within/);
|
||||
expect(state.viewData.hash).toBe(TX_HASH);
|
||||
|
||||
// Nothing is left running, and nothing is left to resume onto.
|
||||
expect(jest.getTimerCount()).toBe(0);
|
||||
expect(state.viewData.pendingWait).toBeUndefined();
|
||||
|
||||
const calls = mockReceiptLookup.mock.calls.length;
|
||||
await jest.advanceTimersByTimeAsync(3600000);
|
||||
expect(mockReceiptLookup).toHaveBeenCalledTimes(calls);
|
||||
expect(state.currentView).toBe("error-tx");
|
||||
});
|
||||
|
||||
test("an answered lookup clears the failure count, so the bound is on consecutive failures", async () => {
|
||||
// Failures interleaved with answers must never accumulate into the
|
||||
// bound: an RPC that is merely flaky keeps waiting for the receipt.
|
||||
mockReceiptLookup.mockImplementation(() => {
|
||||
const n = mockReceiptLookup.mock.calls.length;
|
||||
if (n % 2 === 1) return Promise.reject(new Error("flaky"));
|
||||
return Promise.resolve(null);
|
||||
});
|
||||
|
||||
txStatus.showWait(TX_INFO, TX_HASH);
|
||||
await jest.advanceTimersByTimeAsync(50000);
|
||||
|
||||
// Five polls in: three threw, two answered null, and the answered
|
||||
// ones are all before the deadline. Still waiting.
|
||||
expect(state.currentView).toBe("wait-tx");
|
||||
expect(visible("wait-tx")).toBe(true);
|
||||
expect(jest.getTimerCount()).toBeGreaterThan(0);
|
||||
|
||||
// Past the deadline the first lookup that answers "no receipt"
|
||||
// still times out, with the timeout copy rather than the RPC copy.
|
||||
await jest.advanceTimersByTimeAsync(70000);
|
||||
expect(state.currentView).toBe("error-tx");
|
||||
expect(state.viewData.message).toMatch(/not confirmed within/);
|
||||
});
|
||||
|
||||
test("a resumed wait against a dead RPC also terminates", async () => {
|
||||
// The reopen path is the one that made this unbounded: the wait is
|
||||
// persisted, so without a bound every popup open resumes it forever.
|
||||
mockReceiptLookup.mockResolvedValue(null);
|
||||
txStatus.showWait(TX_INFO, TX_HASH);
|
||||
const persisted = JSON.parse(JSON.stringify(state.viewData));
|
||||
txStatus.endWait();
|
||||
|
||||
jest.advanceTimersByTime(3600000);
|
||||
mockReceiptLookup.mockReset();
|
||||
mockReceiptLookup.mockRejectedValue(new Error("rpc unavailable"));
|
||||
|
||||
state.viewData = persisted;
|
||||
expect(txStatus.restoreWait()).toBe(true);
|
||||
await jest.advanceTimersByTimeAsync(60000);
|
||||
|
||||
expect(state.currentView).toBe("error-tx");
|
||||
expect(state.viewData.message).toMatch(/could not be reached/i);
|
||||
expect(jest.getTimerCount()).toBe(0);
|
||||
expect(state.viewData.pendingWait).toBeUndefined();
|
||||
});
|
||||
});
|
||||
|
||||
describe("wait-tx is a view the popup may reopen onto", () => {
|
||||
// The resume feature is wired through RESTORABLE_VIEWS: restoreView()
|
||||
// refuses any view not in the set, so dropping "wait-tx" from it kills
|
||||
// the resume silently — the tests above call restoreWait() directly and
|
||||
// would all still pass. This pins the membership. Mirrors the exclusion
|
||||
// assertions in tests/showPhrase.test.js.
|
||||
test("wait-tx is restorable", () => {
|
||||
expect(RESTORABLE_VIEWS.has("wait-tx")).toBe(true);
|
||||
});
|
||||
});
|
||||
Reference in New Issue
Block a user