Skip to content

feat(sdk-coin-sui): support MPCv2 signed hot recovery - #9493

Open
ralph-bitgo[bot] wants to merge 2 commits into
masterfrom
wci-1224-sui-mpcv2-signed-hot-recovery
Open

feat(sdk-coin-sui): support MPCv2 signed hot recovery#9493
ralph-bitgo[bot] wants to merge 2 commits into
masterfrom
wci-1224-sui-mpcv2-signed-hot-recovery

Conversation

@ralph-bitgo

@ralph-bitgo ralph-bitgo Bot commented Aug 13, 2026

Copy link
Copy Markdown

What

  • Sui.recover() / the private signRecoveryTransaction() now dispatch on the EddsaSigningMaterial discriminated union (version: 'v1' | 'v2') instead of only handling MPCv1 JSON keycards.
  • Two new protected methods wrap the shared @bitgo/sdk-core EdDSA MPCv2 recovery helpers so they can be stubbed with sinon in tests:
    • getEddsaSigningMaterial(userKey, passphrase) — detects MPCv1 JSON vs MPCv2 CBOR keycards.
    • signSuiMpcV2Recovery(params) — runs the MPS DSG signing flow locally and returns the raw 64-byte Ed25519 signature.
  • For the 'v2' branch, the raw signature is passed through the existing txBuilder.addSignature() / setSerializedSig() flow, which already wraps it in SUI's 0x00 flag + pubkey signature envelope — unchanged from the MPCv1 path.
  • The MPCv1 backup-key decryption now goes through decryptKeychainPrivateKey (shared helper) instead of a bespoke try/catch, and parameter guards use assert() per the pattern established for other MPCv2 coin migrations.
  • Both native SUI transfer recovery (recover()) and token transfer recovery (recoverSuiToken() / getTokenTransferBuilder) share the same dispatch, since both call signRecoveryTransaction().
  • createBroadcastableSweepTransaction() (unsigned sweep / OVC path) and recoverConsolidations() MPCv2 support are unaffected/out of scope (the latter is tracked separately in WCI-1234).
  • Added unit tests mirroring the MPCv2 suite pattern used for TON/SOL: MPCv2 keycard routes to the v2 path (getTSSSignature not called, SUI envelope applied) for both native and token transfers, MPCv1 keycard still routes to the v1 path (regression), and a mismatched bitgoKey vs keycard commonKeyChain throws.

Why

Sui wallets using the newer Silence Labs MPCv2 EdDSA key scheme could not previously complete self-hosted (hot) recovery, since recover() only understood MPCv1 JSON keycards and would fail to parse or sign with an MPCv2 keycard. This closes that gap using the shared EdDSA MPCv2 recovery helpers that landed in @bitgo/sdk-core (WCI-1276), following the same pattern already adopted for TON and SOL as part of the broader EDDSA upgrade from Zengo to Silence Labs.

Test plan

  • yarn build (tsc, scoped to @bitgo/sdk-coin-sui) compiles cleanly.
  • yarn eslint --quiet src/sui.ts test/unit/sui.ts — no lint errors.
  • yarn unit-test — 204 passing, 0 failing (all existing tests plus 4 new MPCv2 tests: native-transfer MPCv2 routing, native-transfer MPCv1 regression, token-transfer MPCv2 routing, and mismatched-key rejection).

Ticket: WCI-1224

@linear-code

linear-code Bot commented Aug 13, 2026

Copy link
Copy Markdown
Contributor

WCI-1224

@bitgo-ai-agent-dev
bitgo-ai-agent-dev Bot force-pushed the wci-1224-sui-mpcv2-signed-hot-recovery branch from 8e6e2c1 to 0ef61b5 Compare August 13, 2026 10:32
@ralph-bitgo
ralph-bitgo Bot force-pushed the wci-1224-sui-mpcv2-signed-hot-recovery branch from 0ef61b5 to 4c55071 Compare August 13, 2026 10:32
@vibhavgo @bitgobot
feat(sdk-coin-sui): support MPCv2 signed hot recovery
a4090f2 
Update Sui.recover()/signRecoveryTransaction() to detect and sign
MPCv2 keycards using the shared eddsaMPCv2 helpers from @bitgo/sdk-core
(getEddsaSigningMaterial, signEddsaMpcV2RecoveryTx), dispatching on the
EddsaSigningMaterial discriminated union instead of a boolean flag.
Both native SUI transfer and token transfer recovery share this path.
The raw 64-byte MPCv2 signature is wrapped in SUI's existing 0x00 flag
+ pubkey signature envelope via the current addSignature/
setSerializedSig flow, unchanged for MPCv1. MPCv1 backup key
decryption now goes through decryptKeychainPrivateKey instead of a
bespoke try/catch, and parameter guards use assert() per the shared
recovery pattern.

Sui wallets using the newer Silence Labs MPCv2 EdDSA key scheme could
not previously complete self-hosted (hot) recovery, since recover()
only understood MPCv1 JSON keycards. This mirrors the pattern already
adopted by TON/SOL for the EDDSA upgrade from Zengo to Silence Labs.

Ticket: WCI-1224
Session-Id: 0c067bc3-368d-4c82-9e68-8d08c89766d5
Task-Id: 8c5b0392-4a96-4479-a83b-8ba80cd2428d
@ralph-bitgo
ralph-bitgo Bot force-pushed the wci-1224-sui-mpcv2-signed-hot-recovery branch from 4c55071 to 0bc2ea6 Compare August 13, 2026 11:11
@bitgo-ai-agent-dev
bitgo-ai-agent-dev Bot force-pushed the wci-1224-sui-mpcv2-signed-hot-recovery branch from 0bc2ea6 to e362cfa Compare August 13, 2026 11:11
@ralph-bitgo
ralph-bitgo Bot force-pushed the wci-1224-sui-mpcv2-signed-hot-recovery branch from e362cfa to 4994308 Compare August 13, 2026 11:11
@vibhavgo
vibhavgo marked this pull request as ready for review August 13, 2026 11:45
@vibhavgo
vibhavgo requested a review from a team as a code owner August 13, 2026 11:45
@ralph-bitgo ralph-bitgo Bot mentioned this pull request Aug 13, 2026
Closed
5 tasks
@vibhavgo vibhavgo mentioned this pull request Aug 14, 2026
Open
5 tasks
@vibhavgo

vibhavgo commented Aug 14, 2026

Copy link
Copy Markdown
Contributor

AI-generated review — auto-generated by Claude Code (wallet-platform:review-pr). Please use your own judgment before acting on any findings.

Phase 1 — Summary

signRecoveryTransaction() in sdk-coin-sui now dispatches on the EddsaSigningMaterial discriminated union (version: 'v1' | 'v2'). Two protected wrapper methods (getEddsaSigningMaterial, signSuiMpcV2Recovery) are introduced to make the shared @bitgo/sdk-core EdDSA MPCv2 helpers stubbable in tests. MPCv1 backup-key decryption is migrated to decryptKeychainPrivateKey; parameter guards are replaced with assert().

Files changed: 2 | +223 / −30

flowchart TD
    A[signRecoveryTransaction] --> B[getEddsaSigningMaterial\nuserKey stripped]
    B -->|version === 'v2'| C[signSuiMpcV2Recovery\nMPS DSG]
    C --> D[txBuilder.addSignature\nraw 64-byte sig]
    D --> E[SUI envelope 0x00+sig+pub = 97 bytes]
    B -->|version === 'v1'| F[JSON.parse userPrv\ndecryptKeychainPrivateKey backupKey]
    F --> G[EDDSAMethods.getTSSSignature\nlegacy MPC]
    G --> H[txBuilder.addSignature]
Loading

Phase 2 — Ticket Alignment

Ticket: WCI-1224

The PR title (feat(sdk-coin-sui): support MPCv2 signed hot recovery) and body description align well with the implied ticket goal of adding MPCv2 signed recovery for SUI. Implementation covers both native and token transfer paths via the shared signRecoveryTransaction() dispatcher, and explicitly calls out out-of-scope items (unsigned sweep / consolidations tracked separately in WCI-1234). Alignment is ✅.

Phase 3 — Index Analysis

No new database queries introduced.

Phase 4 — Type Safety

No as any, @ts-ignore, or double casts found in added lines. The sinon stub cast uses the approved pattern:

Sui.prototype as unknown as { getEddsaSigningMaterial: unknown }

✅ Clean.

Phase 5 — MPCv2 Correctness & Test Quality

Correctness

Check Result Notes
getEddsaSigningMaterial called with whitespace-stripped userKey ✅ userKey = params.userKey.replace(/\s/g, '') before the call
SUI envelope: txBuilder.addSignature wraps raw 64-byte sig in 97-byte envelope ✅ PR body confirms existing builder handles this; both v2 tests assert Buffer.from(tx.signature, 'base64').length === 97
decryptKeychainPrivateKey used for MPCv1 backup key ✅ Replaces bespoke try/catch
assert() guards for userKey / backupKey / walletPassphrase ✅ Three assert() calls at top of signRecoveryTransaction
assert() guard for bitgoKey ❌ params.bitgoKey is consumed immediately as params.bitgoKey.replace(/\s/g, '') with no prior guard — will throw an opaque TypeError if undefined, not a clean assertion error
Unsigned sweep path untouched ✅ createBroadcastableSweepTransaction() not in diff
recoverSuiToken also dispatches MPCv2 ✅ Both paths call signRecoveryTransaction(); dedicated token MPCv2 test confirms routing

Test Quality

Check Result Notes
v2 native path: getTSSSignature NOT called ✅ sandBox.assert.notCalled(getTSSSignatureSpy)
v2 native path: 97-byte SUI envelope verified ✅ Buffer.from(tx.signature, 'base64').length.should.equal(97)
v2 token path: getTSSSignature NOT called ✅ Same pattern
v2 token path: 97-byte SUI envelope verified ✅ Same assertion
v1 path: getTSSSignature WAS called ✅ sandBox.assert.calledOnce(getTSSSignatureStub)
Crypto verification using nacl.sign.detached.verify ❌ No cryptographic signature verification in the test suite — the v2 native and token tests only check envelope size, not that the signature is actually valid against the public key
Mismatched bitgoKey vs commonKeyChain error test ✅ should.be.rejectedWith('EdDSA MPCv2 recovery: commonKeyChain from keycard does not match bitgoKey')
Guard tests for missing params (new describe blocks) ❌ No tests for assert(params.userKey) / assert(params.backupKey) / assert(params.walletPassphrase) in the new MPCv2 describe blocks (these exist implicitly elsewhere but are not exercised for the new path)

Consistency vs DOT (#9484)

Check Result Notes
describe block naming consistent ✅ 'MPCv2 signed recovery' matches expected pattern
Sinon stubs use as unknown as { method: unknown } (not as any) ✅ Confirmed in test diff

Summary of ❌ Findings

  1. bitgoKey missing assert() guard (sui.ts): params.bitgoKey.replace(/\s/g, '') is called without a preceding assert(params.bitgoKey, ...). Add assert(params.bitgoKey, 'missing bitgoKey') alongside the other three guards.

  2. No nacl.sign.detached.verify test: The v2 signing path is only validated by envelope size, not cryptographic correctness. The TON/SOL canonical suite includes a nacl.sign.detached.verify check — consider adding one to the native-transfer v2 test to close the gap.

  3. No guard tests for missing params in new describe blocks: The assert() guards for userKey/backupKey/walletPassphrase are not exercised in the new MPCv2 describe blocks. Consider adding negative tests (or verifying they're covered in the existing suite above).

Generated by [wallet-platform:review-pr] via Claude Code

@vibhavgo
test(sdk-coin-sui): cover MPCv2 recovery for token transfers
0349d5d 
Add an MPCv2-signed recovery test under the token recovery describe
block, mirroring the native-transfer MPCv2 test added in the prior
commit. Asserts getTSSSignature is not called and the resulting
signature is wrapped in SUI's 0x00-flag envelope.

The prior commit wired signRecoveryTransaction()'s MPCv2 dispatch
into both recover() and recoverSuiToken(), but only exercised the
native-transfer path in tests. The ticket explicitly calls out that
token recovery (getTokenTransferBuilder) must also handle MPCv2, so
this closes that coverage gap found during review.

Ticket: WCI-1224
Session-Id: 0c067bc3-368d-4c82-9e68-8d08c89766d5
Task-Id: 8c5b0392-4a96-4479-a83b-8ba80cd2428d
@vibhavgo
vibhavgo force-pushed the wci-1224-sui-mpcv2-signed-hot-recovery branch from 4994308 to 0349d5d Compare August 14, 2026 07:07
@vibhavgo
vibhavgo requested a review from Marzooqa August 14, 2026 08:10
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@Marzooqa Marzooqa Awaiting requested review from Marzooqa

At least 1 approving review is required to merge this pull request.

Assignees

@vibhavgo vibhavgo

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

1 participant

@vibhavgo