Feature: Implement KMS Wallet #510
Conversation
- Added KMS wallet support to handle Ethereum transactions securely using AWS Key Management Service. - Updated configuration to allow KMS key IDs and region settings. - Refactored AdminWallet and Web3Wallet to support KMS-based transaction signing. - Introduced KMSWallet class to encapsulate KMS operations. - Added tests for KMS transaction submission and wallet functionality.
- Updated KMSWallet and Web3Wallet to store and retrieve wallet addresses in a case-insensitive manner. - Removed unnecessary console log from KMSWallet during transaction signing. - Adjusted tests to ensure KMS wallet address normalization is correctly handled.
…just Jest configuration
…ated package versions in package.json and package-lock.json
src/server/blockchain/AdminWallet.js
Outdated
|
|
||
| const sendTx = this.mainnetWeb3.eth.sendSignedTransaction(signedTx) | ||
|
|
||
| return new Promise((res, rej) => { |
src/server/blockchain/KMSWallet.js
Outdated
| const region = this.getRegion(address) | ||
|
|
||
| try { | ||
| if (transaction.rpcUrl != undefined && transaction.rpcUrl.includes('localhost')) { |
There was a problem hiding this comment.
write a comment why you do this
There was a problem hiding this comment.
This logic checks whether the network supports EIP-1559. If it does not, it removes maxFeePerGas and falls back to using the legacy gasPrice.
I’ll refactor this to make the behavior clearer and more robust.
src/server/blockchain/Web3Wallet.js
Outdated
| this.networkId = networkId | ||
| this.numberOfAdminWalletAccounts = conf.privateKey ? 1 : conf.numberOfAdminWalletAccounts | ||
| // Determine number of accounts based on configuration | ||
| let kmsKeyIds = null |
There was a problem hiding this comment.
can we just read existing kmskeys from aws? maybe read keys by tags?
There was a problem hiding this comment.
I’ll tag the KMS key and select the key by tag, then use the matching KMS KeyIds for wallet
src/server/blockchain/Web3Wallet.js
Outdated
| if (conf.kmsKeyIds) { | ||
| kmsKeyIds = conf.kmsKeyIds | ||
| .split(',') | ||
| .map(k => k.trim()) | ||
| .filter(k => k) | ||
| } | ||
| if (kmsKeyIds && kmsKeyIds.length > 0) { | ||
| this.numberOfAdminWalletAccounts = kmsKeyIds.length |
There was a problem hiding this comment.
should be moved to initialize and keys should be fetched by group/tag
src/server/blockchain/Web3Wallet.js
Outdated
| this.wallets[normalizedAddress] = { address, kmsKeyId, isKMS: true } | ||
| } | ||
|
|
||
| getKMSKeyIds() { |
There was a problem hiding this comment.
should be fetched from aws by group/tag
There was a problem hiding this comment.
Sure, I will use kms tag(e.g. Environment=test) for fetching kms key ids
src/server/blockchain/Web3Wallet.js
Outdated
|
|
||
| getKMSKeyIds() { | ||
| // Support comma-separated list of keys | ||
| if (this.conf.kmsKeyIds) { |
There was a problem hiding this comment.
instead of kmskeyids use kmsKeysTag
There was a problem hiding this comment.
Sure, I will use kmeKeysTag
src/server/blockchain/Web3Wallet.js
Outdated
| addresses.forEach(address => { | ||
| const keyId = this.kmsWallet.getKeyId(address) | ||
| this.addKMSWallet(address, keyId) | ||
| // Add KMS addresses to filledAddresses (they don't need admin verification) |
There was a problem hiding this comment.
not true, need same checks as in mnemonic.
dont duplicate code
There was a problem hiding this comment.
make sure balance check are run for these addresses also
There was a problem hiding this comment.
Sure I will do it
src/server/blockchain/Web3Wallet.js
Outdated
| * Send transaction using KMS signing | ||
| * @private | ||
| */ | ||
| async sendTransactionWithKMS( |
There was a problem hiding this comment.
deduplicate code like in AdminWallet you have just one sendTransaction method
| env: 'PRIVATE_KEY', | ||
| default: undefined | ||
| }, | ||
| kmsKeyIds: { |
There was a problem hiding this comment.
Sure, I will use kmeKeysTag
…ce Web3Wallet with optional parameters for improved error handling
…-1559 support detection in Web3Wallet
…let with PromiEvent handling and improved KMS integration
…b3Wallet with EIP-1559 support detection and gas pricing normalization
…ment to prevent async issues
…t initialization logic
…e without adminWalletAddress
…emoving test mode checks and redundant code
…ding EIP-1559 support checks and handling for test environment
…environment, preventing test failures from module imports
blueogin
force-pushed
the
feat/kms-wallet
branch
from
7917929 to
a7643d9
Compare
…andling in AdminWallet tests
…ey handling and removing redundant test environment checks
…let, enhancing key management capabilities
Description
Added the KMS wallet, which signs transactions using keys stored in AWS KMS.
The
kms-ethereum-signingdependency is the modularized PoC project I previously built.About # (link your issue here)
How Has This Been Tested?
The KMS wallet is tested in
adminWalletKMS.test.ts.Checklist: