-
Notifications
You must be signed in to change notification settings - Fork 296
feat(abstract-eth): add recover consolidation for eth #6550
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?
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change | ||||
---|---|---|---|---|---|---|
|
@@ -19,6 +19,7 @@ import { | |||||
IWallet, | ||||||
KeyPair, | ||||||
MPCSweepRecoveryOptions, | ||||||
MPCSweepTxs, | ||||||
MPCTx, | ||||||
MPCTxs, | ||||||
ParsedTransaction, | ||||||
|
@@ -57,7 +58,7 @@ import { BigNumber } from 'bignumber.js'; | |||||
import BN from 'bn.js'; | ||||||
import { randomBytes } from 'crypto'; | ||||||
import debugLib from 'debug'; | ||||||
import { addHexPrefix, bufArrToArr, stripHexPrefix } from 'ethereumjs-util'; | ||||||
import { addHexPrefix, bufArrToArr, stripHexPrefix, bufferToHex, setLengthLeft, toBuffer } from 'ethereumjs-util'; | ||||||
import Keccak from 'keccak'; | ||||||
import _ from 'lodash'; | ||||||
import secp256k1 from 'secp256k1'; | ||||||
|
@@ -70,6 +71,7 @@ import { | |||||
ERC721TransferBuilder, | ||||||
getBufferedByteCode, | ||||||
getCommon, | ||||||
getCreateForwarderParamsAndTypes, | ||||||
getProxyInitcode, | ||||||
getRawDecoded, | ||||||
getToken, | ||||||
|
@@ -224,6 +226,11 @@ export type UnsignedSweepTxMPCv2 = { | |||||
}[]; | ||||||
}; | ||||||
|
||||||
export type UnsignedBuilConsolidation = { | ||||||
transactions: MPCSweepTxs[] | UnsignedSweepTxMPCv2[] | RecoveryInfo[] | OfflineVaultTxInfo[]; | ||||||
lastScanIndex: number; | ||||||
}; | ||||||
|
||||||
export type RecoverOptionsWithBytes = { | ||||||
isTss: true; | ||||||
/** | ||||||
|
@@ -361,6 +368,33 @@ interface EthAddressCoinSpecifics extends AddressCoinSpecific { | |||||
salt?: string; | ||||||
} | ||||||
|
||||||
export const DEFAULT_SCAN_FACTOR = 20; | ||||||
export interface EthConsolidationRecoveryOptions { | ||||||
coinName?: string; | ||||||
walletContractAddress?: string; | ||||||
apiKey?: string; | ||||||
isTss?: boolean; | ||||||
userKey?: string; | ||||||
backupKey?: string; | ||||||
walletPassphrase?: string; | ||||||
recoveryDestination?: string; | ||||||
krsProvider?: string; | ||||||
gasPrice?: number; | ||||||
gasLimit?: number; | ||||||
eip1559?: EIP1559; | ||||||
replayProtectionOptions?: ReplayProtectionOptions; | ||||||
bitgoFeeAddress?: string; | ||||||
bitgoDestinationAddress?: string; | ||||||
tokenContractAddress?: string; | ||||||
intendedChain?: string; | ||||||
common?: EthLikeCommon.default; | ||||||
derivationSeed?: string; | ||||||
bitgoKey?: string; | ||||||
startingScanIndex?: number; | ||||||
endingScanIndex?: number; | ||||||
ignoreAddressTypes?: unknown; | ||||||
} | ||||||
|
||||||
export interface VerifyEthAddressOptions extends BaseVerifyAddressOptions { | ||||||
baseAddress: string; | ||||||
coinSpecific: EthAddressCoinSpecifics; | ||||||
|
@@ -1192,6 +1226,161 @@ export abstract class AbstractEthLikeNewCoins extends AbstractEthLikeCoin { | |||||
return this.recoverEthLike(params); | ||||||
} | ||||||
|
||||||
generateForwarderAddress( | ||||||
baseAddress: string, | ||||||
feeAddress: string, | ||||||
forwarderFactoryAddress: string, | ||||||
forwarderImplementationAddress: string, | ||||||
index: number | ||||||
): string { | ||||||
const salt = addHexPrefix(index.toString(16)); | ||||||
const saltBuffer = setLengthLeft(toBuffer(salt), 32); | ||||||
|
||||||
const { createForwarderParams, createForwarderTypes } = getCreateForwarderParamsAndTypes( | ||||||
baseAddress, | ||||||
saltBuffer, | ||||||
feeAddress | ||||||
); | ||||||
|
||||||
const calculationSalt = bufferToHex(optionalDeps.ethAbi.soliditySHA3(createForwarderTypes, createForwarderParams)); | ||||||
|
||||||
const initCode = getProxyInitcode(forwarderImplementationAddress); | ||||||
return calculateForwarderV1Address(forwarderFactoryAddress, calculationSalt, initCode); | ||||||
} | ||||||
|
||||||
deriveAddressFromPublicKey(commonKeychain: string, index: number): string { | ||||||
const derivationPath = `m/${index}`; | ||||||
const pubkeySize = 33; | ||||||
|
||||||
const ecdsaMpc = new Ecdsa(); | ||||||
const derivedPublicKey = Buffer.from(ecdsaMpc.deriveUnhardened(commonKeychain, derivationPath), 'hex') | ||||||
.subarray(0, pubkeySize) | ||||||
.toString('hex'); | ||||||
|
||||||
const publicKey = Buffer.from(derivedPublicKey, 'hex').slice(0, 66).toString('hex'); | ||||||
|
||||||
const keyPair = new KeyPairLib({ pub: publicKey }); | ||||||
const address = keyPair.getAddress(); | ||||||
return address; | ||||||
} | ||||||
|
||||||
getConsolidationAddress(params: EthConsolidationRecoveryOptions, index: number): string[] { | ||||||
const possibleConsolidationAddresses: string[] = []; | ||||||
if (params.walletContractAddress && params.bitgoFeeAddress) { | ||||||
const ethNetwork = this.getNetwork(); | ||||||
const forwarderFactoryAddress = ethNetwork?.walletV4ForwarderFactoryAddress as string; | ||||||
const forwarderImplementationAddress = ethNetwork?.walletV4ForwarderImplementationAddress as string; | ||||||
try { | ||||||
const forwarderAddress = this.generateForwarderAddress( | ||||||
params.walletContractAddress, | ||||||
params.bitgoFeeAddress, | ||||||
forwarderFactoryAddress, | ||||||
forwarderImplementationAddress, | ||||||
index | ||||||
); | ||||||
possibleConsolidationAddresses.push(forwarderAddress); | ||||||
} catch (e) { | ||||||
console.log(`Failed to generate forwarder address: ${e.message}`); | ||||||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Using console.log for error logging is not appropriate for production code. Consider using a proper logging framework or throwing/handling the error appropriately.
Suggested change
Copilot uses AI. Check for mistakes. Positive FeedbackNegative Feedback |
||||||
} | ||||||
} | ||||||
|
||||||
if (params.userKey) { | ||||||
try { | ||||||
const derivedAddress = this.deriveAddressFromPublicKey(params.userKey, index); | ||||||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. derivedAddress is returned for every There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. We don't have any means to differentiate between v6 and v5 wallet thus we try to derive address every time. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. in that case this would execute for v6 wallet as well right ? |
||||||
possibleConsolidationAddresses.push(derivedAddress); | ||||||
} catch (e) { | ||||||
console.log(`Failed to generate derived address: ${e}`); | ||||||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Using console.log for error logging is not appropriate for production code. Consider using a proper logging framework or throwing/handling the error appropriately.
Suggested change
Copilot uses AI. Check for mistakes. Positive FeedbackNegative Feedback |
||||||
} | ||||||
} | ||||||
|
||||||
if (possibleConsolidationAddresses.length === 0) { | ||||||
throw new Error( | ||||||
'Unable to generate consolidation address. Check that wallet contract address, fee address, or user key is valid.' | ||||||
); | ||||||
} | ||||||
return possibleConsolidationAddresses; | ||||||
} | ||||||
|
||||||
async recoverConsolidations(params: EthConsolidationRecoveryOptions): Promise<UnsignedBuilConsolidation> { | ||||||
const isUnsignedSweep = !params.userKey && !params.backupKey && !params.walletPassphrase; | ||||||
const startIdx = params.startingScanIndex || 1; | ||||||
const endIdx = params.endingScanIndex || startIdx + DEFAULT_SCAN_FACTOR; | ||||||
|
||||||
if (!params.walletContractAddress || params.walletContractAddress === '') { | ||||||
throw new Error(`Invalid wallet contract address ${params.walletContractAddress}`); | ||||||
} | ||||||
|
||||||
if (!params.bitgoFeeAddress || params.bitgoFeeAddress === '') { | ||||||
throw new Error(`Invalid fee address ${params.bitgoFeeAddress}`); | ||||||
} | ||||||
|
||||||
if (startIdx < 1 || endIdx <= startIdx || endIdx - startIdx > 10 * DEFAULT_SCAN_FACTOR) { | ||||||
throw new Error( | ||||||
`Invalid starting or ending index to scan for addresses. startingScanIndex: ${startIdx}, endingScanIndex: ${endIdx}.` | ||||||
); | ||||||
} | ||||||
|
||||||
const consolidatedTransactions: any[] = []; | ||||||
let lastScanIndex = startIdx; | ||||||
|
||||||
for (let i = startIdx; i < endIdx; i++) { | ||||||
const consolidationAddress = this.getConsolidationAddress(params, i); | ||||||
for (const address of consolidationAddress) { | ||||||
const recoverParams = { | ||||||
apiKey: params.apiKey, | ||||||
backupKey: params.backupKey || '', | ||||||
gasLimit: params.gasLimit, | ||||||
recoveryDestination: params.recoveryDestination || '', | ||||||
userKey: params.userKey || '', | ||||||
walletContractAddress: address, | ||||||
derivationSeed: '', | ||||||
isTss: params.isTss, | ||||||
eip1559: { | ||||||
maxFeePerGas: params.eip1559?.maxFeePerGas || 20, | ||||||
maxPriorityFeePerGas: params.eip1559?.maxPriorityFeePerGas || 200000, | ||||||
}, | ||||||
replayProtectionOptions: { | ||||||
chain: params.replayProtectionOptions?.chain || 0, | ||||||
hardfork: params.replayProtectionOptions?.hardfork || 'london', | ||||||
}, | ||||||
bitgoKey: '', | ||||||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Setting bitgoKey to an empty string may be problematic. Consider using a proper default value or making this parameter optional if it's not required.
Suggested change
Copilot uses AI. Check for mistakes. Positive FeedbackNegative Feedback |
||||||
ignoreAddressTypes: [], | ||||||
}; | ||||||
let recoveryTransaction; | ||||||
try { | ||||||
recoveryTransaction = await this.recover(recoverParams); | ||||||
} catch (e) { | ||||||
if ( | ||||||
e.message === 'Did not find address with funds to recover' || | ||||||
e.message === 'Did not find token account to recover tokens, please check token account' || | ||||||
e.message === 'Not enough token funds to recover' | ||||||
) { | ||||||
lastScanIndex = i; | ||||||
continue; | ||||||
} | ||||||
throw e; | ||||||
} | ||||||
if (isUnsignedSweep) { | ||||||
consolidatedTransactions.push((recoveryTransaction as MPCSweepTxs).txRequests[0]); | ||||||
} else { | ||||||
consolidatedTransactions.push(recoveryTransaction); | ||||||
} | ||||||
} | ||||||
// To avoid rate limit for etherscan | ||||||
await new Promise((resolve) => setTimeout(resolve, 1000)); | ||||||
// lastScanIndex = i; | ||||||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. This commented out code should be removed or properly implemented if needed.
Suggested change
Copilot uses AI. Check for mistakes. Positive FeedbackNegative Feedback |
||||||
} | ||||||
|
||||||
if (consolidatedTransactions.length === 0) { | ||||||
throw new Error( | ||||||
`Did not find an address with sufficient funds to recover. Please start the next scan at address index ${ | ||||||
lastScanIndex + 1 | ||||||
}.` | ||||||
); | ||||||
} | ||||||
return { transactions: consolidatedTransactions, lastScanIndex }; | ||||||
} | ||||||
|
||||||
/** | ||||||
* Builds a funds recovery transaction without BitGo for non-TSS transaction | ||||||
* @param params | ||||||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -10,6 +10,7 @@ import { | |
generateRandomPassword, | ||
InvalidAddressError, | ||
InvalidAddressVerificationObjectPropertyError, | ||
MPCSweepTxs, | ||
TransactionType, | ||
UnexpectedAddressError, | ||
Wallet, | ||
|
@@ -22,6 +23,7 @@ import { | |
Teth, | ||
TransactionBuilder, | ||
TransferBuilder, | ||
UnsignedBuilConsolidation, | ||
UnsignedSweepTxMPCv2, | ||
} from '../../src'; | ||
import { EthereumNetwork } from '@bitgo/statics'; | ||
|
@@ -1053,6 +1055,113 @@ describe('ETH:', function () { | |
); | ||
}); | ||
}); | ||
|
||
describe('Build Unsigned Consolidation for Self-Custody Cold Wallets (MPCv2)', function () { | ||
const baseUrl = common.Environments.test.etherscanBaseUrl as string; | ||
let bitgo: TestBitGoAPI; | ||
let basecoin: Hteth; | ||
|
||
before(function () { | ||
bitgo = TestBitGo.decorate(BitGoAPI, { env: 'test' }); | ||
basecoin = bitgo.coin('hteth') as Hteth; | ||
}); | ||
|
||
it('should generate an unsigned consolidation', async function () { | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. add a test case for v5 and v6 separately There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. We don't need to write separate test cases since all the values are provided and based on the which wallet version we are assuming, we are using the relevant values and generating address for consolidation There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
yes, this will run in both the cases since we have no means of verifying which wallet version is this There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. why are you sure whether this would throw an error for V6 wallet? |
||
nock(baseUrl) | ||
.get('/api') | ||
.query(mockData.getTxListRequest(mockData.getBuildUnsignedSweepForSelfCustodyColdWalletsMPCv2().address)) | ||
.times(2) | ||
.reply(200, mockData.getTxListResponse); | ||
|
||
nock(baseUrl) | ||
.get('/api') | ||
.query(mockData.getBalanceRequest(mockData.getBuildUnsignedSweepForSelfCustodyColdWalletsMPCv2().address)) | ||
.times(2) | ||
.reply(200, mockData.getBalanceResponse); | ||
nock(baseUrl) | ||
.get('/api') | ||
.query( | ||
mockData.getBalanceRequest( | ||
mockData.getBuildUnsignedSweepForSelfCustodyColdWalletsMPCv2().walletContractAddress | ||
) | ||
) | ||
.reply(200, mockData.getBalanceResponse); | ||
|
||
nock(baseUrl).get('/api').query(mockData.getContractCallRequest).reply(200, mockData.getContractCallResponse); | ||
|
||
const params = mockData.getBuildUnsignedSweepForSelfCustodyColdWalletsMPCv2(); | ||
const consolidationResult = await (basecoin as AbstractEthLikeNewCoins).recoverConsolidations({ | ||
userKey: params.commonKeyChain, // Box A Data | ||
backupKey: params.commonKeyChain, // Box B Data | ||
derivationSeed: params.derivationSeed, // Key Derivation Seed (optional) | ||
recoveryDestination: params.recoveryDestination, // Destination Address | ||
gasLimit: 200000, // Gas Limit | ||
eip1559: { maxFeePerGas: 20000000000, maxPriorityFeePerGas: 10000000000 }, // Max Fee Per Gas and Max Priority Fee Per Gas | ||
walletContractAddress: params.walletContractAddress, | ||
isTss: true, | ||
replayProtectionOptions: { | ||
chain: '42', | ||
hardfork: 'london', | ||
}, | ||
bitgoFeeAddress: '0x33a42faea3c6e87021347e51700b48aaf49aa1e7', | ||
startingScanIndex: 1, | ||
endingScanIndex: 2, | ||
}); | ||
should.exist(consolidationResult); | ||
const unsignedBuilConsolidation = consolidationResult as UnsignedBuilConsolidation; | ||
unsignedBuilConsolidation.should.have.property('transactions'); | ||
unsignedBuilConsolidation.transactions.should.have.length(2); | ||
|
||
const output = unsignedBuilConsolidation.transactions[0] as MPCSweepTxs; | ||
output.should.have.property('txRequests'); | ||
output.txRequests[0].should.have.property('transactions'); | ||
output.txRequests[0].transactions.should.have.length(1); | ||
output.txRequests[0].should.have.property('walletCoin'); | ||
output.txRequests[0].transactions.should.have.length(1); | ||
output.txRequests[0].transactions[0].should.have.property('unsignedTx'); | ||
output.txRequests[0].transactions[0].unsignedTx.should.have.property('serializedTxHex'); | ||
output.txRequests[0].transactions[0].unsignedTx.should.have.property('signableHex'); | ||
output.txRequests[0].transactions[0].unsignedTx.should.have.property('derivationPath'); | ||
output.txRequests[0].transactions[0].unsignedTx.should.have.property('feeInfo'); | ||
output.txRequests[0].transactions[0].unsignedTx.should.have.property('parsedTx'); | ||
const parsedTx = output.txRequests[0].transactions[0].unsignedTx.parsedTx as { spendAmount: string }; | ||
parsedTx.should.have.property('spendAmount'); | ||
(output.txRequests[0].transactions[0].unsignedTx.parsedTx as { outputs: any[] }).should.have.property( | ||
'outputs' | ||
); | ||
}); | ||
|
||
it('should throw an error for invalid address', async function () { | ||
const params = mockData.getBuildUnsignedSweepForSelfCustodyColdWalletsMPCv2(); | ||
params.recoveryDestination = 'invalidAddress'; | ||
|
||
// Ensure userKey and backupKey are the same | ||
params.userKey = | ||
'0234eb39b22fed523ece7c78da29ba1f1de5b64a6e48013e0914de793bc1df0570e779de04758732734d97e54b782c8b336283811af6a2c57bd81438798e1c2446'; | ||
params.backupKey = | ||
'0234eb39b22fed523ece7c78da29ba1f1de5b64a6e48013e0914de793bc1df0570e779de04758732734d97e54b782c8b336283811af6a2c57bd81438798e1c2446'; | ||
|
||
await assert.rejects( | ||
async () => { | ||
await (basecoin as AbstractEthLikeNewCoins).recover({ | ||
recoveryDestination: params.recoveryDestination, // Destination Address | ||
gasLimit: 2000, // Gas Limit | ||
eip1559: { maxFeePerGas: 200, maxPriorityFeePerGas: 10000 }, // Max Fee Per Gas and Max Priority Fee Per Gas | ||
userKey: params.userKey, // Provide the userKey | ||
backupKey: params.backupKey, // Provide the backupKey | ||
walletContractAddress: params.walletContractAddress, // Provide the walletContractAddress | ||
isTss: true, | ||
replayProtectionOptions: { | ||
chain: '42', | ||
hardfork: 'london', | ||
}, | ||
}); | ||
}, | ||
Error, | ||
'Error: invalid address' | ||
); | ||
}); | ||
}); | ||
}); | ||
|
||
describe('RecoveryBlockchainExplorerQuery', () => { | ||
|
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 is specific for v4 wallet, can you not make it generic ?
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.
Since we are not sure which version is being used, we have kept this in a try statement in order to handle the failure