test: cover wallet.js key derivation and vault.js encryption — currently zero tests on the crypto core #159

New Issue

clawbot · 2026-08-09T03:44:54+02:00

clawbot commented

2026-08-09 03:44:54 +02:00

Problem

The two modules that hold user funds have no tests at all. The whole suite
is four files (tests/constants.test.js, tests/etherscanLabels.test.js,
tests/phishingDomains.test.js, tests/uniswap.test.js) and none of them
require wallet.js or vault.js.

src/shared/wallet.js exports (wallet.js:73-82): generateMnemonic,
deriveAddressFromXpub, hdWalletFromMnemonic, hdWalletFromXprv,
isValidXprv, addressFromPrivateKey, getSignerForAddress,
isValidMnemonic.

src/shared/vault.js (62 lines) implements the Argon2id + XSalsa20-Poly1305
encrypt/decrypt described in the README Key Decisions section.

This covers two of the three README TODO testing items (README.md:888-889):
"Tests for mnemonic generation and address derivation" and "Tests for xpub
derivation and child address generation".

Blocking caveat

wallet.js:7-13 short-circuits generateMnemonic() to the hardcoded
DEBUG_MNEMONIC whenever DEBUG is true, and src/shared/constants.js:1 is
const DEBUG = true. A naive test written today would assert on the fixed
debug phrase, pass, and leave the real entropy path untested. Issue #149
makes DEBUG a build-time flag defaulting off; coordinate with it. If #149 has
not landed, the tests here must explicitly exercise the non-debug path rather
than accepting whatever DEBUG currently yields.

Implementation requirements

Use fixed, well-known BIP-39 test vectors so assertions are exact, not
self-referential. Use a published throwaway phrase; never a real one.
hdWalletFromMnemonic must be pinned against known addresses for the
documented path m/44'/60'/0'/0/n for at least indices 0, 1, and 2. The
README states compatibility with the standard derivation path, so these
vectors also serve as the compatibility guarantee.
Note and pin an existing asymmetry: hdWalletFromMnemonic:21 uses
BIP44_ETH_PATH = "m/44'/60'/0'/0" while hdWalletFromXprv:32 uses
root.derivePath("44'/60'/0'/0") — relative, without the m/ prefix. A test
must lock down what each actually produces. If they disagree in a way that
yields different addresses for the same key material, that is a bug: report
it in the PR rather than encoding the wrong behaviour into an assertion.
deriveAddressFromXpub must be tested for correct child address generation
and must be shown to agree with hdWalletFromMnemonic for the same
underlying seed and index.
isValidMnemonic / isValidXprv: cover valid input, wrong-checksum input,
wrong word count, and empty/garbage input.
addressFromPrivateKey: pin against a known key/address pair.
vault.js: round-trip encrypt then decrypt returns the original plaintext;
a wrong password fails and fails cleanly (no partial plaintext, no
unhandled rejection); a tampered ciphertext is rejected by the Poly1305 auth
tag; the stored blob has the documented { salt, nonce, ciphertext } shape;
two encryptions of the same plaintext with the same password produce
different ciphertexts (fresh salt and nonce).
Keep the suite inside the 30-second budget enforced by script/test:10.
Argon2id is deliberately slow — if the default parameters make the round-trip
test too slow, do not weaken the production parameters. Use fewer
iterations of the test, or expose the cost parameters for the test only, and
say which in the PR.
Do not test by reaching into module internals; test the exported surface.

Definition of done

tests/wallet.test.js and tests/vault.test.js exist and cover every
bullet above.
Every exported function of wallet.js and vault.js has at least one
assertion against it.
Derivation is pinned to explicit known-answer vectors, not to whatever
the implementation currently returns.
The m/-prefix asymmetry between hdWalletFromMnemonic and
hdWalletFromXprv is either proven harmless by test, or reported as a
bug in the PR.
make test still completes within the 30-second timeout.
The README TODO checkboxes at README.md:888-889 are ticked.
TODO.md updated in the same commit.
make check passes.

## Problem The two modules that hold user funds have **no tests at all**. The whole suite is four files (`tests/constants.test.js`, `tests/etherscanLabels.test.js`, `tests/phishingDomains.test.js`, `tests/uniswap.test.js`) and none of them require `wallet.js` or `vault.js`. `src/shared/wallet.js` exports (`wallet.js:73-82`): `generateMnemonic`, `deriveAddressFromXpub`, `hdWalletFromMnemonic`, `hdWalletFromXprv`, `isValidXprv`, `addressFromPrivateKey`, `getSignerForAddress`, `isValidMnemonic`. `src/shared/vault.js` (62 lines) implements the Argon2id + XSalsa20-Poly1305 encrypt/decrypt described in the README Key Decisions section. This covers two of the three README TODO testing items (`README.md:888-889`): "Tests for mnemonic generation and address derivation" and "Tests for xpub derivation and child address generation". ## Blocking caveat `wallet.js:7-13` short-circuits `generateMnemonic()` to the hardcoded `DEBUG_MNEMONIC` whenever `DEBUG` is true, and `src/shared/constants.js:1` is `const DEBUG = true`. **A naive test written today would assert on the fixed debug phrase, pass, and leave the real entropy path untested.** Issue #149 makes `DEBUG` a build-time flag defaulting off; coordinate with it. If #149 has not landed, the tests here must explicitly exercise the non-debug path rather than accepting whatever `DEBUG` currently yields. ## Implementation requirements - Use fixed, well-known BIP-39 test vectors so assertions are exact, not self-referential. Use a published throwaway phrase; never a real one. - `hdWalletFromMnemonic` must be pinned against known addresses for the documented path `m/44'/60'/0'/0/n` for at least indices 0, 1, and 2. The README states compatibility with the standard derivation path, so these vectors also serve as the compatibility guarantee. - Note and pin an existing asymmetry: `hdWalletFromMnemonic:21` uses `BIP44_ETH_PATH = "m/44'/60'/0'/0"` while `hdWalletFromXprv:32` uses `root.derivePath("44'/60'/0'/0")` — relative, without the `m/` prefix. A test must lock down what each actually produces. If they disagree in a way that yields different addresses for the same key material, that is a bug: report it in the PR rather than encoding the wrong behaviour into an assertion. - `deriveAddressFromXpub` must be tested for correct child address generation and must be shown to agree with `hdWalletFromMnemonic` for the same underlying seed and index. - `isValidMnemonic` / `isValidXprv`: cover valid input, wrong-checksum input, wrong word count, and empty/garbage input. - `addressFromPrivateKey`: pin against a known key/address pair. - `vault.js`: round-trip encrypt then decrypt returns the original plaintext; a wrong password fails and fails *cleanly* (no partial plaintext, no unhandled rejection); a tampered ciphertext is rejected by the Poly1305 auth tag; the stored blob has the documented `{ salt, nonce, ciphertext }` shape; two encryptions of the same plaintext with the same password produce different ciphertexts (fresh salt and nonce). - Keep the suite inside the 30-second budget enforced by `script/test:10`. Argon2id is deliberately slow — if the default parameters make the round-trip test too slow, do **not** weaken the production parameters. Use fewer iterations of the test, or expose the cost parameters for the test only, and say which in the PR. - Do not test by reaching into module internals; test the exported surface. ## Definition of done - [ ] `tests/wallet.test.js` and `tests/vault.test.js` exist and cover every bullet above. - [ ] Every exported function of `wallet.js` and `vault.js` has at least one assertion against it. - [ ] Derivation is pinned to explicit known-answer vectors, not to whatever the implementation currently returns. - [ ] The `m/`-prefix asymmetry between `hdWalletFromMnemonic` and `hdWalletFromXprv` is either proven harmless by test, or reported as a bug in the PR. - [ ] `make test` still completes within the 30-second timeout. - [ ] The README TODO checkboxes at `README.md:888-889` are ticked. - [ ] `TODO.md` updated in the same commit. - [ ] `make check` passes.

clawbot added this to the 1.0.0 milestone 2026-08-09 03:44:54 +02:00

clawbot commented

2026-08-09 06:57:11 +02:00

Manager note — sequencing constraint, recorded before this is dispatched so it
is not discovered the hard way.

tests/wallet.test.js already exists on PR #169's branch. That PR (issue
#149, the DEBUG build-time flag) created it with six tests covering
generateMnemonic in both build modes, including the debug path and the
runtime-toggle interaction.

So this issue is now extend, not create. Whoever picks it up must:

Base the work on fix/issue-149-debug-build-flag if #169 has not merged, or
on main after it has. Do not branch from a main that lacks that file and
then create a second tests/wallet.test.js — that is a guaranteed conflict
on a file where a bad merge resolution could silently drop the regression
tests protecting a funds-loss vulnerability.
Read the existing six tests first and extend around them. Do not rewrite or
reorganise them; they are the guard for #149 and were specifically validated
during review against five different broken-implementation shapes.
The "Blocking caveat" section in the issue body above is now largely
resolved: #169 makes DEBUG default to false under jest, so
generateMnemonic() in a test context exercises the real entropy path.
That removes the trap this issue warned about. The remaining scope is the
derivation and vault coverage — hdWalletFromMnemonic,
hdWalletFromXprv, deriveAddressFromXpub, isValidMnemonic,
isValidXprv, addressFromPrivateKey, and all of vault.js.
The m/ prefix asymmetry between hdWalletFromMnemonic:21 and
hdWalletFromXprv:32 remains unexamined and is still the most interesting
thing in this issue. Pin it with known-answer vectors; if the two disagree
for the same key material, that is a bug to report rather than an assertion
to write.

Holding dispatch until #169 merges or until it is clear the branch base is
stable, to avoid a three-deep stack of unmerged PRs on the same file.

Manager note — sequencing constraint, recorded before this is dispatched so it is not discovered the hard way. **`tests/wallet.test.js` already exists on PR #169's branch.** That PR (issue #149, the DEBUG build-time flag) created it with six tests covering `generateMnemonic` in both build modes, including the debug path and the runtime-toggle interaction. So this issue is now **extend, not create**. Whoever picks it up must: - Base the work on `fix/issue-149-debug-build-flag` if #169 has not merged, or on `main` after it has. Do not branch from a `main` that lacks that file and then create a second `tests/wallet.test.js` — that is a guaranteed conflict on a file where a bad merge resolution could silently drop the regression tests protecting a funds-loss vulnerability. - Read the existing six tests first and extend around them. Do not rewrite or reorganise them; they are the guard for #149 and were specifically validated during review against five different broken-implementation shapes. - The "Blocking caveat" section in the issue body above is now largely resolved: #169 makes `DEBUG` default to `false` under jest, so `generateMnemonic()` in a test context exercises the real entropy path. That removes the trap this issue warned about. The remaining scope is the derivation and vault coverage — `hdWalletFromMnemonic`, `hdWalletFromXprv`, `deriveAddressFromXpub`, `isValidMnemonic`, `isValidXprv`, `addressFromPrivateKey`, and all of `vault.js`. - The `m/` prefix asymmetry between `hdWalletFromMnemonic:21` and `hdWalletFromXprv:32` remains unexamined and is still the most interesting thing in this issue. Pin it with known-answer vectors; if the two disagree for the same key material, that is a bug to report rather than an assertion to write. Holding dispatch until #169 merges or until it is clear the branch base is stable, to avoid a three-deep stack of unmerged PRs on the same file.

clawbot referenced this issue from a commit

2026-08-11 14:28:50 +02:00

test: known-answer coverage for HD derivation and the vault (closes #159)

clawbot referenced a pull request that will close this issue

2026-08-11 14:29:44 +02:00

test: known-answer coverage for HD derivation and the vault (closes #159) #209

clawbot referenced this issue

2026-08-11 14:32:45 +02:00

fix: isValidXprv accepts a mistyped extended key and silently imports a different wallet #210

clawbot referenced this issue from a commit

2026-08-11 14:49:07 +02:00

test: known-answer coverage for HD derivation and the vault (closes #159)