-
Notifications
You must be signed in to change notification settings - Fork 300
fix: address validation for SMC wallets #7960
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Conversation
b4a37c1 to
b7d2cf4
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Pull request overview
This pull request fixes address validation for Self-Managed Custodial (SMC) TSS wallets on EdDSA-based coins like Solana. The issue occurred because client-side verification used m/{index} derivation paths while wallet derivation used m/{prefix}/{index}, causing a mismatch.
Changes:
- Extracts derivation prefix from User keychain's
derivedFromParentWithSeedfor cold TSS wallets - Adds optional
derivationPrefixparameter to address verification interfaces - Updates derivation path construction to use prefix when available
- Adds comprehensive unit tests for prefix extraction and validation scenarios
- Excludes a tar security advisory that doesn't affect the project's usage pattern
Reviewed changes
Copilot reviewed 7 out of 7 changed files in this pull request and generated 3 comments.
Show a summary per file
| File | Description |
|---|---|
| modules/sdk-core/src/bitgo/wallet/wallet.ts | Extracts derivation prefix from User keychain for SMC TSS wallets and passes it to address verification |
| modules/sdk-core/src/bitgo/baseCoin/iBaseCoin.ts | Adds optional derivationPrefix field to TssVerifyAddressOptions and VerifyAddressOptions interfaces with documentation |
| modules/sdk-core/src/bitgo/utils/tss/addressVerification.ts | Updates derivation path construction to use {derivationPrefix}/{index} when prefix is present, removes circular dependency with lazy import |
| modules/sdk-core/test/unit/bitgo/wallet/walletTssAddressVerification.ts | Adds comprehensive unit tests for custodial, cold/SMC, and non-TSS wallet scenarios with edge cases |
| modules/sdk-core/test/unit/bitgo/utils/tss/addressVerification.ts | Adds unit tests for extractCommonKeychain function and derivation path format validation |
| modules/sdk-coin-sol/test/unit/sol.ts | Adds integration test verifying address validation with derivation prefix for Solana SMC wallets |
| .iyarc | Excludes tar vulnerability GHSA-r6q2-hw4h-h46w that only affects extraction, not the project's packing usage |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
b7d2cf4 to
cf10521
Compare
…wallet address verification TICKET: WP-7507
cf10521 to
2730531
Compare
|
Changes the commit |
TICKET: WP-7507
Problem
Address validation failed for Self-Managed Custodial (SMC) TSS wallets on EdDSA-based coins (e.g., Solana). The client-side verification used m/{index} while the wallet derivation used m/{prefix}/{index}, causing a mismatch.
Fix
Extract the derivation prefix from the User keychain's derivedFromParentWithSeed for SMC (cold) TSS wallets
Pass the prefix to address verification to construct the correct derivation path
Use m/{prefix}/{index} when a prefix is present, otherwise m/{index}
Changes
Testing
Impact