feat(mbp): recovery consolidate from wallet address#52
feat(mbp): recovery consolidate from wallet address#52mohammadalfaiyazbitgo merged 7 commits intomasterfrom
Conversation
cpatino-intive
force-pushed
the
WP-5143/recovery-consolidate
branch
from
e45c55c to
f4f0859
Compare
cpatino-intive
requested review from
Copilot,
mohammadalfaiyazbitgo and
pranavjain97
There was a problem hiding this comment.
Pull Request Overview
Adds support for recovering consolidation sweeps from TRX and Solana wallet addresses via a new /recoveryConsolidations endpoint.
- Extend
coinUtilsto recognize Sol and TRX coins. - Define a new API spec, router entry, handler, and client interfaces for the recovery consolidations endpoint.
- Add end‐to‐end tests covering TRX and Solana consolidation recovery flows.
Reviewed Changes
Copilot reviewed 5 out of 5 changed files in this pull request and generated 4 comments.
Show a summary per file
| File | Description |
|---|---|
| src/shared/coinUtils.ts | Added isSolCoin and isTrxCoin helpers. |
| src/api/master/routers/masterApiSpec.ts | Declared new request/response types and route spec. |
| src/api/master/handlers/recoveryConsolidationsWallet.ts | Implemented handler logic for TRX/Sol consolidation recovery. |
| src/api/master/clients/enclavedExpressClient.ts | Extended RecoveryMultisigOptions to accept MPC transaction types. |
| src/tests/api/master/recoveryConsolidationsWallet.test.ts | Added tests for TRX and Solana consolidation recovery. |
Comments suppressed due to low confidence (1)
src/api/master/routers/masterApiSpec.ts:189
- Using
t.anyfor the signed transactions response is too permissive. Consider defining a more specific transaction schema to improve validation and client typing.
signedTxs: t.array(t.any), // Array of fully signed transactions
| ); | ||
|
|
||
| // 1. Build unsigned consolidations | ||
| if (isSolCoin(sdkCoin)) { |
There was a problem hiding this comment.
No need for having these if conditions. If the recoverConsolidations method is implemented for a coin, we should continue. If not, then we should throw an error saying it is not supported.
| txs = result.transactions; | ||
| } | ||
|
|
||
| console.log(txs); |
| const signedTxs = []; | ||
| try { | ||
| for (const tx of txs) { |
There was a problem hiding this comment.
why are there multiple unsigned sweeps?
There was a problem hiding this comment.
The return result from sdkCoin.recoverConsolidations is transaction array.
There was a problem hiding this comment.
I guess they are all txs from receive address -> base.
| userKey: userPub, | ||
| backupKey: backupPub, | ||
| bitgoKey, | ||
| durableNonces: req.decoded.durableNonces!, |
There was a problem hiding this comment.
instead of doing this; just do an if check to throw an error if coin is sol and req.decoded.durableNonces is undefined. But call sdkCoin.recoverConsolidations( only once
cpatino-intive
force-pushed
the
WP-5143/recovery-consolidate
branch
from
f679941 to
8aff26f
Compare
cpatino-intive
changed the base branch from
WP-4736/utxo-recoveries
to
WP-4793/utxo-accelerate
cpatino-intive
dismissed
pranavjain97’s stale review
The base branch was changed.
| const signedTxs = []; | ||
| try { | ||
| for (const tx of txs) { |
There was a problem hiding this comment.
I guess they are all txs from receive address -> base.
| const result = await (sdkCoin as any).recoverConsolidations({ | ||
| ...req.decoded, | ||
| userKey: userPub, | ||
| backupKey: backupPub, | ||
| bitgoKey, | ||
| durableNonces: req.decoded.durableNonces, | ||
| }); |
There was a problem hiding this comment.
don't cast to any, use the appropriate coin classes that support recoverConsolidations
| if (typeof (sdkCoin as any).recoverConsolidations !== 'function') { | ||
| throw new Error(`recoverConsolidations is not supported for coin: ${coin}`); | ||
| } |
There was a problem hiding this comment.
same here. create a type guard instead that validates sdkCoin supports recoverConsolidations and returns an extended BaseCoin type.
| durableNonces: t.union([ | ||
| t.undefined, | ||
| t.type({ | ||
| publicKeys: t.array(t.string), | ||
| secretKey: t.string, | ||
| }), | ||
| ]), |
There was a problem hiding this comment.
nit: can use optional
| startingScanIndex: t.union([t.undefined, t.number]), // Optional | ||
| endingScanIndex: t.union([t.undefined, t.number]), // Optional | ||
| seed: t.union([t.undefined, t.string]), // Optional (Sol) | ||
| tokenContractAddress: t.union([t.undefined, t.string]), // Optional (TRX/Sol) |
There was a problem hiding this comment.
can just use optional(t.x)
| userPub: t.string, | ||
| backupPub: t.string, | ||
| bitgoKey: t.string, |
There was a problem hiding this comment.
I don't think you need all 3 values, the pub for them all is going to be the same.
cpatino-intive
force-pushed
the
WP-5143/recovery-consolidate
branch
3 times, most recently
from
acaef72 to
5675ff4
Compare
mohammadalfaiyazbitgo
force-pushed
the
WP-5166-eddsa-recovery-v2
branch
3 times, most recently
from
ff6ecf8 to
88eac6b
Compare
cpatino-intive
force-pushed
the
WP-5143/recovery-consolidate
branch
from
5675ff4 to
41f7b16
Compare
cpatino-intive
force-pushed
the
WP-5143/recovery-consolidate
branch
from
75b87bc to
57775b3
Compare
| bitgoKey: bitgoPub, | ||
| }); | ||
|
|
||
| console.log(`Recovery consolidations result: ${JSON.stringify(result)}`); |
There was a problem hiding this comment.
don't think we need this.
recovery consolidate from wallet address
TASKS: WP-5143