Skip to content

fix(ethereum): route GetWallet's wallet-ID fallback by scheme, not error type#4122

Merged
mswilkison merged 1 commit into
feat/frost-schnorr-migration-scaffoldfrom
fix/canonical-walletid-fallback-on-missing-accessor
Jun 27, 2026
Merged

fix(ethereum): route GetWallet's wallet-ID fallback by scheme, not error type#4122
mswilkison merged 1 commit into
feat/frost-schnorr-migration-scaffoldfrom
fix/canonical-walletid-fallback-on-missing-accessor

Conversation

@mswilkison

@mswilkison mswilkison commented Jun 27, 2026

Copy link
Copy Markdown
Contributor

Summary

Addresses a Codex finding (relayed via the #4119 review): TbtcChain.GetWallet derived a legacy wallet ID on any error from the canonical walletID accessor. For a FROST wallet on a canonical Bridge, a transient call failure would silently yield the left-padded legacy ID — and callers use WalletChainData.WalletID to choose P2TR (FROST) vs P2WPKH (legacy) scripts, so the node would build or search the wrong wallet script.

Why route by scheme (revised after Codex P1)

The first revision distinguished by error type (a sentinel for the missing accessor, surface everything else). Codex correctly flagged a P1 regression: a legacy on-chain Bridge built with the current generated binding still satisfies the accessor interface, so its missing walletID function returns a normal RPC/ABI error — not the sentinel — and that revision would surface it and break GetWallet on exactly the legacy deployments the fallback exists for. Error type cannot reliably separate "function absent on-chain" from "transient."

So this routes by scheme, using the wallet's EcdsaWalletID (which GetWallet already reads, and which the codebase already uses to infer scheme — zero ⇒ FROST):

  • Legacy ECDSA wallet (EcdsaWalletID != 0): its canonical wallet ID equals its legacy derivation, so fall back on any accessor error — and it's the only option on a legacy Bridge lacking the accessor.
  • FROST wallet (EcdsaWalletID == 0): requires the canonical ID; surface the error rather than return a wrong legacy ID. A FROST wallet only exists on a canonical-ID Bridge, so such an error is genuinely transient.

Logic is extracted into resolveWalletID(bridge, walletPublicKeyHash, ecdsaWalletID).

Tests

TestResolveWalletID covers all four cases: accessor success → canonical; FROST + accessor error → surfaced; legacy + accessor error → legacy fallback (the P1 regression guard — verified to fail if the routing surfaces errors for legacy wallets); legacy + missing-accessor binding → legacy fallback. gofmt + go vet clean; full pkg/chain/ethereum suite passes.

Found during the Codex review batch on #4115#4120; revised per the Codex P1 re-review.

Co-Authored-By: Claude Opus 4.8 (1M context) noreply@anthropic.com

🤖 Generated with Claude Code

@coderabbitai

coderabbitai Bot commented Jun 27, 2026

Copy link
Copy Markdown

Important

Review skipped

Auto reviews are disabled on base/target branches other than the default branch.

Please check the settings in the CodeRabbit UI or the .coderabbit.yaml file in this repository. To trigger a single review, invoke the @coderabbitai review command.

⚙️ Run configuration

Configuration used: defaults

Review profile: CHILL

Plan: Pro Plus

Run ID: fa8d44a2-fb68-41bd-8951-17566a447a11

You can disable this status message by setting the reviews.review_status to false in the CodeRabbit configuration file.

Use the checkbox below for a quick retry:

  • 🔍 Trigger review
✨ Finishing Touches
🧪 Generate unit tests (beta)
  • Create PR with unit tests
  • Commit unit tests in branch fix/canonical-walletid-fallback-on-missing-accessor

Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out.

❤️ Share

Comment @coderabbitai help to get the list of available commands.

…ror type

GetWallet derived a legacy wallet ID on ANY error from the canonical walletID
accessor. For a FROST wallet on a canonical Bridge, a transient call failure
would silently yield the left-padded legacy ID, and callers use
WalletChainData.WalletID to choose P2TR (FROST) vs P2WPKH (legacy) scripts, so it
would build or search the wrong wallet script.

The error type cannot reliably tell a legacy on-chain Bridge -- where the walletID
eth_call returns a normal RPC/ABI error even with the current binding -- from a
transient failure, so distinguishing by error is fragile and breaks legacy
deployments (Codex P1 on the first revision of this PR). Route the fallback by
SCHEME instead, using the wallet's ECDSA wallet ID (zero => FROST, non-zero =>
legacy ECDSA), which GetWallet already has:

  - Legacy ECDSA wallet: its canonical wallet ID equals the legacy derivation, so
    fall back on any accessor error (and it is the only option on a legacy Bridge
    lacking the accessor).
  - FROST wallet: requires the canonical ID; surface the error rather than return
    a wrong legacy ID. A FROST wallet only exists on a canonical-ID Bridge, so the
    error is genuinely transient.

Extracted into resolveWalletID with TestResolveWalletID covering all four cases.

Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
@mswilkison mswilkison force-pushed the fix/canonical-walletid-fallback-on-missing-accessor branch from 3e0e61c to e3eef0b Compare June 27, 2026 21:00
@mswilkison mswilkison changed the title fix(ethereum): only derive a legacy wallet ID when the canonical accessor is absent fix(ethereum): route GetWallet's wallet-ID fallback by scheme, not error type Jun 27, 2026
@mswilkison mswilkison merged commit 71ce32b into feat/frost-schnorr-migration-scaffold Jun 27, 2026
16 checks passed
@mswilkison mswilkison deleted the fix/canonical-walletid-fallback-on-missing-accessor branch June 27, 2026 22:06
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.

1 participant