Skip to content

feat(sdk-coin-icp): allow account consolidations, add V2 address tests - #9962

Closed
ralph-bitgo[bot] wants to merge 2 commits into
masterfrom
CECHO-2439-icp-v2-consolidation-address-tests
Closed

ralph-bitgo[bot] wants to merge 2 commits into
masterfrom
CECHO-2439-icp-v2-consolidation-address-tests

Conversation

@ralph-bitgo

@ralph-bitgo ralph-bitgo Bot commented Oct 14, 2026

Copy link
Copy Markdown

Description

@bitgo/sdk-coin-icp now overrides allowsAccountConsolidations() to return true, and the V2 (HD-derived / non-memoId) wallet contract is pinned with tests.

What changed

  • src/icp.ts: add Icp.allowsAccountConsolidations(): boolean returning true (was the false default inherited from BaseCoin). This is the only production change.
  • test/unit/icp.ts: derive the index-1 receive address from the existing commonKeychain fixture exactly the way deriveMPCWalletAddress does (secp256k1 m/1, first 33 bytes of the derived public key) and pin the V2 contract in the renamed V2 non-memoId wallets suite: the V2 root verifies at index 0, a derived address verifies at its own index and is rejected at any other index, and memo-suffixed addresses (root?memoId=5) are never accepted. Also add a verifyTransaction test for the derived-to-root consolidation prebuild shape that sendAccountConsolidation sends (sender = derived, single bare-root recipient), and assert allowsAccountConsolidations() === true.
  • test/unit/transactionBuilder/transactionBuilder.ts: bare sender-to-receiver round-trip with no memo(...): both sender ops and the receiver keep their addresses verbatim and metadata.memo is the DEFAULT_MEMO fill-in (0).
  • test/unit/transaction.ts: the memo-suffixed receiver 2b9b...dd7b?memoId=0 survives verbatim in operations[1] — builders never strip memo suffixes.

Why

sdk-core's wallet.buildAccountConsolidations, sendAccountConsolidation and sendAccountConsolidations throw "<coin> does not allow account consolidations." unless baseCoin.allowsAccountConsolidations() is true (modules/sdk-core/src/bitgo/wallet/wallet.ts). sdk-coin-icp never overrode the false default, so bitgo-retail's consolidate flow fails for ICP before any transaction is built — V2 consolidation from the UI cannot work. The platform enforces V2-only consolidation eligibility (CECHO-2443); this unblocks CECHO-2445.

The remaining changes are test-only: the V2 verification contract and the builder address pass-through were only covered at derivation index 0, so a regression in index handling or memo-suffix stripping would ship unnoticed ahead of the V2 rollout.

Issue Number

CECHO-2439

Type of change

  • Bug fix (non-breaking change which fixes an issue)
  • New feature (non-breaking change which adds functionality)
  • Breaking change (fix or feature that would cause existing functionality to not work as expected)
  • This change requires a documentation update

How Has This Been Tested?

  • cd modules/sdk-coin-icp && yarn unit-test — 105 passing (98 existing + 7 new). New-behavior proof: the same derived address verifies at index: 1 and is rejected at index: 0. Pass-through proof: bare addresses appear verbatim with metadata.memo === 0, and the ?memoId=0 recipient survives unchanged.
  • cd modules/sdk-coin-icp && yarn lint && yarn check-fmt — clean.
  • cd modules/sdk-coin-icp && yarn build-ts — compiles (the module tsconfig includes test/**/*).
  • cd modules/sdk-core && yarn unit-test — passes unchanged (no sdk-core edits).

Checklist:

  • My code follows the style guidelines of this project
  • I have performed a self-review of my own code
  • My code compiles correctly for both Node and Browser environments
  • I have commented my code, particularly in hard-to-understand areas
  • My commits follow Conventional Commits and I have properly described any BREAKING CHANGES
  • The ticket or github issue was included in the commit message as a reference
  • I have made corresponding changes to the documentation and on any new/updated functions and/or methods - jsdoc
  • I have added tests that prove my fix is effective or that my feature works
  • New and existing unit tests pass locally with my changes

Ticket: CECHO-2439

sdk-core's wallet.buildAccountConsolidations, sendAccountConsolidation
and sendAccountConsolidations reject every coin whose base coin does
not override allowsAccountConsolidations() to true. sdk-coin-icp
inherited the false default, so bitgo-retail's consolidate flow fails
for ICP before any transaction is built and V2 consolidation from the
UI cannot work.

Override the flag to true so receive-address -> root consolidations
can be initiated. The platform enforces V2-only consolidation
eligibility (CECHO-2443).

Ticket: CECHO-2439
Session-Id: 84ade726-5312-4794-ba4c-fa887b5aabc5
Task-Id: cabd6df4-ceda-4d9d-abc8-8b4945964ba0
The V2 (non-memoId) wallet contract and the builder address
pass-through were only covered at derivation index 0 with bare test
vectors, so a regression in index handling or memo-suffix stripping
could ship unnoticed ahead of the V2 rollout (CECHO-2445).

Derive the index-1 receive address from the existing commonKeychain
fixture exactly as deriveMPCWalletAddress does (secp256k1 m/1, first
33 bytes) and pin the V2 contract: a derived address verifies only at
its own index, and memo-suffixed addresses are never accepted. Also
pin that builders pass sender/receiver strings through verbatim (the
?memoId=0 recipient survives unchanged), that an omitted memo
defaults to 0, and that verifyTransaction accepts the derived->root
consolidation prebuild shape sent by sendAccountConsolidation.

Ticket: CECHO-2439
Session-Id: 84ade726-5312-4794-ba4c-fa887b5aabc5
Task-Id: cabd6df4-ceda-4d9d-abc8-8b4945964ba0
@linear-code

linear-code Bot commented Oct 14, 2026

Copy link
Copy Markdown
Contributor

CECHO-2439

@github-actions

Copy link
Copy Markdown
Contributor

⚠️ Unit tests are failing on Node 26.x (Current release line, non-blocking). This is not an LTS version yet, so it does not block merge, but it signals an incompatibility to fix before Node 26.x becomes LTS.

View run

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants