Skip to content

test: cover security-critical gaps from post-merge coverage analysis#848

Open
jtsang586 wants to merge 9 commits intomainfrom
test/resolver-and-dapp-connector-error-paths
Open

test: cover security-critical gaps from post-merge coverage analysis#848
jtsang586 wants to merge 9 commits intomainfrom
test/resolver-and-dapp-connector-error-paths

Conversation

@jtsang586
Copy link
Copy Markdown
Contributor

Summary

  • Adds 56 net new tests across 5 packages, targeting gaps identified in a post-merge coverage analysis of features merged between Feb–Apr 2026
  • Focuses on security-critical regressions (per-recipient encryption keys, shell injection), architectural integrity (protocol ACL subpath wiring + ESLint rule enforcement), and error-path coverage (dapp-connector wallet failures, encryption cache invalidation)
  • Adds first-ever test suites to two previously untested packages (protocol, compact)
  • Each test was critically evaluated for value — 12 low-value tests (tautologies, stdlib assertions, fake-interleaving) were written and then pruned in the final commit

Packages touched

Package Tests added What they guard
contracts 5 #745 resolver: burn-point guard, DApp-mapping precedence, spy proving per-recipient key selection
dapp-connector-proof-provider 5 Wallet API error propagation, ZKConfigProvider failure, transient-failure retry semantics
protocol 45 Subpath/namespace parity, representative symbol resolution, ESLint import-restriction rule (via real ESLint.lintText)
level-private-state-provider 1 invalidateEncryptionCache actually forces StorageEncryption.create re-derivation
compact 4 Static-source regression: exec/execSync with template literals cannot be re-introduced (#711)

Key findings during investigation

  • LevelDB exclusive file lock prevents concurrent set/get from a single provider instance — the "web crypto concurrency" gap was overstated; the real interleaving surface is limited to in-memory cache operations
  • Protocol ACL package had zero test coverage despite being a new architectural layer — now covered with structural + ESLint integration tests

Test plan

  • vitest run passes in all 5 touched packages (contracts: 40, dapp-connector: 12, protocol: 45, level-private-state: 153, compact: 4)
  • tsc --noEmit clean in all packages
  • eslint clean in all packages
  • No pre-existing tests were modified or removed (verified via baseline count comparison against main)

🤖 Generated with Claude Code

@jtsang586 jtsang586 requested a review from a team as a code owner April 16, 2026 09:53
@github-actions
Copy link
Copy Markdown
Contributor

github-actions bot commented Apr 16, 2026

Unit Test Results - MidnightJS

   28 files  +  4     86 suites  +6   4m 27s ⏱️ -5s
  610 tests + 59    608 ✅ + 59  2 💤 ±0  0 ❌ ±0 
1 234 runs  +118  1 230 ✅ +118  4 💤 ±0  0 ❌ ±0 

Results for commit 5113b48. ± Comparison against base commit 5ee9d70.

♻️ This comment has been updated with latest results.

@jtsang586 jtsang586 force-pushed the test/resolver-and-dapp-connector-error-paths branch from faa2a66 to c68b657 Compare April 16, 2026 09:56
@github-actions
Copy link
Copy Markdown
Contributor

github-actions bot commented Apr 16, 2026

Title Lines Statements Branches Functions
contracts Coverage: 92%
91.82% (472/514) 84.05% (195/232) 94.39% (101/107)
dapp-connector-proof-provider Coverage: 92%
91.82% (472/514) 84.05% (195/232) 94.39% (101/107)
fetch-zk-config-provider Coverage: 100%
100% (23/23) 84.61% (11/13) 100% (6/6)
http-client-proof-provider Coverage: 81%
81.39% (35/43) 93.33% (14/15) 66.66% (6/9)
indexer-public-data-provider Coverage: 44%
44.76% (107/239) 39.28% (44/112) 27.52% (30/109)
level-private-state-provider Coverage: 92%
92.77% (642/692) 82.15% (267/325) 100% (89/89)
logger-provider Coverage: 100%
100% (15/15) 100% (0/0) 100% (8/8)
midnight-js Coverage: 100%
100% (0/0) 100% (0/0) 100% (0/0)
node-zk-config-provider Coverage: 100%
100% (11/11) 100% (0/0) 100% (5/5)
utils Coverage: 89%
86.79% (46/53) 80.48% (33/41) 63.63% (7/11)

@MidnightCI
Copy link
Copy Markdown
Contributor

MidnightCI commented Apr 16, 2026

E2E Tests Results

e2e_tests_reports: Run #3556

Tests 📝 Passed ✅ Failed ❌ Skipped ⏭️ Pending ⏳ Other ❓ Flaky 🍂 Duration ⏱️
123 119 0 4 0 0 0 32m 7s

🎉 All tests passed!

Suites

119 passed, 0 failed, and 4 other

Suite Passed Failed Other Duration
✅ testkit-js/testkit-js-e2e/test/contracts.blocktime.it.test.ts
        ✅ Block Time Contract Tests 1 > blockTimeLt tests > should succeed when both device time and node time are less than future time
        ✅ Block Time Contract Tests 1 > blockTimeLt tests > should fail immediately on device when device time is already past the check time
        ⏭️ Block Time Contract Tests 1 > blockTimeLt tests > should succeed on device but fail on node when submission is delayed
        ✅ Block Time Contract Tests 1 > blockTimeLt tests > should succeed when both device time and node time are greater than past time
        ✅ Block Time Contract Tests 1 > blockTimeLt tests > should fail immediately on device when device time is less than check time
        ✅ Block Time Contract Tests 1 > blockTimeLt tests > should succeed even with submission delay when checking past time
        ✅ Block Time Contract Tests 1 > blockTimeLt tests > should succeed when both device time and node time are greater than past time
        ✅ Block Time Contract Tests 1 > blockTimeLt tests > should fail when device time is not greater than check time
        ✅ Block Time Contract Tests 1 > blockTimeLt tests > should succeed when both device time and node time are less than or equal to future time
        ✅ Block Time Contract Tests 1 > blockTimeLt tests > should fail immediately on device when device time exceeds check time
        ⏭️ Block Time Contract Tests 1 > blockTimeLt tests > should succeed on device but fail on node when submission delay causes time to exceed threshold
        ✅ Block Time Contract Tests 1 > blockTimeLt tests > should demonstrate different failure points for Lt check > Immediate past time - fails on device
        ⏭️ Block Time Contract Tests 1 > blockTimeLt tests > should demonstrate different failure points for Lt check > Near future time with delay - succeeds on device, fails on node
        ✅ Block Time Contract Tests 1 > blockTimeLt tests > should demonstrate different failure points for Lt check > Far future time - succeeds on both device and node
        ✅ Block Time Contract Tests 1 > blockTimeLt tests > should handle maximum time values
        ✅ Block Time Contract Tests 1 > blockTimeLt tests > should handle zero time value
✅ 13 ❌ 0 ⏭️ 3 2m 47s
✅ testkit-js/testkit-js-e2e/test/contracts.blocktime2.it.test.ts
        ✅ Block Time Contract Tests 2 > blockTimeLt tests > should demonstrate different failure points for Lt check > Immediate past time - fails on device
        ⏭️ Block Time Contract Tests 2 > blockTimeLt tests > should demonstrate different failure points for Lt check > Near future time with delay - succeeds on device, fails on node
        ✅ Block Time Contract Tests 2 > blockTimeLt tests > should demonstrate different failure points for Lt check > Far future time - succeeds on both device and node
        ✅ Block Time Contract Tests 2 > blockTimeLt tests > should handle maximum time values
        ✅ Block Time Contract Tests 2 > blockTimeLt tests > should handle zero time value
✅ 4 ❌ 0 ⏭️ 1 1m 11s
✅ testkit-js/testkit-js-e2e/test/contracts.it.test.ts
        ✅ Contracts API > should create unproven call and deploy transactions for contract with private state
        ✅ Contracts API > should deploy contract on the chain [@slow]
        ✅ Contracts API > should return deployed contract if it exists on specific address
        ✅ Contracts API > should return deployed contract if it exists on specific address without initialPrivateState
        ✅ Contracts API > should throw error if contract address has wrong format - length
        ✅ Contracts API > should return deployed contract if it exists on specific address with initialPrivateState and empty local private state store
        ✅ Contracts API > should return deployed contract if it exists on specific address with different initialPrivateState
        ✅ Contracts API > should wait indefinitely until contract exists on specific address [@slow]
        ✅ Contracts API > should throw for incompatible contract types that differ by circuit ids
        ✅ Contracts API > should throw for incompatible contract types with same shape but different verifier keys
        ✅ Contracts API > should return contract interface and execute circuit operations [@slow]
        ✅ Contracts API > should throw error on undefined public state at wrong address
        ✅ Contracts API > should submit a deploy transaction [@slow]
        ✅ Contracts API > should submit transaction that calls circuit in contract [@slow]
        ✅ Contracts API > should throw error if private state is undefined
        ✅ Contracts API > should throw error if public state is undefined
        ✅ Contracts API > should throw error if contract address has wrong format - not hex
        ✅ Contracts API > should return the latest observed state of a deployed contract and is independent of the chain state
        ✅ Contracts API > should wait indefinitely until state change, if stopped returns last contract state [@slow]
✅ 19 ❌ 0 ⏭️ 0 3m 23s
✅ testkit-js/testkit-js-e2e/test/contracts.scopedtx.it.test.ts
        ✅ Scoped Transaction Contract Tests > should submit scoped transaction that calls circuit in contract [@slow]
        ✅ Scoped Transaction Contract Tests > should submit scoped transaction that calls 2 different circuits in contract [@slow]
        ✅ Scoped Transaction Contract Tests > should submit scoped transaction that calls 2 circuits in contract and DOES NOT preserve execution order [@slow]
        ✅ Scoped Transaction Contract Tests > should not submit scoped transaction when one circuit call fails [@slow]
✅ 4 ❌ 0 ⏭️ 0 53.2s
✅ testkit-js/testkit-js-e2e/test/contracts.singlecontract.nostate.it.test.ts
        ✅ Contracts API > should deploy and find contracts with no private state [@slow]
        ✅ Contracts API > should create unproven call and deploy transactions for contract with no private state
        ✅ Contracts API > should submit deploy and call transactions for contracts with no private state [@slow]
✅ 3 ❌ 0 ⏭️ 0 1m 34s
✅ testkit-js/testkit-js-e2e/test/contracts.snarkupgrade.it.test.ts
        ✅ Contracts API Snark Upgrade [dedicated contract] [@slow] > should successfully remove verifier key using submitRemoveVerifierKeyTx
        ✅ Contracts API Snark Upgrade [dedicated contract] [@slow] > should successfully remove verifier key using createContractMaintenanceTxInterface
        ✅ Contracts API Snark Upgrade [dedicated contract] [@slow] > should successfully remove verifier key and disable circuit operation
        ✅ Contracts API Snark Upgrade [dedicated contract] [@slow] > should succeed on verifier key insertion retry after removal
        ✅ Contracts API Snark Upgrade [dedicated contract] [@slow] > should fail when inserting verifier key for wrong circuit after removal
✅ 5 ❌ 0 ⏭️ 0 3m 19s
✅ testkit-js/testkit-js-e2e/test/contracts.snarkupgrade.singlecontract.it.test.ts
        ✅ Contracts API Snark Upgrade [single contract] > submitReplaceAuthorityTx - successful replace authority with new key[@slow]
        ✅ Contracts API Snark Upgrade [single contract] > submitReplaceAuthorityTx - successful replace authority with same key [@slow]
        ✅ Contracts API Snark Upgrade [single contract] > submitReplaceAuthorityTx - should fail on replace contract that is not deployed to contract address
        ✅ Contracts API Snark Upgrade [single contract] > submitReplaceAuthorityTx - should fail when signing key for contract address does not exist
        ✅ Contracts API Snark Upgrade [single contract] > submitInsertVerifierKeyTx - should fail on invalid verifier key
        ✅ Contracts API Snark Upgrade [single contract] > submitInsertVerifierKeyTx - successful insert on not present circuitId [@slow]
        ✅ Contracts API Snark Upgrade [single contract] > submitInsertVerifierKeyTx - should fail on contract not present on contract address
        ✅ Contracts API Snark Upgrade [single contract] > submitInsertVerifierKeyTx - should fail on providers for different contract with different API
        ✅ Contracts API Snark Upgrade [single contract] > submitRemoveVerifierKeyTx - should fail on not present circuitId
        ✅ Contracts API Snark Upgrade [single contract] > submitRemoveVerifierKeyTx - should fail on contract not present on contract address
        ✅ Contracts API Snark Upgrade [single contract] > submitRemoveVerifierKeyTx - should fail on providers for different contract with different API
        ✅ Contracts API Snark Upgrade [single contract] > createContractMaintenanceTxInterface - replaceAuthority - successful replace authority with the new one [@slow]
        ✅ Contracts API Snark Upgrade [single contract] > createContractMaintenanceTxInterface - replaceAuthority - successful replace authority with the same one [@slow]
        ✅ Contracts API Snark Upgrade [single contract] > createContractMaintenanceTxInterface - replaceAuthority - should fail on contract not present on contract address
        ✅ Contracts API Snark Upgrade [single contract] > createContractMaintenanceTxInterface - insertVerifierKey - fail when key is still present
        ✅ Contracts API Snark Upgrade [single contract] > createContractMaintenanceTxInterface - insertVerifierKey - success when no key present [@slow]
        ✅ Contracts API Snark Upgrade [single contract] > createCircuitMaintenanceTxInterfaces - insertVerifierKey - fail when key is already present
        ✅ Contracts API Snark Upgrade [single contract] > createCircuitMaintenanceTxInterfaces - insertVerifierKey - success when no key present [@slow]
        ✅ Contracts API Snark Upgrade [single contract] > createCircuitMaintenanceTxInterfaces - removeVerifierKey - should fail on contract not present on contract address
✅ 19 ❌ 0 ⏭️ 0 3m 35s
✅ testkit-js/testkit-js-e2e/test/contracts.snarkupgrade.smoke.it.test.ts
        ✅ Contracts API Snark Upgrade [@slow][@smoke] > should update verifier keys from one contract to another [@smoke]
        ✅ Contracts API Snark Upgrade [@slow][@smoke] > should fail on operate with previous authority after replacement
✅ 2 ❌ 0 ⏭️ 0 3m 38s
✅ testkit-js/testkit-js-e2e/test/indexer-public-data-provider.observable1.it.test.ts
        ✅ Indexer API > should return the history of states starting from defined blockHash (inclusive:true, expected:1,2) [@slow]
        ✅ Indexer API > should return the history of states starting from defined blockHash (inclusive:false, expected:2) [@slow]
        ✅ Indexer API > should return the history of states starting from defined txId (inclusive:true, expected states:1,2) [@slow]
        ✅ Indexer API > should return the history of states starting from defined txId (inclusive:false, expected states:2) [@slow]
✅ 4 ❌ 0 ⏭️ 0 3m 34s
✅ testkit-js/testkit-js-e2e/test/indexer-public-data-provider.observable2.it.test.ts
        ✅ Indexer API > should return the history of states starting from defined blockHeight (inclusive:true, expected states:1,2) [@slow]
        ✅ Indexer API > should return the history of states starting from defined blockHeight (inclusive:false, expected states:2) [@slow]
        ✅ Indexer API > should return the entire history of states of the contract with the given address (config:{ type: 'all' }, expected states:0,1,2) [@slow]
        ✅ Indexer API > should return the history of states of the contract with the given address, starting with the most recent state (config:{ type: 'latest' }, expected states:1,2) [@slow]
✅ 4 ❌ 0 ⏭️ 0 3m 35s
✅ testkit-js/testkit-js-e2e/test/indexer-public-data-provider.singlecontract.it.test.ts
        ✅ Indexer API > queryDeployContractState - should return a contract state equivalent to the initial contract state produced during deployment construction
        ✅ Indexer API > queryContractState - should return the current contract state of a deployed contract
        ✅ Indexer API > queryContractState - should return the current contract state of a deployed contract at defined block height
        ✅ Indexer API > queryContractState - should return the current contract state of a deployed contract at defined block hash
        ✅ Indexer API > queryContractState - should return null on no contract at contract address
        ✅ Indexer API > queryZSwapAndContractState - should return the current ZSwap chain state and contract state of a deployed contract
        ✅ Indexer API > queryZSwapAndContractState - should return null on no contract at contract address
        ✅ Indexer API > watchForDeployTxData - should return the data of the transaction containing the deployment of the contract with the given address
        ✅ Indexer API > watchForTxData - should return the data of the transaction containing the contract call with the given transaction id
        ✅ Indexer API > watchForContractState - should immediately return the current state of a deployed contract
✅ 10 ❌ 0 ⏭️ 0 98ms
✅ testkit-js/testkit-js-e2e/test/level-private-state-provider.it.test.ts
        ✅ Level Private State Provider - Export/Import Integration > should preserve private state after database recreation [@slow]
✅ 1 ❌ 0 ⏭️ 0 58.2s
✅ testkit-js/testkit-js-e2e/test/nodejs.it.test.ts
        ✅ Ledger API - NodeJS Integration Tests > should run ESM module successfully
        ✅ Ledger API - NodeJS Integration Tests > should run CJS module with expected exit code
✅ 2 ❌ 0 ⏭️ 0 1m 38s
✅ testkit-js/testkit-js-e2e/test/proof-server.it.test.ts
        ✅ Proof server integration > should create proofs successfully for deploy and call transactions
        ✅ Proof server integration > should create proofs with transactions that has succesfull well-formedness
        ✅ Proof server integration > should execute 5 proveTx calls in parallel without errors
✅ 3 ❌ 0 ⏭️ 0 676ms
✅ testkit-js/testkit-js-e2e/test/shielded.advanced.it.test.ts
        ✅ Shielded tokens - advanced operations > should mint and send immediate shielded tokens
        ✅ Shielded tokens - advanced operations > should mint and burn shielded tokens
✅ 2 ❌ 0 ⏭️ 0 59.9s
✅ testkit-js/testkit-js-e2e/test/shielded.transfer.it.test.ts
        ✅ Shielded tokens > should mint tokens
        ✅ Shielded tokens > should deposit shielded coin via receiveShielded (issue #686)
✅ 2 ❌ 0 ⏭️ 0 1m 17s
✅ testkit-js/testkit-js-e2e/test/unshielded.balance.it.test.ts
        ✅ Unshielded tokens - balance > should get balance of tokens - 0 value
        ✅ Unshielded tokens - balance > should get balance of tokens - minted amount
        ✅ Unshielded tokens - balance > should get balance of tokens - greater than - false
        ✅ Unshielded tokens - balance > should get balance of tokens - greater than - true
        ✅ Unshielded tokens - balance > should get balance of tokens - less than - false
        ✅ Unshielded tokens - balance > should get balance of tokens - less than - true
        ✅ Unshielded tokens - balance > should get balance of tokens - greater than or equal - true (equal)
        ✅ Unshielded tokens - balance > should get balance of tokens - greater than or equal - false
        ✅ Unshielded tokens - balance > should get balance of tokens - less than or equal - true (equal)
        ✅ Unshielded tokens - balance > should get balance of tokens - less than or equal - false
✅ 10 ❌ 0 ⏭️ 0 2m 58s
✅ testkit-js/testkit-js-e2e/test/unshielded.cross-wallet-transfer.it.test.ts
        ✅ Unshielded cross-wallet transfer (issue #720) > should send night tokens to different wallet via right<>(disclose(addr))
        ✅ Unshielded cross-wallet transfer (issue #720) > should send night tokens to different wallet via disclose(recipient) (issue #720)
✅ 2 ❌ 0 ⏭️ 0 36.0s
✅ testkit-js/testkit-js-e2e/test/unshielded.mint-and-send.it.test.ts
        ✅ Unshielded tokens - mint and send variants > should mint tokens to contract address (self)
        ✅ Unshielded tokens - mint and send variants > should mint tokens to user address
        ✅ Unshielded tokens - mint and send variants > should send tokens to self
        ✅ Unshielded tokens - mint and send variants > should send tokens to contract address (self)
✅ 4 ❌ 0 ⏭️ 0 1m 47s
✅ testkit-js/testkit-js-e2e/test/unshielded.transfer.it.test.ts
        ✅ Unshielded tokens > Custom color > should mint different tokens
        ✅ Unshielded tokens > Custom color > should receive tokens - invalid
        ✅ Unshielded tokens > Custom color > should send tokens to wallet
        ✅ Unshielded tokens > Custom color > should receive tokens from wallet
        ✅ Unshielded tokens > Native color > should transfer night from wallet to contract - receiveNightTokens
        ✅ Unshielded tokens > Native color > should transfer night to wallet - sendNightTokensToUser
✅ 6 ❌ 0 ⏭️ 0 2m 6s

Github Test Reporter by CTRF 💚

🔄 This comment has been updated

jtsang586 and others added 7 commits April 16, 2026 11:41
Adds focused tests for two gaps surfaced in the post-merge coverage
analysis of the last two months:

- packages/contracts (#745): per-recipient encryption key resolver —
  burn-constant structural validation, precedence of the fixed wallet
  and burn keys over additional DApp mappings, resolver invocation per
  non-contract recipient in zswapStateToOffer (the security property
  motivating the original fix).
- packages/dapp-connector-proof-provider (#732): sync throws, non-Error
  rejections, ZKConfigProvider failure, and transient-failure retry
  semantics at both the proving-provider and proof-provider layers.

Coverage: zswap-utils.ts 93.81% stmt / 94.59% branch; dapp-connector
source files 100% across all metrics.

Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
Covers the Protocol ACL package (#832), which had zero test coverage.

Structural tests (protocol-acl.test.ts):
- Barrel namespaces (ledger, compactRuntime, compactJs, onchainRuntime,
  platform) each resolve, are non-empty, and the top-level surface is
  exactly those 5 keys.
- Each subpath has identical member keys to its barrel namespace,
  detecting broken subpath wiring or divergence between import styles.
- Representative symbols (sampleSigningKey, StateValue, entryPointHash,
  asBytes) exist on each subpath, detecting empty/stub modules.
- Effect submodule subpaths (compact-js/effect, compact-js/effect/Contract,
  platform-js/effect/Configuration, platform-js/effect/ContractAddress)
  resolve and expose their expected contents.

ESLint rule tests (eslint-restriction.test.ts):
- Runs the real root eslint.config.mjs via ESLint.lintText.
- Flags direct imports of ledger-v8, compact-runtime, compact-js,
  onchain-runtime-v3, platform-js (and submodules) from consumer packages,
  with the error message pointing at the correct ACL replacement.
- Wildcard patterns guard future ledger / onchain-runtime major bumps.
- All ACL subpath imports are allowed from consumer packages.
- The override for packages/protocol/src/ permits direct imports inside
  the ACL implementation, while still forbidding dist imports.

Adds vitest config and test script to the package.

Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
Whitespace/layout normalisation only — no test behaviour changes.

Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
Adds a spy-based test asserting that StorageEncryption.create is invoked
again after `invalidateEncryptionCache()`, catching regressions where
invalidate silently becomes a no-op. Part of the post-merge coverage
review of PRs #538/#798 (encryption caching + web crypto migration).

Also includes a pre-existing prettier pass against the test file that
was already present in the working tree.

Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
Adds static-source regression tests for #711 (replace shell string
interpolation with safe argument arrays in compact CLI tools). The tests
grep the source of fetch-compact.mts and run-compactc.cjs to guarantee
that exec/execSync with interpolated template literals — the exact
pattern the PR removed — cannot be re-introduced without a test failure.

Also adds vitest config and test script to the compact package, which
previously had no test infrastructure.

Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
…ctor suites

Removes 12 tests across two files after an honest re-evaluation of the
branch's additions:

- zswap-utils.test.ts (7 removed): two tautological burn-constant shape
  assertions; tests that exercise Map.get with multiple entries; edge
  cases that work by construction (empty Map); a bidirectional
  normalisation test already covered in the opposite direction by
  enc-pub-key-resolver.test.ts; two thin-delegation wrapper tests.

- dapp-connector-proving-provider.test.ts (2 removed): "sync throw is
  rejected" and "non-Error rejection value is preserved" both test
  language semantics (async/await behaviour) rather than code.

Net effect: every remaining test now encodes a specific regression it
would catch — resolver precedence against DApp mapping overrides (the
security property of #745), the spy test that proves per-recipient
encryption key selection, and error-path propagation with call-ordering
guarantees.

Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
Adds lockfile entries for vitest, @vitest/coverage-v8, and eslint
declared as devDependencies in the protocol and compact packages.
CI runs `yarn install --immutable` which rejects unlocked deps.

Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
@jtsang586 jtsang586 force-pushed the test/resolver-and-dapp-connector-error-paths branch from 0ba0597 to 507a3d3 Compare April 16, 2026 10:42
@sp-io
Copy link
Copy Markdown
Contributor

sp-io commented Apr 16, 2026

Code review

Found 2 issues:

  1. Tautological assertion on hardcoded constant. BURN_ENCRYPTION_PUBLIC_KEY is a hardcoded hex string starting with f5b9fa49... — asserting it is not 64 zeros can never fail. This does not verify the constant is a valid curve point, only that it differs from a value it was never set to. CLAUDE.md says "Do not write tests just to get coverage, they must be important, valid scenarios."

zswapState: {
currentIndex: 0n,
coinPublicKey: randomCoinPublicKey(),
inputs: useAddressAndChainStateTuple ? nonMatchingInputs.concat(matchingInputs) : matchingInputs,

  1. toBeGreaterThan instead of exact count in cache invalidation test. After invalidateEncryptionCache(), the test asserts createSpy.mock.calls.length is merely greater than before. If a bug caused StorageEncryption.create to be called multiple times on cache miss, this test would still pass. An exact assertion like .toBe(callsBeforeInvalidate + 1) would catch that. CLAUDE.md says "Use strict equality: expect(actual.sort()).toEqual(expected.sort())" (Common Mistakes Initial commit #3 — prefer precise assertions over one-directional checks).

privateStoragePasswordProvider: () => TEST_PASSWORD,
accountId: TEST_ACCOUNT_ID
};
const db = levelPrivateStateProvider<string, string>(config);
db.setContractAddress(INVALIDATE_CONTRACT_ADDRESS);
await db.set('key', 'value');

🤖 Generated with Claude Code

- If this code review was useful, please react with 👍. Otherwise, react with 👎.

jtsang586 and others added 2 commits April 16, 2026 11:57
- Remove tautological burn-constant assertion: BURN_ENCRYPTION_PUBLIC_KEY
  is a hardcoded hex literal that can never be all zeros — the test
  asserted a property that holds by definition.
- Replace toBeGreaterThan with exact .toBe(callsBeforeInvalidate + 1)
  in the cache-invalidation test, catching double-derivation bugs that
  the looser assertion would have missed.

Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
- Remove tautological burn-constant assertion: the hardcoded hex literal
  can never be all zeros.
- Replace toBeGreaterThan with exact .toBe(callsBeforeInvalidate + 1)
  in the cache-invalidation test.
- Remove redundant .not.toBe() in spy test (already proved by the two
  .toBe() assertions above it).
- Revert formatter noise from level-private-state-provider test file to
  keep the diff focused on the new test block only.

Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants