fix: WaitTx timeout no longer overwrites a rendered success screen (closes #155)

src/popup/index.js

@@ -165,6 +165,12 @@ function restoreView() {
                 fallbackView();
             }
             break;
+        case "wait-tx":
+            // Resumes the receipt poll from the persisted broadcast time.
+            if (!txStatus.restoreWait()) {
+                fallbackView();
+            }
+            break;
         case "success-tx":
             if (state.viewData && state.viewData.hash) {
                 txStatus.renderSuccess();

src/popup/restorableViews.js

@@ -22,6 +22,7 @@ const RESTORABLE_VIEWS = new Set([
     "settings-addtoken",
     "confirm-tx",
     "transaction",
+    "wait-tx",
     "success-tx",
     "error-tx",
 ]);

src/popup/views/txStatus.js

@@ -16,11 +16,36 @@ const { state, saveState, currentNetwork } = require("../../shared/state");
 const { getProvider } = require("../../shared/balances");
 const { log } = require("../../shared/log");
 
+// Receipt poll cadence and the deadline after which the wait is reported as
+// a timeout. Both are documented in the WaitTx section of README.md.
+const POLL_INTERVAL_MS = 10000;
+const TIMEOUT_MS = 60000;
+
+// How many receipt lookups may fail in a row before the wait is ended and
+// the failure reported. A lookup that throws says nothing about the
+// transaction, so one must not end the wait — but an RPC that never answers
+// (a mistyped URL in settings is the ordinary case) must not leave the wait
+// running forever either, least of all a persisted one that every popup
+// open would resume. Six is 60 seconds at the poll cadence: the same
+// patience the confirmation deadline gets. Any lookup that answers, with a
+// receipt or with null, resets the count.
+const MAX_CONSECUTIVE_LOOKUP_FAILURES = 6;
+
 let ctx;
 let elapsedTimer = null;
 let pollTimer = null;
 
-function clearTimers() {
+// Identifies the wait currently on screen. Bumped by endWait(), so a timer
+// callback or an in-flight receipt lookup that outlives its wait can tell
+// that it is stale and leave the current view alone. Without it, a receipt
+// resolving after the wait has ended renders over whatever view replaced it.
+let waitId = 0;
+
+// End the wait on screen: stop its timers and invalidate its pending async
+// work. Called on receipt, on timeout, when a new wait starts, and when the
+// user navigates away.
+function endWait() {
+    waitId++;
     if (elapsedTimer) {
         clearInterval(elapsedTimer);
         elapsedTimer = null;
@@ -47,8 +72,13 @@ function blockNumberHtml(blockNumber) {
     return copyableHtml(num) + etherscanLinkHtml(link);
 }
 
-function showWait(txInfo, txHash) {
-    clearTimers();
+// Render the wait view and start polling for the receipt. broadcastTime is
+// when the transaction was broadcast, which is what the elapsed counter and
+// the timeout deadline are both measured from; pollNow runs one lookup
+// immediately instead of waiting a full poll interval.
+function startWait(txInfo, txHash, broadcastTime, pollNow) {
+    endWait();
+    const id = waitId;
 
     const symbol = txInfo.token === "ETH" ? "ETH" : txInfo.tokenSymbol || "?";
     $("wait-tx-summary").textContent = txInfo.amount + " " + symbol;
@@ -56,41 +86,130 @@ function showWait(txInfo, txHash) {
     $("wait-tx-hash").innerHTML = txHashHtml(txHash);
     attachCopyHandlers("view-wait-tx");
 
-    const broadcastTime = Date.now();
-    $("wait-tx-status").textContent = "Waiting for confirmation... 0s";
+    // Persisted so closing and reopening the popup resumes this wait
+    // instead of silently abandoning it.
+    state.viewData = {
+        pendingWait: {
+            txInfo: txInfo,
+            hash: txHash,
+            broadcastTime: broadcastTime,
+        },
+    };
 
-    elapsedTimer = setInterval(() => {
+    function renderElapsed() {
         const elapsed = Math.floor((Date.now() - broadcastTime) / 1000);
         $("wait-tx-status").textContent =
             "Waiting for confirmation... " + elapsed + "s";
+    }
+    renderElapsed();
+
+    elapsedTimer = setInterval(() => {
+        if (id !== waitId) return;
+        renderElapsed();
     }, 1000);
 
     const provider = getProvider(state.rpcUrl);
-    pollTimer = setInterval(async () => {
+    let consecutiveFailures = 0;
+
+    async function poll() {
+        if (id !== waitId) return;
+        let receipt = null;
+        let answered = true;
         try {
-            const receipt = await provider.getTransactionReceipt(txHash);
-            if (receipt) {
-                showSuccess(txInfo, txHash, receipt.blockNumber);
-            }
+            receipt = await provider.getTransactionReceipt(txHash);
         } catch (e) {
+            // A thrown lookup means "no answer this tick", not "no
+            // receipt": the RPC failed, the chain said nothing. Declaring
+            // the timeout off it would report a confirmed transaction as
+            // failed — which matters most on a resumed wait, where the
+            // first poll is already past the deadline.
+            answered = false;
             log.errorf("poll receipt failed:", e.message);
         }
-
-        const elapsed = Math.floor((Date.now() - broadcastTime) / 1000);
-        if (elapsed >= 60) {
+        // The lookup is async: the wait may have ended while it was in
+        // flight, in which case this result must not touch the view.
+        if (id !== waitId) return;
+        // Exactly one outcome per wait. A receipt wins even on the tick
+        // that crosses the deadline, because the transaction did confirm.
+        if (receipt) {
+            showSuccess(txInfo, txHash, receipt.blockNumber);
+            return;
+        }
+        if (!answered) {
+            consecutiveFailures++;
+            // The failure is the user's news, and it is a different fact
+            // from "the transaction did not confirm" — the chain was never
+            // asked. Ending the wait here is what keeps it bounded and
+            // gives the user a Done button to leave by.
+            if (consecutiveFailures >= MAX_CONSECUTIVE_LOOKUP_FAILURES) {
+                showError(
+                    txInfo,
+                    txHash,
+                    "The network could not be reached to check this transaction — " +
+                        MAX_CONSECUTIVE_LOOKUP_FAILURES +
+                        " lookups failed in a row. Check the RPC URL in Settings. The transaction may still have confirmed — check Etherscan.",
+                );
+            }
+            // Otherwise keep polling: the next tick may answer.
+            return;
+        }
+        consecutiveFailures = 0;
+        if (Date.now() - broadcastTime >= TIMEOUT_MS) {
             showError(
                 txInfo,
                 txHash,
                 "Transaction was not confirmed within 60 seconds. It may still confirm later \u2014 check Etherscan.",
             );
         }
-    }, 10000);
+    }
+
+    pollTimer = setInterval(poll, POLL_INTERVAL_MS);
 
     showView("wait-tx");
+
+    if (pollNow) poll();
+}
+
+function showWait(txInfo, txHash) {
+    startWait(txInfo, txHash, Date.now(), false);
+}
+
+// Resume a wait persisted by a previous popup session. The deadline still
+// runs from the original broadcast, so a wait that has already outlived it
+// resolves on the immediate first poll rather than restarting the clock.
+// Returns false when there is nothing resumable to resume. Every field
+// startWait() goes on to use is validated, not just the presence of the
+// containers: txInfo.to reaches addressTitle(), which calls
+// address.toLowerCase(), and txInfo.amount is rendered into the summary, so
+// an object merely missing one of them throws a TypeError out of
+// restoreView() — which init() does not guard, skipping the rest of popup
+// init and leaving wait-tx on screen with no back control. A non-numeric
+// broadcastTime leaves an unexitable wait counting "NaNs". txInfo.token and
+// txInfo.tokenSymbol are deliberately unchecked: they are compared and
+// coalesced rather than dereferenced, and tokenSymbol is null for ETH.
+function restoreWait() {
+    const d = state.viewData;
+    if (!d || !d.pendingWait) return false;
+    const w = d.pendingWait;
+    if (!w.hash) return false;
+    // typeof [] is "object", so an array passes an object check.
+    const info = w.txInfo;
+    if (!info || typeof info !== "object" || Array.isArray(info)) return false;
+    // A string is the whole requirement: the empty string is what a
+    // contract-deployment approval persists (approval.js writes `to: toAddr
+    // || ""`), and both fields render harmlessly when empty, so refusing it
+    // would abandon a wait the live path itself created.
+    if (typeof info.to !== "string") return false;
+    if (typeof info.amount !== "string") return false;
+    if (typeof w.broadcastTime !== "number" || !isFinite(w.broadcastTime)) {
+        return false;
+    }
+    startWait(w.txInfo, w.hash, w.broadcastTime, true);
+    return true;
 }
 
 function showSuccess(txInfo, txHash, blockNumber) {
-    clearTimers();
+    endWait();
 
     const symbol = txInfo.token === "ETH" ? "ETH" : txInfo.tokenSymbol || "?";
     state.viewData = {
@@ -182,7 +301,7 @@ function renderSuccess() {
 }
 
 function showError(txInfo, txHash, message) {
-    clearTimers();
+    endWait();
 
     const symbol = txInfo.token === "ETH" ? "ETH" : txInfo.tokenSymbol || "?";
     state.viewData = {
@@ -218,6 +337,9 @@ function isApprovalPopup() {
 }
 
 function navigateBack() {
+    // Nothing should still be polling by now, but leaving a view is the
+    // point at which its timers must be gone.
+    endWait();
     if (isApprovalPopup()) {
         window.close();
         return;
@@ -242,4 +364,12 @@ function init(_ctx) {
     $("btn-error-tx-done").addEventListener("click", navigateBack);
 }
 
-module.exports = { init, showWait, showError, renderSuccess, renderError };
+module.exports = {
+    init,
+    showWait,
+    restoreWait,
+    endWait,
+    showError,
+    renderSuccess,
+    renderError,
+};