feat(sdk-core): EVM keyring wallet and address creation changes #6872
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
nvjsr
force-pushed
the
COIN-5193-EVMkeyring
branch
4 times, most recently
from
a15e458 to
e9ef8af
Compare
nvjsr
changed the title
nvjsr
force-pushed
the
COIN-5193-EVMkeyring
branch
from
e9ef8af to
fa9cf7a
Compare
nvjsr
commented
Sep 3, 2025
shivam-bitgo
reviewed
Sep 3, 2025
nvjsr
force-pushed
the
COIN-5193-EVMkeyring
branch
from
fa9cf7a to
94ad1b1
Compare
nvjsr
force-pushed
the
COIN-5193-EVMkeyring
branch
from
94ad1b1 to
645eb31
Compare
pritam-gembali
approved these changes
Sep 3, 2025
nvjsr
changed the title
shivam-bitgo
approved these changes
Sep 3, 2025
pranavjain97
requested changes
Sep 3, 2025
Comment on lines
+1349
to
+1353
| } else if (!_.isUndefined(referenceCoin)) { | ||
| // referenceCoin cannot be used without referenceAddress | ||
| throw new Error('referenceAddress is required when using referenceCoin for EVM keyring'); | ||
| } | ||
|
|
There was a problem hiding this comment.
Can you instead do a if(_.isUndefined(referenceCoin) && !_.isUndefined(referenceAddress)) { throw error }
That's more readable.
There was a problem hiding this comment.
Adding changes in followup PR for same
| } | ||
| } | ||
|
|
||
| // Validate EVM keyring params, referenceAddress is required and referenceCoin is optional for EVM keyring |
There was a problem hiding this comment.
if referenceCoin by itself isn't valid, it should't be an independent option. Let's instead have referenceAddressEvmKeyRing and have the address and coin as nested fields.
Comment on lines
+104
to
+118
| // Validate referenceWalletId parameter | ||
| if (params.referenceWalletId) { | ||
| if (!_.isString(params.referenceWalletId)) { | ||
| throw new Error('invalid referenceWalletId argument, expecting string'); | ||
| } | ||
| if (!this.baseCoin.isEVM()) { | ||
| throw new Error('referenceWalletId is only supported for EVM chains'); | ||
| } | ||
| } | ||
|
|
||
| // For wallets with referenceWalletId, skip multisig validation as configuration is inherited | ||
| if (params.referenceWalletId) { | ||
| // Skip all multisig validation - configuration will be inherited from reference wallet | ||
| } else if (params.type !== 'custodial') { | ||
| // Standard validation for non-custodial wallets without referenceWalletId |
There was a problem hiding this comment.
We shoudn't be having a referenceId as a top level param that is EVM based only. Can we atleast name it evmKeyRingReferenceWalletId? Coin-specific logic shoudn't be leaking in generic add wallet methods, can we create a coin-specific util in eth-lib that can take care of this validation?
pranavjain97
reviewed
Sep 3, 2025
Comment on lines
+341
to
+370
| if (isEVMWithReference) { | ||
| // For EVM keyring wallets, multisigType will be inferred from the reference wallet | ||
| // No need to explicitly validate TSS requirement here as it will be handled by bgms | ||
|
|
||
| // For EVM keyring wallets, we use the add method directly with referenceWalletId | ||
| // This bypasses the normal key generation process since keys are shared via keyring | ||
| const addWalletParams = { | ||
| label, | ||
| referenceWalletId, | ||
| }; | ||
|
|
||
| const newWallet = await this.bitgo.post(this.baseCoin.url('/wallet/add')).send(addWalletParams).result(); | ||
|
|
||
| // For EVM keyring wallets, we need to get the keychains from the reference wallet | ||
| const userKeychain = this.baseCoin.keychains().get({ id: newWallet.keys[KeyIndices.USER] }); | ||
| const backupKeychain = this.baseCoin.keychains().get({ id: newWallet.keys[KeyIndices.BACKUP] }); | ||
| const bitgoKeychain = this.baseCoin.keychains().get({ id: newWallet.keys[KeyIndices.BITGO] }); | ||
|
|
||
| const [userKey, backupKey, bitgoKey] = await Promise.all([userKeychain, backupKeychain, bitgoKeychain]); | ||
|
|
||
| const result: WalletWithKeychains = { | ||
| wallet: new Wallet(this.bitgo, this.baseCoin, newWallet), | ||
| userKeychain: userKey, | ||
| backupKeychain: backupKey, | ||
| bitgoKeychain: bitgoKey, | ||
| responseType: 'WalletWithKeychains', | ||
| }; | ||
|
|
||
| return result; | ||
| } |
There was a problem hiding this comment.
This should also be an EVM lib util i.e something like createEvmKeyRingWallet.
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
TICKET: COIN-5193