[pranishnepal]

What

Makes coin.recover() work with Safe wallet keycards across EVM TSS, STX, AVAXC and Cosmos.

A Safe wallet derives one child key set from a 4-slot keycard, which buildSafeRecoverKeyParams maps into recover(). In unsigned mode that shape is { bitgoKey, isTss, broadcast } — no userKey, no backupKey. Four coin families either rejected that shape outright or skipped the RecoveryKeyMismatchError / ErrorNoInputToRecover / RecoveryProviderError contracts the recovery orchestrator uses to decide whether to try the next key slot, keep scanning, or stop and surface a network failure.

WCN-3946

Don't panic at 29 files — here's the shape of it

	files	+lines	what it is
New tests	5	1,221 (79%)	end-to-end Safe recovery suites
Real logic	5	~260	the actual change — review these
Mechanical ripple	6	67	same 3-line edit in each
Fixture touch-ups	8	76	1–2 lines each
package.json	2	3	declare test-only deps

Review these five, skip the rest:

• abstract-eth/src/abstractEthLikeNewCoins.ts — accept the bitgoKey-only shape; verify the m/0 address before any balance query; dispatch MPCV2 coins to buildUnsignedSweepTxnMPCv2 from the base class
• sdk-coin-stx/src/stx.ts — derive the 2-of-3 multisig address from the child pubs and compare it to rootAddress before touching the node
• sdk-coin-avaxc/src/avaxc.ts — check user/backup keys are wallet-contract signers before spending gas, via a JSON-RPC queryIsWalletSigner override (avax.network isn't Etherscan-compatible)
• abstract-cosmos/src/cosmosCoin.ts — derive senderAddress from bitgoKey in unsigned mode instead of trusting rootAddress
• sdk-core/src/bitgo/recovery/initiate.ts — new classifyRecoveryKeys helper (see below)

Why the ripple exists

RecoverOptions.userKey and .backupKey were required strings. The unsigned Safe shape has neither, so they had to become optional — otherwise every Safe caller needs a cast and the type keeps claiming two keys the runtime no longer requires.

That one-line type change is the entire reason 6 unrelated files are touched: each reads params.userKey.replace(...), which TypeScript then flags. Every one of those edits is the same shape:

const recoveryKeys = classifyRecoveryKeys(params);
const { backupKey, isKrsRecovery, isUnsignedSweep } = recoveryKeys;
let userKey = recoveryKeys.userKey;

---

Testing

End-to-end suites for eth, stx, avaxc and cosmos driving the real chain (deriveSafeWalletKeys → buildSafeRecoverKeyParams → recover()) with only the network mocked. Each covers signed and unsigned happy paths, wrong passphrase, empty balance and provider failure.

Plus a bsc test pinning the MPCv2 dispatch, and a regression test for derivationSeed sweeps.

[linear-code]

WCN-3946

[github-actions]

⚠️ Compatibility checks are failing on Node.js 27-nightly (experimental, non-blocking). This does not block merge, but it signals an incompatibility to fix before this release becomes supported.

View run

[pranishnepal]

@claude review always

[github-actions]

Claude finished @pranishnepal's task in 47s —— View job

---

Review of #10106: Safe keycard params for EVM TSS / STX / AVAXC / Cosmos recovery

I read the source diff across abstract-eth, abstract-cosmos, stx, avaxc, evm, etc, tempo and the eth tokens. I did not read the new test files in detail and did not run the build or tests.

The design is sound. The checks run before any network call, and the error contracts (RecoveryKeyMismatchError, ErrorNoInputToRecover, RecoveryProviderError) are used consistently. I have no blocking issues. The points below are worth a look before merge.

Worth addressing

1. Possible behaviour change for existing unsigned TSS callers (abstractEthLikeNewCoins.ts, buildUnsignedSweepTxnTSS and buildUnsignedSweepTxnMPCv2).

   • assertTssBaseAddressMatchesWallet was previously enforced only on the signed MPCv2 path.
   • It now also runs on the unsigned paths, and also for MPCv1 coins.
   • An existing offline-vault caller that passes a forwarder or other non-m/0 address as walletContractAddress for an unsigned TSS sweep will now fail with RecoveryKeyMismatchError. Before this PR it built a tx from the m/0 address.
   • That is probably the intended fail-closed behaviour, but it is a breaking change for those callers. Please call it out in the PR description or release notes, and confirm that no WRW or CLI flow relies on the old behaviour.

2. recoverConsolidations now overrides walletContractAddress for TSS (abstractEthLikeNewCoins.ts:1563).

   • It passes the derived m/0 address instead of the per-index consolidation address.
   • This makes the new mismatch check pass trivially for every index, so it isn't a real check in that flow.
   • getTssCommonKeychain prefers backupKey ‖ userKey ‖ bitgoKey, but getConsolidationAddress derives from params.userKey (line 1520). If the two ever differ, the two paths use different keychains.
   • If getTssCommonKeychain returns an empty string, deriveAddressFromPublicKey throws inside the loop. Check how the surrounding code handles that.

3. Inconsistent key classification in eth.ts (modules/sdk-coin-eth/src/eth.ts).

   • The other files (erc20, erc721, erc7984, etc, avaxc, base) classify from the raw keys and say so in a comment: "stripping whitespace first would flip the result for a key pasted with a leading newline".
   • Eth.recover instead moves the whitespace strip first and classifies from the stripped keys. That is the opposite of the behaviour the other comments describe, and it also changes the pre-existing eth.ts behaviour.
   • Please make it consistent. Using the raw keys here too looks right.

4. AVAXC drops apiKey (modules/sdk-coin-avaxc/src/avaxc.ts).

   • assertRecoveryKeysAreWalletSigners(userKey, backupKeyAddress, params.walletContractAddress) is called without params.apiKey. The base class passes it.
   • The override accepts apiKey and forwards it to recoveryBlockchainExplorerQuery, so it should be threaded through.
   • The other AVAXC calls (getAddressNonce, balance) may not use it either. If so, this matches existing behaviour, but it is worth checking.

Minor

• validateUnsignedSweepTSSParams: it was async but never awaited, and the old condition isUndefined(x) && x === '' could never be true. It is now awaited and the condition is real, so it can start throwing where it was previously a no-op. This is a good fix, but a test for the missing commonKeyChain path would help. I couldn't confirm there is one.
• Cosmos isSameBaseAddress: it lowercases and strips ?memoId=. That is fine for bech32. Stripping the memo means the unsigned senderAddress no longer carries a memoId from rootAddress, which is a small behavioural difference. Confirm that is acceptable.
• STX: the getSTXAddressFromPubKeys call runs after getBip32Keys, but getBip32Keys with requireBitGoXpub may still do a lookup. Fine if it is offline, as the PR claims.
• Duplicated "raw keys" comment: the same 3-line comment is pasted into about 7 files. A tiny shared helper (e.g. classifyRecoveryKeys(params)) would avoid the drift in item 3.
• evmCoin.ts: removing the override is correct given the base-class dispatch. Subclasses that override buildUnsignedSweepTxnTSS (e.g. tempo) bypass the dispatch, which is fine because tempo has its own MPCv2 body.

Positive

• The MPCv2 dispatch in the base class fixes the bsc, arbeth, opeth and bera Unsupported public key failures, and the bsc test pins it.
• The mismatch tests assert that no network call was made before the error, which is the right invariant to test.
  · branch WCN-3946

[claude]

Findings marked 🟡 are optional suggestions and need no follow-up push.

Additional findings (outside the current diff — GitHub can't attach inline comments there):

• 🟡 modules/sdk-coin-eth/src/eth.ts — nit: for the same keys, ETH and the ERC20/ERC721/ERC7984 tokens can now disagree on whether a recovery is KRS or an unsigned sweep. Eth.recoverEthLike strips whitespace before calling getIsKrsRecovery/getIsUnsignedSweep. Every other consumer classifies from the raw keys, and the commit message says all of them do. Fix: classify the same way at every site, either all raw or all stripped.

  Why this was flagged

  Take a backup xpub pasted with a leading newline. At eth.ts:640-644 the ETH coin strips whitespace, so getIsKrsRecovery and getIsUnsignedSweep see a key starting with xpub. That makes the recovery KRS or unsigned-sweep, and checkKrsProvider runs. erc20Token.ts:540-545, erc721Token.ts, erc7984Token.ts, ethLikeToken.ts, avaxc.ts and etc.ts classify from rawUserKey/rawBackupKey. For the same input they treat it as signed and then fail to decrypt the xpub. Before this change ETH also used the raw keys, so this divergence is new.

  Verification: Nit. The trigger is an ETH (non-TSS) recovery where a user or backup key is passed with leading whitespace. Eth.recoverEthLike at modules/sdk-coin-eth/src/eth.ts:209-214 strips whitespace first, then classifies from the stripped keys. The base code classified from the raw params. erc20Token.ts:159-162 and the other token consumers classify from the raw keys. Both helpers use startsWith('xpub'), so a leading '\n' really does flip the result.

[claude]

Claude Code Review

This repository is configured for manual code reviews. Comment @claude review for a one-time review, or @claude review always to subscribe this PR to a review on every future push.

_{Tip: disable this comment in your organization's Code Review settings.}

[pranishnepal]

@claude review always

[github-actions]

Claude finished @pranishnepal's task in 19s —— View job

---

Review of #10106 (re-review of current head)

I re-read the eth.ts, tempo.ts and initiate.ts diffs. I did not re-read the other source files or the new test suites in this pass, and I did not run the build or tests.

Earlier findings that are now fixed in the branch

• Tempo mismatch check: both Tempo paths call assertTssBaseAddressMatchesWallet. That covers signed recoverTSS and buildUnsignedSweepTxnTSS. The unsigned path skips the check only when derivationSeed is set, which matches the base class. Tempo now fails closed with RecoveryKeyMismatchError on a wrong keychain.
• Key classification: classifyRecoveryKeys in sdk-core classifies from the raw keys and returns the stripped keys. Eth.recover uses it, so ETH, the tokens, ETC and AVAXC can no longer disagree on KRS or unsigned-sweep classification for the same input.
• Missing commonKeyChain: Tempo.buildUnsignedSweepTxnTSS throws missing commonKeyChain when no keychain can be resolved.

Still worth confirming (non-blocking)

1. Behaviour change for unsigned TSS callers: assertTssBaseAddressMatchesWallet now runs on unsigned and MPCv1 paths too. An offline-vault caller that passes a non-m/0 walletContractAddress will now get RecoveryKeyMismatchError. This is probably the intended fail-closed behaviour. Please note it in the release notes and confirm that no WRW or CLI flow relies on the old behaviour.
2. recoverConsolidations: it passes the derived m/0 address as walletContractAddress for TSS, so the new check passes trivially there. Also, getTssCommonKeychain and getConsolidationAddress may resolve different keychains (backupKey ‖ userKey ‖ bitgoKey versus params.userKey).
3. AVAXC: assertRecoveryKeysAreWalletSigners is called without params.apiKey. The base class forwards it. Check whether this was intended.
4. Cosmos: isSameBaseAddress strips ?memoId=, so the unsigned senderAddress no longer carries a memo from rootAddress. Confirm that is acceptable.
5. Test coverage: I couldn't confirm a test for the missing commonKeyChain path now that validateUnsignedSweepTSSParams is awaited and its condition is real. Tempo's new throw has the same gap.

I have no blocking issues. The earlier red items are resolved.
· branch WCN-3946

[claude]

Nothing blocking. The comments below are optional suggestions. There is no need to push a fix for them before merging.

Additional findings (outside the current diff — GitHub can't attach inline comments there):

• 🟡 modules/abstract-eth/src/abstractEthLikeNewCoins.ts — TSS consolidation callers now get many identical sweeps of the m/0 base address back, where signed MPCv2 runs used to stop with RecoveryKeyMismatchError. Every scanned index and candidate address gets walletContractAddress: tssBaseAddress, so each recover() call passes the address check and rebuilds the same m/0 sweep. The m/i receive addresses being scanned are never swept. Fix: when isTss, build and return the m/0 sweep at most once, or sweep each m/i (for example via a per-index derivation). Do not substitute the address being compared, because that turns the mismatch check into a no-op. [also at: modules/abstract-eth/src/abstractEthLikeNewCoins.ts:1577 - TSS consolidation recoveries with the wrong keychain will still sweep, with no RecoveryKeyMismatchError. The new wallet-address guard is never checked against what the caller supplied. recoverConsolidations derives tssBaseAddress from the keychain and passes it as walletContractAddress.]

  Why this was flagged

  Trigger: recoverConsolidations called with isTss: true and a common keychain in userKey, backupKey or bitgoKey (the bitgoKey-only shape is newly accepted by validateRecoveryParams). tssBaseAddress is loop-invariant (line 1587) and replaces every consolidation address. recoverTSS/buildUnsignedSweepTxnMPCv2 always derive m/0 and call buildTssRecoveryTxn(baseAddress, ...), which never reads walletContractAddress (line 2711). So each of the (endIdx - startIdx) x addresses iterations queries the same balance and nonce, and pushes an identical m/0 sweep into consolidatedTransactions. Only the first copy can confirm, since all share one nonce, and no receive address m/i is consolidated. On the base branch, the signed MPCv2 path compared m/0 with the per-index address and threw RecoveryKeyMismatchError on the first iteration. assertTssBaseAddressMatchesWallet cannot catch this because it now compares m/0 with itself.

  Verification: Every iteration passes walletContractAddress: tssBaseAddress ?? address (abstractEthLikeNewCoins.ts:1577). buildTssRecoveryTxn(baseAddress, ...) (line 2710-2738) never reads walletContractAddress. The m/i receive addresses are never queried or swept. Existing tests use startingScanIndex: 1, endingScanIndex: 2 (eth.ts ~3617), so they do not catch the duplication.

• 🟣 modules/abstract-eth/src/abstractEthLikeNewCoins.ts — Pre-existing: Safe recoveries of an empty EVM TSS wallet still fail with a plain Error, not ErrorNoInputToRecover, so the orchestrator cannot tell "nothing to recover" from a real failure. validateBalanceAndGetTxAmount throws Backup key address ... has balance 0 Gwei whenever the balance is below gas, including zero. Fix: throw ErrorNoInputToRecover when the swept address balance is zero, before the gas check, on every TSS sweep path, while keeping the plain gas error for a non-zero balance below gas. Same pattern at 2 sites (abstractEthLikeNewCoins.ts:2749, tempo.ts:449).

  Why this was flagged

  The trigger is a Safe ecdsaMpc recovery, { bitgoKey, isTss: true } or the signed shape, on an EVM TSS coin whose m/0 base address holds zero balance. It goes recover() -> recoverTSS -> buildUnsignedSweepTxnMPCv2, or the signed branch, -> buildTssRecoveryTxn (abstractEthLikeNewCoins.ts:2711). validateBalanceAndGetTxAmount then runs baseAddressBalance.lt(totalGasNeeded) at :2749 and throws a plain Error that names a "Backup key address". The PR says it makes EVM TSS honour the ErrorNoInputToRecover contract the orchestrator uses to keep scanning. For EVM TSS and Tempo it did not, and ethSafeRecovery.ts:252 pins the plain /has balance 0 Gwei/ error. Tempo's buildTempoSweepTx (tempo.ts:449) likewise throws a plain Insufficient balance in ... Error. The bitgoKey-only shape is newly accepted (validateRecoveryParams :1352), so Safe callers newly reach this path. They get a generic failure where the contract promises ErrorNoInputToRecover.

  Verification: At abstractEthLikeNewCoins.ts:2749 validateBalanceAndGetTxAmount does if (baseAddressBalance.lt(totalGasNeeded)) and throws a plain Error. Nothing checks for zero balance first, so an empty wallet gets a plain Error, never ErrorNoInputToRecover. recoverConsolidations (:1595) only treats that type as "nothing here". The base commit has the same throw (base :2694), so this is not a regression.

[claude]

No new blocking issues. 1 optional suggestion (a nit or a note on pre-existing code) was found and not posted. Nothing in this review needs a push before merging.