-
Notifications
You must be signed in to change notification settings - Fork 295
feat(bsc): added support for mpcv2 in recovery #5957
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
base: master
Are you sure you want to change the base?
Conversation
9084c45
to
6f22a6d
Compare
274c78c
to
de7be29
Compare
action: 'balance', | ||
address: address, | ||
}); | ||
async queryAddressBalance(address: string, apiKey?: string): Promise<any> { |
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.
let's update the function documentation with this new param.
action: 'balance', | ||
address: address, | ||
}); | ||
async queryAddressBalance(address: string, apiKey?: string): Promise<any> { |
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.
We don't need to update this method signature.
Please refer the implementation of recoveryBlockchainExplorerQuery
in Coredoa, Oas, Flr (any other EVM coin) to see how we are fetching APIKey there.
@@ -837,15 +841,18 @@ export abstract class AbstractEthLikeNewCoins extends AbstractEthLikeCoin { | |||
* @param {string} address | |||
* @returns {Promise<number>} | |||
*/ | |||
async getAddressNonce(address: string): Promise<number> { | |||
async getAddressNonce(address: string, apiKey?: string): Promise<number> { |
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.
Same as above,
Please refer the implementation of recoveryBlockchainExplorerQuery
in Coredoa, Oas, Flr (any other EVM coin) to see how we are fetching APIKey there.
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.
+1
@@ -2198,7 +2205,7 @@ export abstract class AbstractEthLikeNewCoins extends AbstractEthLikeCoin { | |||
return txAmount; | |||
} | |||
|
|||
async recoveryBlockchainExplorerQuery(query: Record<string, string>): Promise<any> { | |||
async recoveryBlockchainExplorerQuery(query: Record<string, string>, apiKey?: string): Promise<any> { |
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.
Changing this function signature in abstract class will break recovery flow for all other coins.
const common = EthereumCommon.forCustomChain( | ||
'mainnet', | ||
{ | ||
name: 'bsc', | ||
networkId: networkId, | ||
chainId: txChainId, | ||
}, | ||
hardfork | ||
); |
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.
Please consider overriding getCustomChainCommon
method from abstract class instead of replicating the complete txn building logic here
apiKey?: string | ||
): Promise<Record<string, unknown>> { | ||
// Add delay before making API call to avoid rate limiting | ||
await this.delayBetweenApiCalls(); |
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.
This should be in the common implementation of - recoveryBlockchainExplorerQuery(query, explorerUrl as string, apiToken)
@@ -837,15 +841,18 @@ export abstract class AbstractEthLikeNewCoins extends AbstractEthLikeCoin { | |||
* @param {string} address | |||
* @returns {Promise<number>} | |||
*/ | |||
async getAddressNonce(address: string): Promise<number> { | |||
async getAddressNonce(address: string, apiKey?: string): Promise<number> { |
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.
+1
protected async buildUnsignedSweepTxnTSS(params: RecoverOptions): Promise<OfflineVaultTxInfo | UnsignedSweepTxMPCv2> { | ||
if (!params.replayProtectionOptions) { | ||
params.replayProtectionOptions = { | ||
chain: this.getChain().includes('t') ? '97' : '56', |
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.
get the chainId from config
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 PR adds support for MPCv2 in recovery operations, specifically targeting Binance Smart Chain and Polygon.
- Introduces new tests and mock data for MPCv2 recovery and unsigned sweep transactions.
- Updates BSC configurations, transaction builder methods, and recovery workflows to support TSS with MPCv2.
- Revises dependency versions and API query token naming for consistency.
Reviewed Changes
Copilot reviewed 9 out of 9 changed files in this pull request and generated 1 comment.
Show a summary per file
File | Description |
---|---|
modules/sdk-coin-polygon/test/unit/polygon.ts | Adds a new test for handling invalid user key in Polygon recovery. |
modules/sdk-coin-bsc/test/unit/bsc.ts | Introduces tests for MPCv2 recovery and sweep transactions; note an unexpected nesting in test structure. |
modules/sdk-coin-bsc/test/fixtures/ecdsa.ts | Provides new mock data functions for MPCv2 recovery and unsigned sweep operations. |
modules/sdk-coin-bsc/src/lib/transactionBuilder.ts | Adds an addSignature method to support signature handling in transactions. |
modules/sdk-coin-bsc/src/bsc.ts | Extends BSC-specific configurations and transaction-building logic to support MPCv2, including rate limiting and replay protection options. |
modules/sdk-coin-bsc/package.json | Updates dependencies including sdk-lib-mpc and ethereumjs/tx to required versions. |
modules/bitgo/test/v2/unit/recovery.ts | Adds direct nock setups for BSC recovery tests to simulate external API responses. |
modules/abstract-eth/src/lib/utils.ts | Refactors the explorer query token parameter from "apikey" to "apiKey". |
modules/abstract-eth/src/abstractEthLikeNewCoins.ts | Passes apiKey in recovery and nonce/query functions; adjusts tx building and ensures proper conversion of transaction hex. |
Comments suppressed due to low confidence (1)
modules/abstract-eth/src/abstractEthLikeNewCoins.ts:2027
- Ensure that 'addHexPrefix' is imported from the appropriate utility library to avoid potential runtime errors. If it's not imported, add the necessary import statement.
txBuilder.from(addHexPrefix(transaction.serializedTxHex) as string);
bsc.getBaseFactor().should.equal(1e18); | ||
bsc.supportsTss().should.equal(true); | ||
bsc.allowsAccountConsolidations().should.equal(true); | ||
after(function () { |
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.
The test suite nests 'describe' blocks inside the 'after' hook, which may prevent these tests from running. Consider moving these 'describe' blocks outside the 'after' hook to ensure proper test execution.
Copilot uses AI. Check for mistakes.
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.
Looks like more work is needed.
This PR is stale because it has been open 45 days with no activity. Remove stale label or comment or this will be closed in 10 days. |
TICKET: WIN-4618