feat(root): enable passing apiKey for recovery on eth likes #6511
Conversation
This comment was marked as outdated.
This comment was marked as outdated.
Sorry, something went wrong.
mohammadalfaiyazbitgo
dismissed stale reviews from pranavjain97 and margueriteblair
via
b4b8112
mohammadalfaiyazbitgo
force-pushed
the
WP-000000/refactor-ethlikes-to-use-apiKey
branch
from
a575e29 to
b4b8112
Compare
mohammadalfaiyazbitgo
requested review from
margueriteblair,
pranavjain97 and
Copilot
This comment was marked as outdated.
This comment was marked as outdated.
Sorry, something went wrong.
- also added example for how to generate an unsigned tx Ticket: WP-5348
mohammadalfaiyazbitgo
dismissed stale reviews from pranavjain97 and margueriteblair
via
dc8d3c2
mohammadalfaiyazbitgo
force-pushed
the
WP-000000/refactor-ethlikes-to-use-apiKey
branch
from
b4b8112 to
dc8d3c2
Compare
mohammadalfaiyazbitgo
requested review from
Taseen08,
sasikumar-bitgo,
Copilot and
margueriteblair
There was a problem hiding this comment.
Pull Request Overview
This PR enables passing a custom API key for recovery operations on Ethereum-like blockchain networks instead of relying solely on environment-configured API keys. The changes provide more flexibility for recovery operations by allowing users to specify their own blockchain explorer API keys.
- Adds optional
apiKeyparameter torecoveryBlockchainExplorerQuerymethods across all ETH-like coin implementations - Updates recovery logic to propagate the custom API key through all blockchain explorer queries
- Adds comprehensive test coverage to verify custom API key usage during recovery operations
Reviewed Changes
Copilot reviewed 20 out of 20 changed files in this pull request and generated 3 comments.
Show a summary per file
| File | Description |
|---|---|
| modules/abstract-eth/src/abstractEthLikeNewCoins.ts | Core implementation adding apiKey parameter to recovery methods and explorer queries |
| modules/sdk-coin-eth/src/eth.ts | ETH coin implementation with apiKey support and updated recovery logic |
| modules/sdk-coin-eth/src/erc20Token.ts | ERC20 token recovery with apiKey propagation |
| modules/sdk-coin-/src/.ts | Individual coin implementations adding apiKey parameter to recoveryBlockchainExplorerQuery |
| modules/bitgo/test/v2/unit/recovery.ts | Comprehensive test coverage for API key usage in recovery |
| modules/sdk-coin-eth/test/unit/eth.ts | Unit tests verifying API key override functionality |
Comments suppressed due to low confidence (1)
modules/sdk-coin-eth/test/unit/eth.ts:1076
- The test mocks the global
request.getfunction which could affect other tests if not properly isolated. Consider using a more targeted mocking approach like sinon.stub to avoid potential test interference.
request.get = function (url: string) {
| query: Record<string, string>, | ||
| apiKey?: string | ||
| ): Promise<Record<string, unknown>> { | ||
| const apiToken = apiKey ?? common.Environments[this.bitgo.getEnv()][this.getFamily().toLowerCase() + 'ApiToken']; |
There was a problem hiding this comment.
[nitpick] Use consistent operator choice with other implementations. Other files use || operator for the same pattern, but this file uses ??. Consider using || for consistency across the codebase.
| const apiToken = apiKey ?? common.Environments[this.bitgo.getEnv()][this.getFamily().toLowerCase() + 'ApiToken']; | |
| const apiToken = apiKey || common.Environments[this.bitgo.getEnv()][this.getFamily().toLowerCase() + 'ApiToken']; |
Copilot uses AI. Check for mistakes.
| if (params.userKey === undefined) { | ||
| throw new Error('missing userKey'); | ||
| } | ||
|
|
||
| if (_.isUndefined(params.backupKey)) { | ||
| if (params.backupKey === undefined) { |
There was a problem hiding this comment.
The change from _.isUndefined() to strict equality checks removes the lodash dependency but may have different behavior for null values. Consider using == null to check for both undefined and null, or ensure this change is intentional.
Copilot uses AI. Check for mistakes.
| throw new Error('missing backupKey'); | ||
| } | ||
|
|
||
| if (_.isUndefined(params.walletPassphrase) && !params.userKey.startsWith('xpub') && !params.isTss) { | ||
| if ( | ||
| !params.isUnsignedSweep && |
There was a problem hiding this comment.
The validation logic now includes !params.isUnsignedSweep condition but this parameter is optional and may be undefined. Consider using params.isUnsignedSweep !== true to be more explicit about falsy values.
| !params.isUnsignedSweep && | |
| params.isUnsignedSweep !== true && |
Copilot uses AI. Check for mistakes.
| walletContractAddress: 'Address-of-your-multisig-wallet', | ||
| recoveryDestination: 'Address-To-Recover-Funds-To', | ||
| isUnsignedSweep: true, | ||
| apiKey: 'Add Your Etherscan ApiKey here', |
There was a problem hiding this comment.
may I know why are we adding apikey here instead of doingnew BitGoAPI({ env: environment, etherscanApiToken: apiKey });
There was a problem hiding this comment.
The motivation is to make recoveries have a similar interface across coins. If you look at UTXO recoveries,
they expose an API key param as well. Which to me makes sense as you only need the api key at time of recovery, which may not be when the bitgo object is initiated, so this flow is more flexible when it comes to usage.
There was a problem hiding this comment.
This is relevant for applications like WRW
Ticket: WP-5348