Repository navigation
feat(sdk-coin-xtz): enable Safe wallet recovery - #10071
Conversation
764ccf4 to
9062455
Compare
|
|
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
There was a problem hiding this comment.
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.
Additively extend Xtz.recover() to accept Safe-derived keys from
buildSafeRecoverKeyParams (slot-1 secp256k1Multisig) in both signed and
unsigned modes, and fail closed with RecoveryKeyMismatchError when the
user, backup, or bitgo key is not an owner of the KT1 wallet contract.
Legacy recovery stays byte-for-byte unchanged.
- infer unsigned-sweep mode for Safe calls (bitgoKey-gated) so xpub
Safe params no longer hit bitgo.decrypt
- resolve raw xprv/hex/spsk keys directly; decrypt v1/v2 SJCL envelopes
otherwise (consolidated resolveKeyPair)
- verify contract ownership against the TZKT normalized /storage shape
({ stored_counter, threshold, keys }) before any fee-account balance
check or signing, in both signed and unsigned modes
- add bitgoKey to RecoverOptions and verify the bitgo key is an owner;
reject malformed bitgo keys without echoing the value
- bound and validate the explorer-provided owner list; fail closed on
fetch or parse failures without chaining the apiKey-bearing cause
- Safe E2E tests: signed, unsigned, raw-xprv, xpub-user, owner mismatch,
provider failures, and the legacy no-storage path.
TICKET: WCN-2883
9062455 to
8d11d42
Compare
dpkjnr
left a comment
dpkjnr
left a comment
There was a problem hiding this comment.
Reviewed commit 8d11d422f6d83a6ddd5f478ec100af0de4dfa916. I found two execution blockers in the newly enabled Safe path and one raw-key validation issue.
1. [P1] Signed Safe recovery contains no contract-owner signatures
Location: modules/sdk-coin-xtz/src/xtz.ts:535–544
The resolved user key is used only for ownership validation. Later, txBuilder.sign({ key: backupSigningKey }) signs only the outer operation because the backup address is also the transaction source.
I decoded the returned signed transaction: its contract signature list is [None, None, None]. A normal 2-of-3 wallet will reject it. The Safe path needs user and backup signatures over the contract payload, positioned according to the on-chain owner order, followed by the outer fee-account signature. Please extend the signed-recovery test to verify the contract signatures rather than only asserting that a transaction string exists.
2. [P1] Safe recovery discards the contract counter and substitutes the fee-account nonce
Location: modules/sdk-coin-xtz/src/xtz.ts:340–350
The new storage query retrieves stored_counter but retains only the owner keys. Both packDataToSign() and the contract transfer still use backupKeyNonce.
With storage counter 7 and fee-account counter 100, both signed and unsigned recovery produce contract counter 101, rather than 7. The contract rejects this independently of the missing signatures. Please retain and validate stored_counter, and use it for the contract payload while keeping the manager counter separate. A test with deliberately different counters would catch this.
Scope clarification: the first two defects are inherited from the legacy builder flow, not regressions in legacy recovery introduced by this PR. They nevertheless prevent the newly advertised Safe recovery from producing an executable transaction.
3. [P2] Raw private keys still require an unused passphrase
Location: modules/sdk-coin-xtz/src/xtz.ts:268–273
Although resolveKeyPair() now supports raw xprv/hex/spsk inputs, validation rejects them without walletPassphrase before reaching that resolver. I reproduced raw-xprv Safe recovery failing with missing wallet passphrase.
The new test masks this by supplying 'unused-passphrase'. Please require a passphrase only for inputs that need decryption, and test raw keys with the passphrase omitted.
Validation: all 130 XTZ unit tests passed on Node 24.22.1 using existing workspace dependencies. Additional checks decoded the signed and unsigned Safe transactions and reproduced the signature, counter, and raw-key validation issues described above. No transaction was broadcast.
Safe recovery now produces a transaction the 2-of-3 wallet contract will accept, additively in the Safe branch (bitgoKey present) while the legacy path stays byte-for-byte unchanged: - sign the contract payload with the user and backup keys at their on-chain owner indexes (new TransactionBuilder.addContractSigner, which bypasses source routing so the fee-payer backup key can also emit a contract signature); the backup key still signs the outer source operation - use the contract's stored_counter (from TZKT storage) for the contract payload in packDataToSign and the transfer, keeping the fee-account nonce as the source/manager counter; unsigned mode reports the split via sourceCounter/transferCounters - return ownerKeys + storedCounter from extractContractStorage so the caller can place signatures and counters correctly - require a private user key for signed Safe recovery (clear error for xpub-only input) instead of silently producing an invalid tx - only require walletPassphrase when a key needs decryption, so raw xprv/hex/spsk keys work without one Tests decode the recovered transaction and verify 2-of-2 contract signatures at the right indexes, the contract counter (7) vs the manager counter (101), and raw-key flows with no passphrase. 130 passing, legacy fixtures byte-identical. All changes gated on the Safe marker; legacy recovery unchanged. TICKET: WCN-2883
Addressed, thank you |
What
Additively extend Xtz.recover() to accept Safe-derived keys from buildSafeRecoverKeyParams (slot-1 secp256k1Multisig) in both signed and unsigned modes, and fail closed with RecoveryKeyMismatchError when the user, backup, or bitgo key is not an owner of the KT1 wallet contract. Legacy recovery stays byte-for-byte unchanged.
TICKET: WCN-2883