Compare commits

...

2 Commits

Author SHA1 Message Date
852798d93a feat: remove an address from an HD wallet, behind a confirmation (closes #162)
All checks were successful
check / check (push) Successful in 24s
Address rows on Home now carry an [x] control on wallets that derive their
addresses from an extended key and hold more than one; it opens a confirmation
screen before anything is removed.

Removing an address destroys nothing: it stays derivable from key material the
wallet still holds, and any funds at it stay where they are. That is also why
the screen is not password-gated, unlike delete-wallet — a password gates the
disclosure or destruction of a secret, and this does neither.

Getting the address back into the list is another matter, and the copy states
it exactly rather than promising a route the app refuses. "+" derives the next
unused index, because the derivation counter is a high-water mark and is not
rewound, and re-importing the wallet's key material is rejected as a duplicate
for as long as the wallet is present — which it always is here, since a wallet
never gives up its last address. What works is deleting the whole wallet in
Settings, which asks for the password and destroys the stored secret, then
importing again: the scan that follows rediscovers the address only if it has
on-chain activity, and an address that was never used does not come back at
all. The text is built by recoveryPathText() rather than sitting in index.html
so it can name the wallet's own kind of key material, an xprv wallet having no
recovery phrase to re-import.

A balance is surfaced as a warning, never a refusal, and holding something
means any ERC-20 as well as ETH, at any size: an address with no ETH and a
stablecoin position must not get a blank line on the screen whose job is to
warn. The warning names no figure of its own, because the balance lines round
to four decimals and a sentence built from a rounded number would report
0.0000 ETH for an address holding real money; the amounts come from the same
balanceLinesForAddress() and getAddressValueUsd() every other screen uses.

The state transition lives next to the wallet one in
src/shared/walletDelete.js and shares its address comparison, site-permission
cleanup and broadcast, so the rules match one level down: the last address of
a wallet is never removable, the selection moves only when it was the address
removed, an index after the splice is decremented, a selection in another
wallet is untouched, and AUTISTMASK_ACTIVE_CHANGED is broadcast when the
active address moves so a connected site stops being told about an address the
user removed.
2026-08-12 08:44:38 +00:00
bd4bdcafc7 fix: explain a stored non-master xprv wallet instead of throwing at signing time (closes #234)
All checks were successful
check / check (push) Successful in 30s
2026-08-12 10:41:49 +02:00
17 changed files with 1414 additions and 36 deletions

View File

@@ -130,8 +130,11 @@ transfer, and the recovery phrase screen — which wallet types are offered it,
that it holds nothing before the password is accepted, that a wrong password
reveals nothing, that leaving it by either route wipes it — including a leave
taken while the decrypt is still running — and that reopening the popup does not
land on it. All outbound network is intercepted at the browser level and served
from fixtures in `tests/e2e/network.js`, so the run is deterministic and fully
land on it. It also covers address removal: which wallets offer the control at
all, that the confirmation states the route back rather than showing an empty
paragraph, that leaving the confirmation removes nothing, and that confirming it
does. All outbound network is intercepted at the browser level and served from
fixtures in `tests/e2e/network.js`, so the run is deterministic and fully
offline; unrecognised outbound requests are reported as failures rather than
silently allowed.
@@ -436,7 +439,11 @@ The core hierarchy is **Wallets → Addresses**:
multi-address behavior as an HD wallet, including the "+" button and the
address scan on import, but imported from an extended private key rather
than a recovery phrase. It therefore has no recovery phrase to display or
back up.
back up. Only a master key may be imported; an xprv wallet already in
storage that was imported from a non-master key is detected from the depth
of its stored `xpub` by `src/shared/walletDefects.js`, explained in the
wallet list, and blocked from signing, sending and private-key export. It
is never deleted or rewritten.
- An **address** holds ETH and ERC-20 tokens.
- The user can have multiple wallets, each with multiple addresses (HD) or a
single address (key).
@@ -509,8 +516,9 @@ of it.
- Wallet list: each wallet shows its name (tap to rename inline) and a "+"
button for HD and xprv wallets, then one block per address with "Address
N" (bold when active), the ENS name if resolved, the full address, an
`[info]` button, the address USD total, and a balance line for ETH and for
each token shown for that address
`[info]` button, an `[x]` button (only on HD and xprv wallets holding more
than one address), the address USD total, and a balance line for ETH and
for each token shown for that address
- "Recent Transactions": up to 25 transactions merged across every address
of every wallet, deduplicated by hash and filtered
- "Add additional wallet..." link at bottom
@@ -520,6 +528,7 @@ of it.
- Tap wallet name → inline rename field (no screen change)
- "+" on wallet → derives the next address inline (no screen change)
- `[info]` on address → **AddressDetail**
- `[x]` on address → **DeleteAddress**
- "Send" → **Send** (refuses with a flash message on a zero balance)
- "Receive" → **Receive** (shows active address QR)
- Tap home tx row → **TransactionDetail**
@@ -880,6 +889,55 @@ of it.
nothing deleted
- "Back" → previous screen (Settings)
#### DeleteAddress (`delete-address-confirm`)
- **When**: User tapped the `[x]` next to an address on Home. Offered only on HD
and xprv wallets holding more than one address: the last address of a wallet
is never removable, and a key wallet has exactly one.
- **Elements**:
- "Back" button, "Remove Address" heading
- The address's own label ("Address N") and its wallet's name
- The full address (color dot, etherscan link, tap to copy), with the ENS
name above it if resolved
- Explanation that this only stops the wallet tracking the address: nothing
is destroyed, no key is deleted, and funds stay where they are
- The route back, stated with its limit, because the obvious two are both
refused: "+" derives the next unused index (`nextIndex` is a high-water
mark), and re-importing the wallet's key material is rejected as a
duplicate by `findWalletByXpub` while the wallet is still present. What
works is deleting the whole wallet in Settings — password-gated, and it
destroys the stored secret — then importing again, whereupon
`scanForAddresses()` rediscovers the address **only if it has on-chain
activity**. An address that was never used does not come back. The text is
written by `recoveryPathText()` rather than sitting in `index.html`, so it
can name the wallet's own kind of key material: an xprv wallet has no
recovery phrase to re-import.
- A warning when the address holds anything, ETH or any tracked ERC-20,
followed by the holdings themselves via `balanceLinesForAddress()` and the
USD total via `getAddressValueUsd()`. The sentence names no figure of its
own: the lines round to four decimals, so a sentence built from a rounded
number would report `0.0000 ETH` for an address holding real money. The
predicate is `addressHoldsFunds()` in `src/popup/views/helpers.js`,
unrounded and token-aware. A balance is a warning, never a refusal.
- The rule that a wallet always keeps at least one address, and that
removing the last one means deleting the wallet from Settings
- Error line
- "Remove Address" button
- **Transitions**:
- "Remove Address" → removes the address and its site permissions, then →
previous screen (Home) with an "Address removed." flash message
- "Back" → previous screen (Home), nothing removed
- **Deliberately not password-gated**, unlike DeleteWallet: a password gates the
disclosure or destruction of a secret, and this does neither. The address
stays derivable from key material the wallet still holds.
- The active address moves only if it was the address removed, and then to the
wallet's first remaining address, with `AUTISTMASK_ACTIVE_CHANGED` broadcast
so a connected site stops being told about an address the user removed
(`src/shared/walletDelete.js`). A selection in any other wallet is left alone;
one in this wallet follows the splice.
- The wallet's derivation counter (`nextIndex`) is not rewound, so "+" derives a
fresh address rather than handing back the one just removed.
#### SettingsAddToken (`settings-addtoken`)
- **When**: User tapped "+ Add token" in Settings. Tokens added here are tracked
@@ -1363,7 +1421,7 @@ Currently supported:
### Wallet Management
- [x] Delete wallet (with confirmation)
- [ ] Delete address from HD wallet (with confirmation)
- [x] Delete address from HD wallet (with confirmation)
- [x] Show wallet's recovery phrase (requires password)
### Transactions

10
TODO.md
View File

@@ -44,6 +44,16 @@ undefined identifiers, which is how
# Completed Steps
- 2026-08-12: An address can be removed from an HD or xprv wallet behind a
confirmation screen that states nothing is destroyed, sharing the deletion
state transitions with wallet deletion so the selection, site permissions and
active-address broadcast follow the same rules
([#162](https://git.eeqj.de/sneak/AutistMask/issues/162)).
- 2026-08-12: An xprv wallet already in storage that was imported from a
non-master key is detected from the depth of its stored `xpub`, explained in
the wallet list, and blocked from signing, sending and private-key export
instead of throwing on the send screen
([#234](https://git.eeqj.de/sneak/AutistMask/issues/234)).
- 2026-08-12: An unreported `holders_count` is now parsed as `null` rather than
`0`, so the low-holder rule declines to judge an unknown count instead of
hiding a legitimate token as spam, in both the transaction history and the

View File

@@ -1142,6 +1142,62 @@
</button>
</div>
<!-- ============ DELETE ADDRESS CONFIRM ============ -->
<div id="view-delete-address-confirm" class="view hidden">
<button
id="btn-delete-address-back"
class="border border-border px-2 py-1 hover:bg-fg hover:text-bg cursor-pointer mb-2"
>
&lt; Back
</button>
<h2 class="font-bold mb-3">Remove Address</h2>
<p class="text-xs mb-2">
You are about to remove
<strong id="delete-address-label"></strong> from
<strong id="delete-address-wallet-name"></strong>.
</p>
<div
id="delete-address-value"
class="text-xs mb-2 break-all min-h-[1rem]"
></div>
<div
class="text-xs mb-2 border border-border border-dashed p-2"
>
This only stops this wallet from tracking the address.
Nothing is destroyed and no key is deleted. Any funds at the
address stay exactly where they are, and the address remains
yours. Any site permissions granted to this address are
forgotten.
</div>
<!-- Filled by src/popup/views/deleteAddress.js: the route
back names the wallet's own kind of key material. -->
<div
id="delete-address-recovery"
class="text-xs mb-2 border border-border border-dashed p-2"
></div>
<div
id="delete-address-balance"
class="text-xs mb-2 min-h-[1.25rem] pointer-events-none"
>
&nbsp;
</div>
<p class="text-xs text-muted mb-3">
A wallet always keeps at least one address. To remove the
last one, delete the whole wallet from Settings instead.
</p>
<div
id="delete-address-flash"
class="text-xs text-red-500 mb-2 min-h-[1.25rem]"
style="visibility: hidden"
></div>
<button
id="btn-delete-address-confirm"
class="border border-border text-red-500 px-2 py-1 hover:bg-fg hover:text-bg cursor-pointer"
>
Remove Address
</button>
</div>
<!-- ============ SHOW RECOVERY PHRASE ============ -->
<div id="view-show-phrase" class="view hidden">
<button

View File

@@ -33,6 +33,7 @@ const receive = require("./views/receive");
const addToken = require("./views/addToken");
const settings = require("./views/settings");
const settingsAddToken = require("./views/settingsAddToken");
const deleteAddress = require("./views/deleteAddress");
const approval = require("./views/approval");
function renderWalletList() {
@@ -101,6 +102,10 @@ const ctx = {
pushCurrentView();
settingsAddToken.show();
},
showDeleteAddress: (walletIdx, addrIdx) => {
pushCurrentView();
deleteAddress.show(walletIdx, addrIdx);
},
};
function needsAddress(view) {
@@ -250,6 +255,7 @@ async function init() {
addToken.init(ctx);
settings.init(ctx);
settingsAddToken.init(ctx);
deleteAddress.init(ctx);
if (!state.hasWallet) {
showView("welcome");

View File

@@ -29,6 +29,16 @@ const { log } = require("../../shared/log");
const makeBlockie = require("ethereum-blockies-base64");
const { decryptWithPassword } = require("../../shared/vault");
const { getSignerForAddress } = require("../../shared/wallet");
const { walletDefect } = require("../../shared/walletDefects");
// The defect of the wallet the selected address belongs to, or null. Both the
// send and the private-key export path check it before asking for a password,
// so a wallet that cannot derive its keys says so instead of failing after the
// user has typed one in.
function selectedWalletDefect() {
if (state.selectedWallet === null) return null;
return walletDefect(state.wallets[state.selectedWallet]);
}
let ctx;
@@ -254,6 +264,11 @@ function init(_ctx) {
});
$("btn-send").addEventListener("click", () => {
const defect = selectedWalletDefect();
if (defect) {
showFlash(defect.shortMessage);
return;
}
const addr =
state.wallets[state.selectedWallet].addresses[
state.selectedAddress
@@ -298,6 +313,14 @@ function init(_ctx) {
$("btn-export-privkey").addEventListener("click", () => {
moreDropdown.classList.add("hidden");
moreBtn.classList.remove("bg-fg", "text-bg");
// There is no private key to export for an address this wallet
// cannot derive. Without this the export screen would take a
// password and then report it as wrong.
const defect = selectedWalletDefect();
if (defect) {
showFlash(defect.shortMessage);
return;
}
pushCurrentView();
const wallet = state.wallets[state.selectedWallet];
const addr = wallet.addresses[state.selectedAddress];

View File

@@ -35,6 +35,7 @@ const {
} = require("./send");
const { log } = require("../../shared/log");
const makeBlockie = require("ethereum-blockies-base64");
const { walletDefect } = require("../../shared/walletDefects");
let ctx;
@@ -338,6 +339,11 @@ function init(_ctx) {
});
$("btn-address-token-send").addEventListener("click", () => {
const defect = walletDefect(state.wallets[state.selectedWallet]);
if (defect) {
showFlash(defect.shortMessage);
return;
}
const addr =
state.wallets[state.selectedWallet].addresses[
state.selectedAddress

View File

@@ -21,6 +21,7 @@ const { ERC20_ABI } = require("../../shared/constants");
const { TOKEN_BY_ADDRESS } = require("../../shared/tokenList");
const { decryptWithPassword } = require("../../shared/vault");
const { getSignerForAddress } = require("../../shared/wallet");
const { walletDefect } = require("../../shared/walletDefects");
const { getProvider } = require("../../shared/balances");
const txStatus = require("./txStatus");
const uniswap = require("../../shared/uniswap");
@@ -280,6 +281,7 @@ function showTxApproval(details) {
showView("approve-tx");
attachCopyHandlers("view-approve-tx");
gateOnWalletDefect("approve-tx-error", "btn-approve-tx");
}
function decodeHexMessage(hex) {
@@ -379,6 +381,7 @@ function showSignApproval(details) {
showView("approve-sign");
attachCopyHandlers("view-approve-sign");
gateOnWalletDefect("approve-sign-error", "btn-approve-sign");
}
function show(id) {
@@ -431,6 +434,20 @@ function setSignButtonBusy(busy) {
$("btn-approve-sign").classList.toggle("text-muted", busy);
}
// Say so on the approval screen itself, and disable the approve button, when
// the active address belongs to a wallet whose keys cannot be derived. Without
// this the screen would take a password and fail after deriving it. Reject
// stays available; the wallet is not touched. Returns true when it gated.
function gateOnWalletDefect(errorId, buttonId) {
const active = findActiveWallet();
const defect = active ? walletDefect(active.wallet) : null;
if (!defect) return false;
showError(errorId, defect.shortMessage);
$(buttonId).disabled = true;
$(buttonId).classList.add("text-muted");
return true;
}
// Locate the wallet and the address index owning the currently active
// address. Returns null when no wallet holds it.
function findActiveWallet() {
@@ -492,6 +509,14 @@ function init(ctx) {
return;
}
const defect = walletDefect(active.wallet);
if (defect) {
password = null;
showError("approve-tx-error", defect.shortMessage);
setTxButtonBusy(false);
return;
}
// Decrypt here, in the popup. The password must never cross the
// extension messaging boundary; only the signed transaction does.
let decryptedSecret;
@@ -583,6 +608,14 @@ function init(ctx) {
return;
}
const defect = walletDefect(active.wallet);
if (defect) {
password = null;
showError("approve-sign-error", defect.shortMessage);
setSignButtonBusy(false);
return;
}
// Decrypt here, in the popup. The password must never cross the
// extension messaging boundary; only the signature does.
let decryptedSecret;

View File

@@ -0,0 +1,176 @@
// Confirmation screen for removing one address from a wallet that derives
// its addresses from an extended key.
//
// No password is asked for, unlike delete-wallet. A password gates the
// disclosure or destruction of a secret, and this does neither: the address
// is derived from key material the wallet still holds, so removing it only
// stops the wallet tracking it. An explicit confirmation screen is the
// proportionate treatment.
const {
$,
showView,
showFlash,
goBack,
renderAddressHtml,
attachCopyHandlers,
addressHoldsFunds,
balanceLinesForAddress,
} = require("./helpers");
const { formatUsd, getAddressValueUsd } = require("../../shared/prices");
const { walletHasRecoveryPhrase } = require("../../shared/wallet");
const { state, saveState } = require("../../shared/state");
const {
canRemoveAddress,
removeAddressFromState,
broadcastActiveChanged,
} = require("../../shared/walletDelete");
// The wallet and address indices this screen is confirming, or null when it
// is not confirming anything.
let target = null;
let ctx = null;
function setFlash(msg) {
const el = $("delete-address-flash");
el.textContent = msg;
el.style.visibility = msg ? "visible" : "hidden";
}
// What it actually takes to get the address back, which is not what the
// screen used to claim.
//
// Neither obvious route works: "+" derives the next unused index, because
// wallet.nextIndex is a high-water mark and is deliberately not rewound; and
// re-importing this wallet's key material is refused as a duplicate by
// findWalletByXpub() for as long as the wallet is here. What remains is to
// delete the whole wallet in Settings — which asks for the password and
// destroys the stored secret — and import again, after which
// scanForAddresses() rediscovers the address only if it has on-chain
// activity. An address that was never used is not found by that scan, and
// the copy must not imply otherwise.
//
// The noun follows the wallet: an xprv wallet holds no recovery phrase, and
// this screen is offered on xprv wallets too.
function recoveryPathText(wallet) {
const secret = walletHasRecoveryPhrase(wallet)
? "recovery phrase"
: "extended private key";
return (
"Getting the address back into this list is not easy, so be sure. " +
"Adding an address derives the next unused one, not this one, and " +
"importing this " +
secret +
" again is refused while this wallet is still here. The way back is " +
"to delete the whole wallet in Settings, which asks for your " +
"password and destroys the stored " +
secret +
", and then import that " +
secret +
" again. The scan that follows only finds addresses that have " +
"on-chain activity, so an address that has never been used is not " +
"found by it."
);
}
// The balance warning, or a blank line when the address holds nothing.
//
// A balance is a reason to be careful, not a reason to refuse: the funds are
// at the address, not in this list, and stay there either way.
//
// "Holds" means ETH or any ERC-20 the wallet knows about — an address with no
// ETH and a five-figure stablecoin position must not get the blank line on
// the one screen whose job is to warn. The sentence names no figure of its
// own: the rendered lines round to four decimals, so a sentence built from a
// rounded number would report "0.0000 ETH" for an address holding real money.
// The lines below it carry the amounts, in the same format as Home and
// AddressDetail, followed by the USD total when prices are known (null on
// testnet and before the first price fetch, where the line is left off rather
// than printed as $0.00).
function balanceWarningHtml(addr) {
if (!addressHoldsFunds(addr)) return "&nbsp;";
const usd = getAddressValueUsd(addr);
const total =
usd === null
? ""
: `<div class="text-xs text-muted mt-1">Total: ${formatUsd(usd)}</div>`;
return (
`<p class="mb-1">This address holds a balance. Removing it does not ` +
`move or spend anything; the balance stays at the address.</p>` +
balanceLinesForAddress(addr, state.trackedTokens, false) +
total
);
}
function show(walletIdx, addrIdx) {
const wallet = state.wallets[walletIdx];
const addr = wallet && wallet.addresses[addrIdx];
if (!addr) return;
target = { walletIdx, addrIdx };
$("delete-address-label").textContent = "Address " + (addrIdx + 1);
$("delete-address-wallet-name").textContent =
wallet.name || "Wallet " + (walletIdx + 1);
const value = $("delete-address-value");
value.innerHTML = renderAddressHtml(addr.address, {
ensName: addr.ensName,
});
attachCopyHandlers(value);
$("delete-address-recovery").textContent = recoveryPathText(wallet);
$("delete-address-balance").innerHTML = balanceWarningHtml(addr);
setFlash("");
showView("delete-address-confirm");
}
function init(_ctx) {
ctx = _ctx;
$("btn-delete-address-back").addEventListener("click", () => {
target = null;
goBack();
});
$("btn-delete-address-confirm").addEventListener("click", async () => {
if (target === null) {
setFlash("No address is selected for removal.");
return;
}
const { walletIdx, addrIdx } = target;
if (!canRemoveAddress(state.wallets[walletIdx])) {
setFlash(
"This address cannot be removed, because a wallet always " +
"keeps at least one address.",
);
return;
}
const { removed, activeAddressChanged } = removeAddressFromState(
state,
walletIdx,
addrIdx,
);
if (!removed) {
setFlash("This address could not be removed.");
return;
}
target = null;
// Save before broadcasting: the background reads the active address
// back out of storage to build accountsChanged.
await saveState();
if (activeAddressChanged) broadcastActiveChanged();
ctx.renderWalletList();
goBack();
showFlash("Address removed.");
});
}
// recoveryPathText and balanceWarningHtml are exported so the two pieces of
// copy that carry the screen's substance can be tested without a DOM; show()
// is a one-line assignment for each.
module.exports = { init, show, recoveryPathText, balanceWarningHtml };

View File

@@ -25,6 +25,7 @@ const VIEWS = [
"add-token",
"settings",
"delete-wallet-confirm",
"delete-address-confirm",
"settings-addtoken",
"transaction",
"approve-site",
@@ -217,6 +218,20 @@ function balanceLinesForAddress(addr, trackedTokens, showZero) {
return html;
}
// Whether an address holds anything at all: ETH or any ERC-20 the wallet
// knows about. Deliberately unrounded — the rendered lines round to four
// decimals, so a dust balance displays as 0.0000 while still being real
// money at a real address. Callers that warn about holdings must ask this,
// not the rendered figure.
function addressHoldsFunds(addr) {
if (!addr) return false;
if (parseFloat(addr.balance || "0") > 0) return true;
for (const t of addr.tokenBalances || []) {
if (parseFloat(t.balance || "0") > 0) return true;
}
return false;
}
// Truncate the middle of a string, replacing removed characters with "…".
// Safety: refuses to truncate more than 10 characters, which is the maximum
// that still prevents address spoofing attacks (see Display Consistency in
@@ -463,6 +478,7 @@ module.exports = {
flashCopyFeedback,
balanceLine,
balanceLinesForAddress,
addressHoldsFunds,
addressColor,
addressDotHtml,
escapeHtml,

View File

@@ -21,6 +21,11 @@ const {
resetSendValidation,
} = require("./send");
const { deriveAddressFromXpub } = require("../../shared/wallet");
const { canRemoveAddress } = require("../../shared/walletDelete");
const {
walletDefect,
walletDefectHtml,
} = require("../../shared/walletDefects");
const {
formatUsd,
getPrice,
@@ -214,30 +219,34 @@ async function loadHomeTxs(ctx) {
}
}
function render(ctx) {
const container = $("wallet-list");
if (state.wallets.length === 0) {
container.innerHTML =
'<p class="text-muted py-2">No wallets yet. Add one to get started.</p>';
renderTotalValue();
renderActiveAddress();
return;
}
// The wallet list markup. Pure: it reads state and returns a string, so the
// list can be asserted on without a DOM.
function walletListHtml() {
let html = "";
state.wallets.forEach((wallet, wi) => {
const defect = walletDefect(wallet);
html += `<div>`;
html += `<div class="flex justify-between items-center bg-section py-1 px-2" style="margin:0 -0.5rem">`;
html += `<span class="font-bold cursor-pointer wallet-name underline decoration-dashed" data-wallet="${wi}">${wallet.name}</span>`;
if (wallet.type === "hd" || wallet.type === "xprv") {
// No "+" on a defective wallet: deriving another address from that
// xpub would only add one more address the key does not produce
// under the standard path.
if (!defect && (wallet.type === "hd" || wallet.type === "xprv")) {
html += `<button class="btn-add-address border border-border px-1 hover:bg-fg hover:text-bg cursor-pointer text-xs" data-wallet="${wi}" title="Add another address to this wallet">+</button>`;
}
html += `</div>`;
html += walletDefectHtml(wallet);
wallet.addresses.forEach((addr, ai) => {
html += `<div class="address-row py-1 border-b border-border-light cursor-pointer hover:bg-hover" data-wallet="${wi}" data-address="${ai}">`;
const isActive = state.activeAddress === addr.address;
const infoBtn = `<span class="btn-addr-info text-xs cursor-pointer border border-border hover:bg-fg hover:text-bg" style="padding:0" data-wallet="${wi}" data-address="${ai}">[info]</span>`;
// Only where a wallet can spare the address: a wallet holding a
// single address has no remove control, because its last address
// is never removable.
const removeBtn = canRemoveAddress(wallet)
? `<span class="btn-remove-address text-xs cursor-pointer border border-border hover:bg-fg hover:text-bg ml-1" style="padding:0" data-wallet="${wi}" data-address="${ai}" title="Remove this address from the wallet">[x]</span>`
: "";
const dot = addressDotHtml(addr.address);
const titleBold = isActive ? "font-bold" : "";
html += `<div class="text-xs ${titleBold}">Address ${ai + 1}</div>`;
@@ -246,7 +255,7 @@ function render(ctx) {
}
html += `<div class="flex text-xs items-center justify-between">`;
html += `<span class="flex items-center break-all">${addr.ensName ? "" : dot}${addr.address}</span>`;
html += `<span class="flex-shrink-0 ml-1">${infoBtn}</span>`;
html += `<span class="flex-shrink-0 ml-1">${infoBtn}${removeBtn}</span>`;
html += `</div>`;
const addrUsd = formatUsd(getAddressValueUsd(addr));
html += `<div class="text-xs text-muted text-right min-h-[1rem]">${addrUsd || "&nbsp;"}</div>`;
@@ -260,7 +269,20 @@ function render(ctx) {
html += `</div>`;
});
container.innerHTML = html;
return html;
}
function render(ctx) {
const container = $("wallet-list");
if (state.wallets.length === 0) {
container.innerHTML =
'<p class="text-muted py-2">No wallets yet. Add one to get started.</p>';
renderTotalValue();
renderActiveAddress();
return;
}
container.innerHTML = walletListHtml();
container.querySelectorAll(".address-row").forEach((row) => {
row.addEventListener("click", async () => {
@@ -289,6 +311,16 @@ function render(ctx) {
});
});
container.querySelectorAll(".btn-remove-address").forEach((btn) => {
btn.addEventListener("click", (e) => {
e.stopPropagation();
ctx.showDeleteAddress(
parseInt(btn.dataset.wallet, 10),
parseInt(btn.dataset.address, 10),
);
});
});
container.querySelectorAll(".btn-add-address").forEach((btn) => {
btn.addEventListener("click", async (e) => {
e.stopPropagation();
@@ -348,6 +380,13 @@ function render(ctx) {
loadHomeTxs(ctx);
}
// The defect of the wallet the selected address belongs to, or null. Call
// after selectActiveAddress().
function selectedWalletDefect() {
if (state.selectedWallet === null) return null;
return walletDefect(state.wallets[state.selectedWallet]);
}
function selectActiveAddress() {
for (let wi = 0; wi < state.wallets.length; wi++) {
for (let ai = 0; ai < state.wallets[wi].addresses.length; ai++) {
@@ -371,6 +410,13 @@ function init(ctx) {
showFlash("No active address selected.");
return;
}
// Before the balance check and before any password is asked for: this
// wallet cannot sign at all, so the send screen is a dead end.
const defect = selectedWalletDefect();
if (defect) {
showFlash(defect.shortMessage);
return;
}
const addr = currentAddress();
if (!addr.balance || parseFloat(addr.balance) === 0) {
showFlash("Cannot send \u2014 zero balance.");
@@ -396,4 +442,4 @@ function init(ctx) {
});
}
module.exports = { init, render };
module.exports = { init, render, walletListHtml };

View File

@@ -120,9 +120,24 @@ function getSignerForAddress(walletData, addrIndex, decryptedSecret) {
return node.deriveChild(addrIndex);
}
if (walletData.type === "xprv") {
const node =
masterXprvOrThrow(decryptedSecret).derivePath(BIP44_ETH_PATH);
return node.deriveChild(addrIndex);
// Checked here rather than through masterXprvOrThrow so the message
// fits the situation: nobody is importing anything at signing time,
// and this wallet is already in storage. src/shared/walletDefects.js
// catches it at list-render time; this is the backstop behind that.
const node = parseExtendedKey(decryptedSecret);
if (!node || !node.privateKey) {
throw new Error(
"This wallet's stored key is not a valid extended private " +
"key, so it cannot sign.",
);
}
if (node.depth !== MASTER_DEPTH) {
throw new Error(
"This wallet was imported from an extended private key that " +
"is not a master key, so it cannot sign.",
);
}
return node.derivePath(BIP44_ETH_PATH).deriveChild(addrIndex);
}
return new Wallet(decryptedSecret);
}
@@ -142,6 +157,7 @@ function walletHasRecoveryPhrase(walletData) {
module.exports = {
generateMnemonic,
parseExtendedKey,
deriveAddressFromXpub,
hdWalletFromMnemonic,
hdWalletFromXprv,

View File

@@ -0,0 +1,86 @@
// Wallets already in stored state whose key cannot be used, and the copy that
// explains them.
//
// Refusing a non-master extended private key at import time does nothing for a
// wallet imported before that refusal existed. Such a wallet is detected here,
// at wallet-list render time, so the user meets the explanation on the list
// screen rather than an exception on the send screen. Nothing here modifies or
// removes a wallet: the record is the user's data.
const { parseExtendedKey } = require("./wallet");
const NON_MASTER_XPRV = "non-master-xprv";
// An "xprv" wallet stores the neutered BIP-44 Ethereum node, four levels below
// the key that was imported: the current import path derives the absolute
// m/44'/60'/0'/0 from a depth-0 key, and the pre-#210 path derived the same
// four levels as a relative path beneath whatever depth it was given. A master
// import therefore stores a depth-4 xpub and a depth-d import stores depth
// d + 4, which makes the stored xpub an exact read on the imported key's
// depth — and it is readable without the password, unlike the key itself.
const BIP44_ETH_XPUB_DEPTH = 4;
const DEFECTS = {
[NON_MASTER_XPRV]: {
id: NON_MASTER_XPRV,
heading: "This wallet's addresses were derived incorrectly.",
paragraphs: [
"This wallet was imported from an extended private key that is " +
"not a master key. An earlier version applied the Ethereum " +
"derivation path beneath that key instead of from a master " +
"key, so the addresses listed here are not the ones that key " +
"produces under the standard path.",
"Signing and sending are disabled for this wallet. The addresses " +
"do descend from the extended private key you imported, so " +
"anything they hold is still reachable by software that " +
"repeats the same non-standard derivation. Check them in a " +
"block explorer before deciding what to do.",
"To see the addresses this key produces under the standard path, " +
"import the master extended private key, or the recovery " +
"phrase it came from, as a new wallet. Nothing here has been " +
"changed or removed, and this wallet stays until you delete " +
"it yourself.",
],
// One sentence for the places that have room for one: the flash on a
// blocked Send, the inline error on the approval screens.
shortMessage:
"This wallet cannot sign, because it was imported from an " +
"extended private key that is not a master key. The wallet list " +
"explains what happened.",
},
};
// The defect record for a wallet, or null if there is nothing wrong with it
// that this module can see. Read-only.
//
// A wallet whose xpub will not parse gets null rather than a defect: there is
// no basis in that case to tell the user their key was not a master key, and a
// wrong explanation is worse than none.
function walletDefect(walletData) {
if (!walletData || walletData.type !== "xprv") return null;
const node = parseExtendedKey(walletData.xpub);
if (!node) return null;
if (node.depth === BIP44_ETH_XPUB_DEPTH) return null;
return DEFECTS[NON_MASTER_XPRV];
}
// The notice block for the wallet list, or "" for a wallet with no defect.
// The copy is fixed text from this module, so it needs no escaping.
function walletDefectHtml(walletData) {
const defect = walletDefect(walletData);
if (!defect) return "";
let html =
'<div class="border border-red-500 border-dashed p-2 my-1 text-xs text-red-500">';
html += `<div class="font-bold mb-1">${defect.heading}</div>`;
for (const p of defect.paragraphs) {
html += `<p class="mb-1">${p}</p>`;
}
html += "</div>";
return html;
}
module.exports = {
NON_MASTER_XPRV,
walletDefect,
walletDefectHtml,
};

View File

@@ -1,5 +1,22 @@
// Wallet deletion state transition, kept out of the view so the selection
// and broadcast rules are testable without a DOM.
// Wallet and address deletion state transitions, kept out of the views so the
// selection and broadcast rules are testable without a DOM.
// Two records of the same address can be stored in different cases, so
// address equality is never a literal string comparison.
function sameAddress(a, b) {
if (a === null || a === undefined || b === null || b === undefined) {
return false;
}
return String(a).toLowerCase() === String(b).toLowerCase();
}
// Forget every site permission held against the given addresses.
function dropSitePermissions(state, addresses) {
for (const addr of addresses) {
delete state.allowedSites[addr];
delete state.deniedSites[addr];
}
}
// Remove wallet `walletIdx` from `state` and repair the derived state.
//
@@ -18,19 +35,13 @@ function removeWalletFromState(state, walletIdx) {
const wallet = state.wallets[walletIdx];
const addresses = (wallet.addresses || []).map((a) => a.address);
const previousActive = state.activeAddress;
const activeWasDeleted =
previousActive !== null &&
previousActive !== undefined &&
addresses.some(
(a) => a.toLowerCase() === String(previousActive).toLowerCase(),
const activeWasDeleted = addresses.some((a) =>
sameAddress(a, previousActive),
);
state.wallets.splice(walletIdx, 1);
for (const addr of addresses) {
delete state.allowedSites[addr];
delete state.deniedSites[addr];
}
dropSitePermissions(state, addresses);
state.hasWallet = state.wallets.length > 0;
@@ -58,6 +69,77 @@ function removeWalletFromState(state, walletIdx) {
return { activeAddressChanged: state.activeAddress !== previousActive };
}
// Whether a wallet may be offered a per-address remove control, and the same
// gate the removal itself is held behind.
//
// Only a wallet that derives its addresses from an extended key can hold more
// than one, so only those get the control — a key wallet has exactly one
// address and no "+" button either. The last address of any wallet is never
// removable: a wallet with no addresses is what delete-wallet is for.
function canRemoveAddress(wallet) {
if (!wallet) return false;
if (wallet.type !== "hd" && wallet.type !== "xprv") return false;
return (wallet.addresses || []).length > 1;
}
// Remove address `addrIdx` of wallet `walletIdx` and repair the derived state.
//
// Nothing is destroyed here. The address stays derivable from the wallet's own
// key material and any funds at it are untouched; this only stops the wallet
// tracking it. `nextIndex` is deliberately left alone — it is a derivation
// high-water mark, so "+" derives a fresh index rather than handing back the
// address just removed, and the gap it leaves is within what
// `scanForAddresses()` re-discovers on a later import.
//
// The rules mirror removeWalletFromState() one level down:
// - The call is refused unless canRemoveAddress() allows it, so the last
// address of a wallet always survives.
// - Site permissions are dropped for the removed address.
// - `selectedAddress` follows the splice, but only within the wallet that
// lost the address: it is decremented when an earlier address was
// removed, and falls back to that wallet's first address when the
// selection itself was removed. `selectedWallet` never moves, because the
// wallet list does not.
// - `activeAddress` moves only when it was the removed address, and then to
// the wallet's first remaining address.
//
// Returns whether the address was removed and whether `activeAddress`
// changed, so the caller can broadcast it.
function removeAddressFromState(state, walletIdx, addrIdx) {
const wallet = state.wallets[walletIdx];
const refused = { removed: false, activeAddressChanged: false };
if (!canRemoveAddress(wallet)) return refused;
if (!wallet.addresses[addrIdx]) return refused;
const address = wallet.addresses[addrIdx].address;
const previousActive = state.activeAddress;
const activeWasRemoved = sameAddress(address, previousActive);
wallet.addresses.splice(addrIdx, 1);
dropSitePermissions(state, [address]);
if (state.selectedWallet === walletIdx) {
if (state.selectedAddress === addrIdx) {
state.selectedAddress = 0;
} else if (
typeof state.selectedAddress === "number" &&
state.selectedAddress > addrIdx
) {
state.selectedAddress -= 1;
}
}
if (activeWasRemoved) {
state.activeAddress = wallet.addresses[0].address;
}
return {
removed: true,
activeAddressChanged: state.activeAddress !== previousActive,
};
}
// Tell the background the active address changed, so it re-emits
// accountsChanged to connected sites. Same call shape as the address
// switch in the home view.
@@ -67,4 +149,9 @@ function broadcastActiveChanged() {
runtime.sendMessage({ type: "AUTISTMASK_ACTIVE_CHANGED" });
}
module.exports = { removeWalletFromState, broadcastActiveChanged };
module.exports = {
canRemoveAddress,
removeAddressFromState,
removeWalletFromState,
broadcastActiveChanged,
};

158
tests/deleteAddress.test.js Normal file
View File

@@ -0,0 +1,158 @@
// Tests for the copy on the address-removal confirmation (issue #162).
//
// The screen's whole job is to warn before a destructive-looking action, so
// the copy is the substance and is tested as such. Two things it must not
// get wrong: what it takes to get the address back — the app refuses both
// obvious routes — and what counts as holding something, which is any
// ERC-20 as well as ETH, at any size, including a balance that rounds to
// zero at the four decimals the balance lines render. The DOM behaviour
// around them is driven against the real popup by tests/e2e/run.js.
// helpers.js pulls in state.js, which reads chrome.storage.local at load.
globalThis.chrome = {
storage: { local: { get: async () => ({}), set: async () => {} } },
};
const { addressHoldsFunds } = require("../src/popup/views/helpers");
const {
recoveryPathText,
balanceWarningHtml,
} = require("../src/popup/views/deleteAddress");
const { prices, clearPrices } = require("../src/shared/prices");
const USDC = "0xa0b86991c6218b36c1d19d4a2e9eb0ce3606eb48";
const EMPTY = { address: "0x1", balance: "0.0000", tokenBalances: [] };
const ETH_ONLY = { address: "0x1", balance: "1.5", tokenBalances: [] };
const DUST = { address: "0x1", balance: "0.00001", tokenBalances: [] };
const TOKEN_ONLY = {
address: "0x1",
balance: "0.0000",
tokenBalances: [{ address: USDC, symbol: "USDC", balance: "2500.0" }],
};
const ZERO_TOKEN = {
address: "0x1",
balance: "0",
tokenBalances: [{ address: USDC, symbol: "USDC", balance: "0" }],
};
afterEach(() => {
clearPrices();
});
describe("what the screen says it takes to get the address back", () => {
// The screen used to promise the address "can be brought back at any
// time by importing this wallet's recovery phrase again". That import is
// refused as a duplicate for as long as the wallet is present, which it
// always is here — a wallet never gives up its last address.
test("it does not promise a re-import while the wallet is here", () => {
const text = recoveryPathText({ type: "hd" });
expect(text).not.toMatch(/at any time/);
expect(text).toContain("is refused while this wallet is still here");
});
test("it names deleting the whole wallet as the route back", () => {
expect(recoveryPathText({ type: "hd" })).toContain(
"delete the whole wallet in Settings",
);
});
// The scan after a re-import finds used addresses only, so an address
// that never saw a transaction does not come back at all. Saying so is
// the difference between a warning and a false reassurance.
test("it states the limit: only on-chain activity is found", () => {
const text = recoveryPathText({ type: "hd" });
expect(text).toContain("only finds addresses that have on-chain");
expect(text).toContain("never been used is not found by it");
});
// The screen is offered on xprv wallets too, and an xprv wallet holds no
// recovery phrase — telling its owner to import one would send them
// looking for words that do not exist.
test("an xprv wallet is told about its extended private key", () => {
const text = recoveryPathText({ type: "xprv" });
expect(text).toContain("extended private key");
expect(text).not.toContain("recovery phrase");
});
test("an HD wallet is told about its recovery phrase", () => {
const text = recoveryPathText({ type: "hd" });
expect(text).toContain("recovery phrase");
expect(text).not.toContain("extended private key");
});
});
describe("whether an address holds anything", () => {
test("ETH counts", () => {
expect(addressHoldsFunds(ETH_ONLY)).toBe(true);
});
// The case that decides the screen: no ETH at all, and $2500 of a
// stablecoin sitting at the address.
test("an ERC-20 balance counts even with no ETH", () => {
expect(addressHoldsFunds(TOKEN_ONLY)).toBe(true);
});
// 0.00001 ETH renders as "0.0000" at four decimals. It is still money.
test("an ETH balance below the displayed precision counts", () => {
expect(addressHoldsFunds(DUST)).toBe(true);
});
test("an address holding nothing does not", () => {
expect(addressHoldsFunds(EMPTY)).toBe(false);
expect(addressHoldsFunds(ZERO_TOKEN)).toBe(false);
});
test("a missing address or missing fields do not", () => {
expect(addressHoldsFunds(undefined)).toBe(false);
expect(addressHoldsFunds({ address: "0x1" })).toBe(false);
});
});
describe("the balance warning on the removal confirmation", () => {
test("an address holding nothing gets a blank line, not a warning", () => {
expect(balanceWarningHtml(EMPTY)).toBe("&nbsp;");
expect(balanceWarningHtml(ZERO_TOKEN)).toBe("&nbsp;");
});
test("an ERC-20-only address is warned about, and its token listed", () => {
const html = balanceWarningHtml(TOKEN_ONLY);
expect(html).toContain("This address holds a balance.");
expect(html).toContain("does not move or spend anything");
expect(html).toContain("USDC");
expect(html).toContain("2500.0000");
});
// The rendered line says 0.0000 for this address — that is the display
// format, shared with Home and AddressDetail — and the warning is shown
// all the same, because the balance is not zero.
test("an ETH balance that renders as 0.0000 is warned about", () => {
const html = balanceWarningHtml(DUST);
expect(html).toContain("This address holds a balance.");
expect(html).toContain("<span>0.0000</span>");
});
// The sentence must not assert an amount, because any amount it could
// assert has been rounded: "This address holds 0.0000 ETH." is what the
// rounded form produces for an address that holds real money.
test("the warning sentence asserts no rounded amount", () => {
for (const addr of [DUST, ETH_ONLY, TOKEN_ONLY]) {
expect(balanceWarningHtml(addr)).not.toMatch(
/holds [\d.]+ (ETH|USDC)/,
);
}
});
test("the USD total is shown when prices are known", () => {
prices.ETH = 2000;
prices.USDC = 1;
expect(balanceWarningHtml(TOKEN_ONLY)).toContain("Total: $2,500.00");
expect(balanceWarningHtml(ETH_ONLY)).toContain("Total: $3,000.00");
});
// getAddressValueUsd() returns null on testnet and before the first
// price fetch. A "Total: $0.00" there would be a lie about the holdings.
test("no USD total is shown when prices are not known", () => {
expect(balanceWarningHtml(TOKEN_ONLY)).not.toContain("Total:");
});
});

View File

@@ -398,6 +398,101 @@ test("reopening the popup never lands on the phrase screen (#161)", async (env)
assertWiped(st, env.phrase, "after reopening the popup");
});
// -------------------------------------------- address removal (#162)
// Number of address rows across every wallet in the list, counted in the DOM
// whether or not Home is the screen on top.
function addressRowCount(page) {
return page.locator("#wallet-list .btn-addr-info").count();
}
function waitForAddressRows(page, n) {
return page.waitForFunction(
(want) =>
document.querySelectorAll("#wallet-list .btn-addr-info").length ===
want,
n,
{ timeout: 60000 },
);
}
// The suite arrives here with two wallets, an HD one and a key one, holding
// one address each.
test("only a wallet that can spare an address offers to remove one (#162)", async (env) => {
await visible(env.page, "#view-main");
const rows = await addressRowCount(env.page);
assert(rows === 2, "expected two address rows, got " + rows);
const offered = await env.page
.locator("#wallet-list .btn-remove-address")
.count();
assert(
offered === 0,
"a wallet holding its last address offered to remove it",
);
await env.page.click("#wallet-list .btn-add-address");
await waitForAddressRows(env.page, 3);
// Only the HD wallet's two rows; the key wallet still holds one address.
const nowOffered = await env.page
.locator("#wallet-list .btn-remove-address")
.count();
assert(
nowOffered === 2,
"expected the HD wallet's two rows to offer removal, got " + nowOffered,
);
});
// The gate itself: the control opens a confirmation, and leaving that
// confirmation by "Back" removes nothing.
test("leaving the removal confirmation removes nothing (#162)", async (env) => {
await env.page.locator("#wallet-list .btn-remove-address").nth(1).click();
await visible(env.page, "#view-delete-address-confirm");
const label = await env.page.locator("#delete-address-label").innerText();
assert(
label === "Address 2",
"the confirmation names the wrong address: " + JSON.stringify(label),
);
// The route back is written by the view, not by index.html, so an empty
// paragraph here means the user is confirming with no idea what it
// takes to undo. This wallet is an HD one, so it is told about its
// recovery phrase.
const recovery = await env.page
.locator("#delete-address-recovery")
.innerText();
assert(
recovery.includes("delete the whole wallet in Settings") &&
recovery.includes("recovery phrase"),
"the confirmation does not state the route back: " +
JSON.stringify(recovery),
);
// "Back" re-renders Home, so a count taken after it is a real
// measurement of the wallet rather than a stale screen.
await env.page.click("#btn-delete-address-back");
await visible(env.page, "#view-main");
const rows = await addressRowCount(env.page);
assert(rows === 3, "the address was removed without a confirmation");
});
test("confirming removes the address and returns Home (#162)", async (env) => {
await env.page.locator("#wallet-list .btn-remove-address").nth(1).click();
await visible(env.page, "#view-delete-address-confirm");
await env.page.click("#btn-delete-address-confirm");
await visible(env.page, "#view-main");
await waitForAddressRows(env.page, 2);
const offered = await env.page
.locator("#wallet-list .btn-remove-address")
.count();
assert(
offered === 0,
"the HD wallet still offers to remove its last address",
);
});
// ---------------------------------------------------------------- runner
async function main() {

290
tests/walletDefects.test.js Normal file
View File

@@ -0,0 +1,290 @@
// Tests for the stored-state half of the non-master extended key problem.
//
// Refusing a non-master xprv at import time does nothing for a wallet that is
// already in storage: the import that created it ran before the refusal
// existed. Such a wallet used to sign for the wrong tree and now throws on the
// send screen instead. These tests pin down that it is named and explained in
// the wallet list, that nothing on the way there throws, and that a wallet
// imported from a real master key is untouched by any of it.
const { HDNodeWallet, Mnemonic } = require("ethers");
const wallet = require("../src/shared/wallet");
const {
walletDefect,
walletDefectHtml,
NON_MASTER_XPRV,
} = require("../src/shared/walletDefects");
// BIP-39 test vector phrase, published; never used for real funds.
const VECTOR_PHRASE =
"test test test test test test test test test test test junk";
function seedNode(phrase) {
return HDNodeWallet.fromSeed(Mnemonic.fromPhrase(phrase, "").computeSeed());
}
// The master (depth-0) key, which is what the import flow accepts today.
function masterXprv(phrase) {
return seedNode(phrase).extendedKey;
}
// The account-level (depth-3) key m/44'/60'/0'. A normal thing for a user to
// hold, and what the import flow used to accept.
function accountXprv(phrase) {
return seedNode(phrase).derivePath("m/44'/60'/0'").extendedKey;
}
// The wallet record the CURRENT import path writes for a master key: the
// neutered m/44'/60'/0'/0 node, four levels below a depth-0 key.
function healthyXprvWallet(name = "Master") {
const { xpub, firstAddress } = wallet.hdWalletFromXprv(
masterXprv(VECTOR_PHRASE),
);
return {
name,
type: "xprv",
xpub,
nextIndex: 1,
encryptedSecret: "irrelevant-to-these-tests",
addresses: [{ address: firstAddress, balance: "0.0000" }],
};
}
// The wallet record the PRE-#210 import path wrote for an account-level key:
// the same four levels, but derived as a relative path *beneath* the key, so
// the stored xpub sits at depth 3 + 4 = 7. Built here the way the old code
// built it rather than by calling the module under test, which now refuses.
function brokenXprvWallet(name = "Imported xprv") {
const node = HDNodeWallet.fromExtendedKey(
accountXprv(VECTOR_PHRASE),
).derivePath("44'/60'/0'/0");
return {
name,
type: "xprv",
xpub: node.neuter().extendedKey,
nextIndex: 1,
encryptedSecret: "irrelevant-to-these-tests",
addresses: [
{ address: node.deriveChild(0).address, balance: "0.0000" },
],
};
}
describe("the fixtures are what the two import paths actually produced", () => {
test("a master import stores a depth-4 xpub", () => {
expect(
HDNodeWallet.fromExtendedKey(healthyXprvWallet().xpub).depth,
).toBe(4);
});
test("the pre-fix account-level import stored a depth-7 xpub", () => {
expect(
HDNodeWallet.fromExtendedKey(brokenXprvWallet().xpub).depth,
).toBe(7);
});
});
describe("walletDefect", () => {
test("names the defect on a stored non-master xprv wallet", () => {
const defect = walletDefect(brokenXprvWallet());
expect(defect).not.toBeNull();
expect(defect.id).toBe(NON_MASTER_XPRV);
});
test("a depth-0 xprv wallet has no defect", () => {
expect(walletDefect(healthyXprvWallet())).toBeNull();
});
test("hd and key wallets are never assessed", () => {
expect(
walletDefect({ type: "hd", xpub: brokenXprvWallet().xpub }),
).toBe(null);
expect(walletDefect({ type: "key" })).toBeNull();
});
test("an xprv wallet whose xpub cannot be parsed makes no claim", () => {
// No basis to say the key was non-master, so nothing is asserted
// about it rather than guessing.
expect(walletDefect({ type: "xprv", xpub: "not-a-key" })).toBeNull();
expect(walletDefect({ type: "xprv" })).toBeNull();
});
test("nothing about the wallet record is modified by the check", () => {
const w = brokenXprvWallet();
const before = JSON.stringify(w);
walletDefect(w);
expect(JSON.stringify(w)).toBe(before);
});
});
describe("the explanatory copy", () => {
const defect = walletDefect(brokenXprvWallet());
test("every sentence of it is a full sentence", () => {
for (const text of [defect.heading, ...defect.paragraphs]) {
expect(text).toMatch(/^[A-Z]/);
expect(text.trimEnd()).toMatch(/\.$/);
}
});
test("it says what was derived wrongly and that these are not the standard addresses", () => {
const body = defect.paragraphs.join(" ");
expect(body).toContain("not a master key");
expect(body).toMatch(/standard path/);
});
test("it does not claim the funds are safe and does not claim a loss", () => {
const all = [defect.heading, ...defect.paragraphs].join(" ");
expect(all).not.toMatch(/\bsafe\b/i);
expect(all).not.toMatch(/\blost\b|\bstolen\b|\bgone\b/i);
});
test("it says the wallet is not deleted and what the user can do", () => {
const body = defect.paragraphs.join(" ");
expect(body).toMatch(/until you delete it yourself/);
expect(body).toMatch(/recovery phrase/);
});
test("it uses the project's vocabulary", () => {
const all = [
defect.heading,
...defect.paragraphs,
defect.shortMessage,
].join(" ");
expect(all).not.toMatch(/seed phrase|mnemonic|passphrase/i);
expect(all).not.toMatch(/\baccounts?\b/i);
});
});
describe("walletDefectHtml", () => {
test("renders the heading and every paragraph for a defective wallet", () => {
const defect = walletDefect(brokenXprvWallet());
const html = walletDefectHtml(brokenXprvWallet());
expect(html).toContain(defect.heading);
for (const p of defect.paragraphs) {
expect(html).toContain(p);
}
});
test("renders nothing at all for a healthy wallet", () => {
expect(walletDefectHtml(healthyXprvWallet())).toBe("");
});
});
describe("the wallet list", () => {
let home;
let state;
beforeAll(() => {
global.chrome = {
storage: { local: { get: async () => ({}), set: async () => {} } },
runtime: { sendMessage: () => {} },
};
home = require("../src/popup/views/home");
state = require("../src/shared/state").state;
});
afterEach(() => {
state.wallets = [];
state.activeAddress = null;
});
test("a stored depth-3 xprv wallet renders the explanation", () => {
state.wallets = [brokenXprvWallet("Imported xprv")];
const html = home.walletListHtml();
expect(html).toContain(walletDefect(state.wallets[0]).heading);
expect(html).toContain("Imported xprv");
});
test("it does not offer to derive further addresses from that wallet", () => {
state.wallets = [brokenXprvWallet()];
expect(home.walletListHtml()).not.toContain("btn-add-address");
});
test("a normal depth-0 xprv wallet renders exactly as it did before", () => {
state.wallets = [healthyXprvWallet("Master")];
const html = home.walletListHtml();
expect(html).not.toContain(walletDefect(brokenXprvWallet()).heading);
expect(html).toContain("btn-add-address");
expect(html).toContain(state.wallets[0].addresses[0].address);
});
test("the defective wallet's notice does not bleed onto a healthy one", () => {
state.wallets = [brokenXprvWallet("Broken"), healthyXprvWallet("Fine")];
const html = home.walletListHtml();
const healthyPart = html.slice(html.indexOf("Fine"));
expect(html).toContain(walletDefect(state.wallets[0]).heading);
expect(healthyPart).not.toContain(
walletDefect(state.wallets[0]).heading,
);
expect(healthyPart).toContain("btn-add-address");
});
});
describe("no path throws an unhandled error for a defective wallet", () => {
test("address derivation from the stored xpub still works", () => {
// The stored xpub is at a non-standard depth but is a valid extended
// key; deriving from it is what the list render already does.
const w = brokenXprvWallet();
expect(() => wallet.deriveAddressFromXpub(w.xpub, 0)).not.toThrow();
expect(wallet.deriveAddressFromXpub(w.xpub, 0)).toBe(
w.addresses[0].address,
);
});
test("the wallet list renders without throwing", () => {
const { state } = require("../src/shared/state");
const home = require("../src/popup/views/home");
state.wallets = [brokenXprvWallet()];
expect(() => home.walletListHtml()).not.toThrow();
state.wallets = [];
});
test("signing refuses with the named defect rather than a bare failure", () => {
// getSignerForAddress is the backstop behind the UI gate. It must
// still refuse, and it must say why in a sentence the user can read.
let thrown = null;
try {
wallet.getSignerForAddress(
{ type: "xprv" },
0,
accountXprv(VECTOR_PHRASE),
);
} catch (e) {
thrown = e;
}
expect(thrown).not.toBeNull();
expect(thrown.message).toMatch(/master key/);
expect(thrown.message.trimEnd()).toMatch(/\.$/);
});
test("a healthy xprv wallet signs as it always did", () => {
const signer = wallet.getSignerForAddress(
{ type: "xprv" },
0,
masterXprv(VECTOR_PHRASE),
);
expect(signer.address).toBe(healthyXprvWallet().addresses[0].address);
});
});

View File

@@ -1,4 +1,6 @@
const {
canRemoveAddress,
removeAddressFromState,
removeWalletFromState,
broadcastActiveChanged,
} = require("../src/shared/walletDelete");
@@ -6,6 +8,7 @@ const {
// Fixed addresses — never used for anything but these tests.
const A0 = "0x66133E8ea0f5D1d612D2502a968757D1048c214a";
const A1 = "0xdAC17F958D2ee523a2206206994597C13D831ec7";
const A2 = "0x514910771AF9Ca656af840dff83E8264EcF986CA";
const B0 = "0x2260FAC5E5542a773Aa44fBCfeDf7C193bc2C599";
const C0 = "0xA0b86991c6218b36c1d19D4a2e9Eb0cE3606eB48";
@@ -111,6 +114,219 @@ describe("removeWalletFromState", () => {
});
});
// An HD wallet with three addresses next to a single-address key wallet.
// `nextIndex` is the wallet's derivation high-water mark, three addresses in.
function makeAddressState(overrides = {}) {
return {
hasWallet: true,
wallets: [
{ ...wallet("A", [A0, A1, A2]), type: "hd", nextIndex: 3 },
{ ...wallet("B", [B0]), type: "key" },
],
selectedWallet: 0,
selectedAddress: 0,
activeAddress: A0,
allowedSites: { [A0]: ["a.example"], [A1]: ["b.example"] },
deniedSites: { [A1]: ["d.example"], [B0]: ["e.example"] },
...overrides,
};
}
describe("canRemoveAddress", () => {
test("an HD wallet with more than one address may remove one", () => {
expect(canRemoveAddress({ type: "hd", addresses: [{}, {}] })).toBe(
true,
);
});
test("an xprv wallet with more than one address may too", () => {
expect(canRemoveAddress({ type: "xprv", addresses: [{}, {}] })).toBe(
true,
);
});
// The last address is what delete-wallet is for.
test("a wallet holding a single address may not", () => {
expect(canRemoveAddress({ type: "hd", addresses: [{}] })).toBe(false);
});
// A key wallet holds one bare private key and cannot derive more, so it
// has no "+" button and gets no remove control either.
test("a key wallet may not, whatever its address count", () => {
expect(canRemoveAddress({ type: "key", addresses: [{}] })).toBe(false);
expect(canRemoveAddress({ type: "key", addresses: [{}, {}] })).toBe(
false,
);
});
test("a missing or typeless wallet may not", () => {
expect(canRemoveAddress(undefined)).toBe(false);
expect(canRemoveAddress({})).toBe(false);
});
});
describe("removeAddressFromState", () => {
test("removing a non-selected address leaves the selection where it is", () => {
const state = makeAddressState({
selectedAddress: 2,
activeAddress: A2,
});
const { removed, activeAddressChanged } = removeAddressFromState(
state,
0,
0,
);
expect(removed).toBe(true);
// A2 moved from index 2 to index 1 by the splice.
expect(state.wallets[0].addresses.map((a) => a.address)).toEqual([
A1,
A2,
]);
expect(state.selectedWallet).toBe(0);
expect(state.selectedAddress).toBe(1);
expect(state.activeAddress).toBe(A2);
expect(activeAddressChanged).toBe(false);
// The wallet list itself is untouched.
expect(state.wallets).toHaveLength(2);
expect(state.hasWallet).toBe(true);
});
test("removing an address after the selection does not shift it", () => {
const state = makeAddressState({
selectedAddress: 0,
activeAddress: A0,
});
const { removed, activeAddressChanged } = removeAddressFromState(
state,
0,
2,
);
expect(removed).toBe(true);
expect(state.selectedAddress).toBe(0);
expect(state.activeAddress).toBe(A0);
expect(activeAddressChanged).toBe(false);
});
test("a selection in another wallet is untouched", () => {
const state = makeAddressState({
selectedWallet: 1,
selectedAddress: 0,
activeAddress: B0,
});
const { removed, activeAddressChanged } = removeAddressFromState(
state,
0,
1,
);
expect(removed).toBe(true);
expect(state.selectedWallet).toBe(1);
expect(state.selectedAddress).toBe(0);
expect(state.activeAddress).toBe(B0);
expect(activeAddressChanged).toBe(false);
});
test("removing the selected address falls back to the wallet's first address", () => {
const state = makeAddressState({
selectedAddress: 1,
activeAddress: A1,
});
const { removed, activeAddressChanged } = removeAddressFromState(
state,
0,
1,
);
expect(removed).toBe(true);
expect(state.wallets[0].addresses.map((a) => a.address)).toEqual([
A0,
A2,
]);
expect(state.selectedWallet).toBe(0);
expect(state.selectedAddress).toBe(0);
expect(state.activeAddress).toBe(A0);
expect(activeAddressChanged).toBe(true);
});
// The active address can be persisted in a different case than the
// wallet's copy of it, so the comparison must not be literal.
test("the active address is matched case-insensitively", () => {
const state = makeAddressState({
selectedAddress: 1,
activeAddress: A1.toLowerCase(),
});
const { activeAddressChanged } = removeAddressFromState(state, 0, 1);
expect(state.activeAddress).toBe(A0);
expect(activeAddressChanged).toBe(true);
});
test("site permissions are dropped for the removed address only", () => {
const state = makeAddressState();
removeAddressFromState(state, 0, 1);
expect(state.allowedSites).toEqual({ [A0]: ["a.example"] });
expect(state.deniedSites).toEqual({ [B0]: ["e.example"] });
});
// The derivation counter is a high-water mark, never rewound: "+" derives
// a fresh index rather than re-deriving the address just removed.
test("the wallet's derivation counter is not rewound", () => {
const state = makeAddressState();
removeAddressFromState(state, 0, 1);
expect(state.wallets[0].nextIndex).toBe(3);
});
test("the last address of a wallet is refused, and nothing changes", () => {
const state = makeAddressState({
selectedWallet: 1,
selectedAddress: 0,
activeAddress: B0,
});
const { removed, activeAddressChanged } = removeAddressFromState(
state,
1,
0,
);
expect(removed).toBe(false);
expect(activeAddressChanged).toBe(false);
expect(state.wallets[1].addresses.map((a) => a.address)).toEqual([B0]);
expect(state.activeAddress).toBe(B0);
expect(state.hasWallet).toBe(true);
});
// The same refusal reached the other way: an HD wallet worn down to one
// address is no more removable than a key wallet.
test("an HD wallet down to its last address is refused too", () => {
const state = makeAddressState();
expect(removeAddressFromState(state, 0, 2).removed).toBe(true);
expect(removeAddressFromState(state, 0, 1).removed).toBe(true);
expect(removeAddressFromState(state, 0, 0).removed).toBe(false);
expect(state.wallets[0].addresses.map((a) => a.address)).toEqual([A0]);
});
test("an out-of-range address index is refused", () => {
const state = makeAddressState();
expect(removeAddressFromState(state, 0, 7).removed).toBe(false);
expect(removeAddressFromState(state, 7, 0).removed).toBe(false);
expect(state.wallets[0].addresses).toHaveLength(3);
});
});
describe("broadcastActiveChanged", () => {
afterEach(() => {
delete global.chrome;