Skip to content

feat(sdk-coin-polyx): add MPCv2 signed hot recovery - #9483

Open
ralph-bitgo[bot] wants to merge 1 commit into
masterfrom
WCI-1228-sdk-coin-polyx-mpcv2-signed-hot-recovery
Open

feat(sdk-coin-polyx): add MPCv2 signed hot recovery#9483
ralph-bitgo[bot] wants to merge 1 commit into
masterfrom
WCI-1228-sdk-coin-polyx-mpcv2-signed-hot-recovery

Conversation

@ralph-bitgo

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

Copy link
Copy Markdown

What

  • Add isMpcv2SigningMaterial() and addRecoverySignature() private methods to Polyx in modules/sdk-coin-polyx/src/polyx.ts.
  • Polyx.recover() now detects once whether the keycard is MPCv1 (JSON) or MPCv2 (CBOR) and dispatches signing accordingly, reusing the shared getEddsaSigningMaterial/signEddsaMpcV2RecoveryTx helpers from @bitgo/sdk-core (extracted in WCI-1276). No new params — detection is automatic, and the existing MPCv1 / unsigned-sweep behavior is unchanged.
  • Add unit tests: MPCv2 happy path, MPCv1 regression (still works when signing material is MPCv1 format), mismatched bitgoKey vs keycard commonKeyChain throws, and guard-clause/error-path coverage for missing userKey/backupKey/walletPassphrase and decrypt failures.

Why

POLYX wallets are migrating from the Zengo to the Silence Labs EdDSA MPCv2 implementation, but Polyx.recover() only understood the legacy MPCv1 JSON keycard format — customers with MPCv2 keycards could not perform hot wallet recovery. This mirrors the MPCv2 recovery support already shipped for SOL (WCI-398) and TON (WCI-1225), reusing the shared sdk-core helpers instead of duplicating the signing logic, per WCI-1228.

Test plan

  • yarn unit-test --scope @bitgo/sdk-coin-polyx — 259 passing, 2 pending, 0 failing
  • nix develop . --command npx lerna run build --scope @bitgo/sdk-coin-polyx --include-dependencies — builds clean
  • eslint on changed files — no errors

Ticket: WCI-1228

@linear-code

linear-code Bot commented Aug 12, 2026

Copy link
Copy Markdown
Contributor

WCI-1228

@ralph-bitgo
ralph-bitgo Bot force-pushed the WCI-1228-sdk-coin-polyx-mpcv2-signed-hot-recovery branch from 2f801bc to dc26b6c Compare August 12, 2026 11:29
@vibhavgo
vibhavgo force-pushed the WCI-1228-sdk-coin-polyx-mpcv2-signed-hot-recovery branch 3 times, most recently from 16e2240 to edbf701 Compare August 13, 2026 06:42
@vibhavgo
vibhavgo marked this pull request as ready for review August 13, 2026 07:08
@vibhavgo
vibhavgo requested a review from a team as a code owner August 13, 2026 07:08
@vibhavgo
vibhavgo force-pushed the WCI-1228-sdk-coin-polyx-mpcv2-signed-hot-recovery branch from edbf701 to 88f5e93 Compare August 13, 2026 14:44
@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

Polyx.recover() previously only supported MPCv1 (JSON keycard) signing. This PR adds auto-detection of MPCv1 vs MPCv2 (CBOR) signing material and dispatches accordingly, reusing the getEddsaSigningMaterial / signSubstrateMpcV2Recovery helpers from sdk-core. No API surface changes; unsigned-sweep path is untouched.

Changed files: 2 (+154 / -44)

  • modules/sdk-coin-polyx/src/polyx.ts
  • modules/sdk-coin-polyx/test/unit/polyx.ts
flowchart TD
    A["recover()"] --> B{isUnsignedSweep?}
    B -- yes --> C[return unsigned tx]
    B -- no --> D[assert userKey / backupKey / walletPassphrase]
    D --> E["getEddsaSigningMaterial(userKey, passphrase)"]
    E --> F{signingMaterial.version}
    F -- v2 --> G["signSubstrateMpcV2Recovery()"]
    G --> H["addSignature([0x00] + rawSig)"]
    F -- v1 --> I["bitgo.decrypt(backupKey)"]
    I --> J["EDDSAMethods.getTSSSignature()"]
    J --> K["addSignature(signatureHex)"]
    H --> L["txBuilder.build()"]
    K --> L
Loading

Phase 2 — Ticket Alignment

Ticket: WCI-1228

PR title (feat(sdk-coin-polyx): add MPCv2 signed hot recovery) and body are consistent with the ticket scope. The PR correctly notes it mirrors WCI-398 (SOL) and WCI-1225 (TON) and reuses the shared helpers extracted in WCI-1276. Alignment: ✅

Phase 3 — Index Analysis

No new database queries introduced.

Phase 4 — Type Safety

Scanned all added lines (+) for unsafe patterns:

Pattern Found?
as any ❌ None
!. (non-null assertion) ❌ None
@ts-ignore ❌ None
Double cast (e.g. as Foo as Bar without unknown bridge) ❌ None

Test stub uses the correct safe pattern: baseCoin as unknown as { getEddsaSigningMaterial: unknown }. ✅

Phase 5 — MPCv2 Correctness & Test Quality

Correctness

Check Result
getEddsaSigningMaterial called with whitespace-stripped userKey? ⚠️ Potential issue — params.userKey is passed without .replace(/\s/g, ''). The old code stripped it explicitly. If the shared helper does not strip internally, keycards with whitespace (e.g. line-wrapped PEM-style exports) will fail to decrypt. Verify that getEddsaSigningMaterial in sdk-core strips whitespace before decryption.
POLYX-specific: 0x00 Substrate Ed25519 discriminant manually prepended before addSignature? ✅ Buffer.concat([Buffer.from([ED25519_PREFIX]), rawSig]) where ED25519_PREFIX = 0x00
MPCv1 path backup key decryption uses bitgo.decrypt directly? ✅ this.bitgo.decrypt({ input: params.backupKey.replace(/\s/g, ''), password: params.walletPassphrase })
assert() guards used for userKey / backupKey / walletPassphrase? ✅ All three replaced with assert()
Unsigned sweep path untouched? ✅

Test Quality

Check Result
v2 path test asserts getTSSSignature NOT called? ✅ sandBox.assert.notCalled(getTSSSignatureSpy)
v2 path test verifies signature is 65 bytes with [0] === 0x00? ✅ signature.length.should.equal(65); signature[0].should.equal(0x00)
v1 path test asserts getTSSSignature WAS called? ✅ sandBox.assert.calledOnce(getTSSSignatureStub)
Crypto verification uses nacl.sign.detached.verify (not just length check)? ❌ Missing — the MPCv2 happy path test only checks byte length and discriminant. There is no cryptographic verification that the produced signature is valid against the account's public key. Consider adding nacl.sign.detached.verify(payload, sig.slice(1), pubKeyBytes) similar to the DOT test pattern.
Mismatched bitgoKey vs commonKeyChain error test? ✅ Present
Guard tests for missing userKey / backupKey / walletPassphrase? ✅ All three present

Consistency vs DOT (PR #9484 as canonical reference)

Check Result
Describe block naming consistent? ⚠️ Unable to verify without DOT diff — but 'MPCv2 signed recovery' is a reasonable name; confirm it matches DOT's describe label.
Sinon stubs use as unknown as { method: unknown } (not as any)? ✅ Correct pattern used

Summary of ❌ / ⚠️ Items

  1. ⚠️ Whitespace stripping on userKey before getEddsaSigningMaterial — confirm the sdk-core helper strips whitespace internally; if not, this is a latent bug for keycards with embedded newlines.
  2. ❌ No nacl.sign.detached.verify in MPCv2 happy-path test — the test validates structure (length, discriminant) but not cryptographic correctness. Adding a real signature verification step would significantly strengthen confidence in the signing path.

Otherwise the implementation looks correct and well-structured. The 0x00 prefix handling, assert() guard replacement, MPCv1 regression test, and mismatch error test are all solid.

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

@vibhavgo
vibhavgo force-pushed the WCI-1228-sdk-coin-polyx-mpcv2-signed-hot-recovery branch from 88f5e93 to ce5c0c3 Compare August 14, 2026 07:07
@vibhavgo
feat(sdk-coin-polyx): add MPCv2 signed hot recovery
fb53e52 
Detect CBOR (MPCv2) vs JSON (MPCv1) keycards in Polyx.recover() using
the shared getEddsaSigningMaterial/signSubstrateMpcV2Recovery helpers
from SubstrateCoin. Fixes missing 0x00 Ed25519 discriminant prefix on
MPCv2 signatures. Uses assert() for guards in newly introduced recovery
code.

Ticket: WCI-1228
@vibhavgo
vibhavgo force-pushed the WCI-1228-sdk-coin-polyx-mpcv2-signed-hot-recovery branch from ce5c0c3 to fb53e52 Compare August 14, 2026 07:21
@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