From e3eef0b1316da2a94007e313b07880e2d95d63e7 Mon Sep 17 00:00:00 2001 From: maclane Date: Sat, 27 Jun 2026 16:36:27 -0400 Subject: [PATCH] fix(ethereum): route GetWallet's wallet-ID fallback by scheme, not error 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) --- pkg/chain/ethereum/tbtc.go | 44 ++++++++++++- pkg/chain/ethereum/tbtc_test.go | 106 ++++++++++++++++++++++++++++++++ 2 files changed, 147 insertions(+), 3 deletions(-) diff --git a/pkg/chain/ethereum/tbtc.go b/pkg/chain/ethereum/tbtc.go index 858468849e..5d1a42ce5f 100644 --- a/pkg/chain/ethereum/tbtc.go +++ b/pkg/chain/ethereum/tbtc.go @@ -2166,13 +2166,13 @@ func (tc *TbtcChain) GetWallet( return nil, fmt.Errorf("cannot parse wallet state: [%v]", err) } - walletID, err := walletIDForWalletPublicKeyHash( + walletID, err := resolveWalletID( tc.bridge, walletPublicKeyHash, + wallet.EcdsaWalletID, ) if err != nil { - // Fallback for legacy deployments where walletID accessor may not exist. - walletID = tbtc.DeriveLegacyWalletID(walletPublicKeyHash) + return nil, err } return &tbtc.WalletChainData{ @@ -2214,6 +2214,44 @@ func walletIDForWalletPublicKeyHash( return resolver.WalletID(walletPublicKeyHash) } +// resolveWalletID returns the canonical wallet ID for the wallet identified by +// walletPublicKeyHash. ecdsaWalletID is that wallet's ECDSA wallet ID from the +// Bridge record -- zero for FROST wallets, non-zero for legacy ECDSA wallets. +// +// On an accessor error the fallback is routed by SCHEME, not by error type +// (which cannot reliably distinguish a legacy on-chain Bridge -- whose walletID +// eth_call returns a normal RPC/ABI error even when the node uses the current +// binding -- from a transient failure): +// +// - A legacy ECDSA wallet's canonical wallet ID equals its legacy derivation, +// so falling back is correct, and it is the only option on a legacy Bridge +// whose contract lacks the walletID accessor. +// - A FROST wallet requires its canonical wallet ID; the legacy derivation +// would be a different value and would select the wrong (P2WPKH vs P2TR) +// wallet script, so the error is surfaced instead of falling back. A FROST +// wallet only exists on a canonical-ID Bridge, so such an error is genuinely +// transient. +func resolveWalletID( + bridge any, + walletPublicKeyHash [20]byte, + ecdsaWalletID [32]byte, +) ([32]byte, error) { + walletID, err := walletIDForWalletPublicKeyHash(bridge, walletPublicKeyHash) + if err == nil { + return walletID, nil + } + + if ecdsaWalletID == ([32]byte{}) { + return [32]byte{}, fmt.Errorf( + "cannot resolve canonical wallet ID for FROST wallet [0x%x]: [%w]", + walletPublicKeyHash, + err, + ) + } + + return tbtc.DeriveLegacyWalletID(walletPublicKeyHash), nil +} + type walletPublicKeyHashForWalletIDFn interface { WalletPubKeyHashForWalletID(walletID [32]byte) ([20]byte, error) } diff --git a/pkg/chain/ethereum/tbtc_test.go b/pkg/chain/ethereum/tbtc_test.go index 206c7dead1..b2c800d206 100644 --- a/pkg/chain/ethereum/tbtc_test.go +++ b/pkg/chain/ethereum/tbtc_test.go @@ -826,6 +826,112 @@ func TestPastNewWalletRegisteredV2Events_ReturnsErrorWhenRawMissing(t *testing.T } } +type walletIDForWalletPublicKeyHashBridgeMock struct { + resolve func(walletPublicKeyHash [20]byte) ([32]byte, error) +} + +func (m *walletIDForWalletPublicKeyHashBridgeMock) WalletID( + walletPublicKeyHash [20]byte, +) ([32]byte, error) { + return m.resolve(walletPublicKeyHash) +} + +func TestResolveWalletID(t *testing.T) { + walletPublicKeyHash := [20]byte{0xaa} + legacyEcdsaWalletID := [32]byte{0x07} // non-zero -> legacy ECDSA wallet + frostEcdsaWalletID := [32]byte{} // zero -> FROST wallet + + t.Run("returns the canonical wallet ID when the accessor succeeds", func(t *testing.T) { + expectedWalletID := [32]byte{0x01} + + actualWalletID, err := resolveWalletID( + &walletIDForWalletPublicKeyHashBridgeMock{ + resolve: func(actual [20]byte) ([32]byte, error) { + if actual != walletPublicKeyHash { + t.Fatalf("unexpected wallet public key hash: [%x]", actual) + } + return expectedWalletID, nil + }, + }, + walletPublicKeyHash, + frostEcdsaWalletID, + ) + if err != nil { + t.Fatalf("unexpected error: [%v]", err) + } + if actualWalletID != expectedWalletID { + t.Fatalf( + "unexpected wallet ID\nexpected: [%x]\nactual: [%x]", + expectedWalletID, + actualWalletID, + ) + } + }) + + t.Run("surfaces the error for a FROST wallet when the accessor fails", func(t *testing.T) { + // A FROST wallet (zero ECDSA wallet ID) requires its canonical ID; the + // legacy derivation would be the wrong value (wrong P2WPKH vs P2TR + // script), so the error must surface rather than fall back. + _, err := resolveWalletID( + &walletIDForWalletPublicKeyHashBridgeMock{ + resolve: func([20]byte) ([32]byte, error) { + return [32]byte{}, errors.New("execution reverted: temporary") + }, + }, + walletPublicKeyHash, + frostEcdsaWalletID, + ) + if err == nil { + t.Fatal("expected an error for a FROST wallet with an unresolvable canonical ID") + } + }) + + t.Run("falls back to the legacy ID for a legacy ECDSA wallet on accessor error", func(t *testing.T) { + // Regression for legacy on-chain Bridges: the accessor exists in the + // binding but the contract lacks the walletID function, so the call + // returns a normal RPC/ABI error (NOT a typed missing-accessor signal). A + // legacy ECDSA wallet (non-zero ECDSA wallet ID) must still fall back. + actualWalletID, err := resolveWalletID( + &walletIDForWalletPublicKeyHashBridgeMock{ + resolve: func([20]byte) ([32]byte, error) { + return [32]byte{}, errors.New("execution reverted") + }, + }, + walletPublicKeyHash, + legacyEcdsaWalletID, + ) + if err != nil { + t.Fatalf("unexpected error: [%v]", err) + } + if expected := tbtcpkg.DeriveLegacyWalletID(walletPublicKeyHash); actualWalletID != expected { + t.Fatalf( + "unexpected wallet ID\nexpected: [%x]\nactual: [%x]", + expected, + actualWalletID, + ) + } + }) + + t.Run("falls back to the legacy ID for a legacy wallet on a Bridge without the accessor", func(t *testing.T) { + // Legacy deployment where the binding itself lacks the accessor. + actualWalletID, err := resolveWalletID( + struct{}{}, + walletPublicKeyHash, + legacyEcdsaWalletID, + ) + if err != nil { + t.Fatalf("unexpected error: [%v]", err) + } + if expected := tbtcpkg.DeriveLegacyWalletID(walletPublicKeyHash); actualWalletID != expected { + t.Fatalf( + "unexpected wallet ID\nexpected: [%x]\nactual: [%x]", + expected, + actualWalletID, + ) + } + }) +} + type walletPublicKeyHashForWalletIDBridgeMock struct { resolve func(walletID [32]byte) ([20]byte, error) }