Add Wallet APIs for local development #335
Conversation
|
Warning Review failedThe pull request is closed. WalkthroughThe recent changes introduce wallet management functionalities into the system. Specifically, they update configuration options to include a wallet API key, augment the API to handle wallet operations such as managing accounts, signing transactions, and conditionally include these wallet-based functionalities based on configuration flags. They also modify the build script to accommodate these new configurations for local testing purposes. Changes
Sequence Diagram(s)sequenceDiagram
participant User
participant Config as Config
participant Makefile as Makefile
participant Main as Main.go
participant Bootstrap as Bootstrap
participant API as API
participant WalletAPI as WalletAPI
User->>Config: Set --wallet-api-key flag
Config->>Makefile: Updated start-local target
Makefile->>Main: Include wallet-api-key parameter
Main->>Bootstrap: Initialize components
Bootstrap->>Config: Check WalletEnabled
Config-->>Bootstrap: WalletEnabled = true
Bootstrap->>WalletAPI: Instantiate WalletAPI
Bootstrap-->>API: Include WalletAPI in SupportedAPIs
User->>API: Send transaction request
API->>WalletAPI: Handle transaction
Poem
Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (invoked as PR comments)
Additionally, you can add CodeRabbit Configration File (
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 4
Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Files selected for processing (4)
- api/api.go (3 hunks)
- api/wallet.go (1 hunks)
- bootstrap/bootstrap.go (1 hunks)
- config/config.go (5 hunks)
Additional comments not posted (4)
api/wallet.go (3)
23-28: Constructor forWalletAPIlooks good.The
NewWalletAPIfunction correctly initializes theWalletAPIstruct.
37-51: Sign function correctly indicates unsupported operation.The
Signfunction correctly returnsErrNotSupported.
102-114: SendTransaction function looks good.The
SendTransactionfunction correctly handles the process of signing and sending a transaction.api/api.go (1)
Line range hint
36-73: SupportedAPIs function looks good.The
SupportedAPIsfunction correctly handles the inclusion ofwalletAPI.
| // Accounts returns the collection of accounts this node manages. | ||
| func (w *WalletAPI) Accounts() ([]common.Address, error) { | ||
| return []common.Address{ | ||
| crypto.PubkeyToAddress(w.config.WalletKey.PublicKey), | ||
| }, nil | ||
| } |
There was a problem hiding this comment.
Add error handling for unset WalletKey.
The Accounts function should handle the case where WalletKey is not set to avoid potential runtime errors.
func (w *WalletAPI) Accounts() ([]common.Address, error) {
+ if w.config.WalletKey == nil {
+ return nil, fmt.Errorf("wallet key is not set")
+ }
return []common.Address{
crypto.PubkeyToAddress(w.config.WalletKey.PublicKey),
}, nil
}Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| // Accounts returns the collection of accounts this node manages. | |
| func (w *WalletAPI) Accounts() ([]common.Address, error) { | |
| return []common.Address{ | |
| crypto.PubkeyToAddress(w.config.WalletKey.PublicKey), | |
| }, nil | |
| } | |
| func (w *WalletAPI) Accounts() ([]common.Address, error) { | |
| if w.config.WalletKey == nil { | |
| return nil, fmt.Errorf("wallet key is not set") | |
| } | |
| return []common.Address{ | |
| crypto.PubkeyToAddress(w.config.WalletKey.PublicKey), | |
| }, nil | |
| } |
api/wallet.go
Outdated
| // SignTransaction will sign the given transaction with the from account. | ||
| // The node needs to have the private key of the account corresponding with | ||
| // the given from address and it needs to be unlocked. | ||
| func (w *WalletAPI) SignTransaction( | ||
| ctx context.Context, | ||
| args TransactionArgs, | ||
| ) (*SignTransactionResult, error) { | ||
| nonce := uint64(0) | ||
| if args.Nonce != nil { | ||
| nonce = uint64(*args.Nonce) | ||
| } else { | ||
| num := rpc.BlockNumberOrHashWithNumber(rpc.LatestBlockNumber) | ||
| n, err := w.net.GetTransactionCount(ctx, *args.From, &num) | ||
| if err != nil { | ||
| return nil, err | ||
| } | ||
| nonce = uint64(*n) | ||
| } | ||
|
|
||
| var data []byte | ||
| if args.Data != nil { | ||
| data = *args.Data | ||
| } | ||
|
|
||
| tx := types.NewTx(&types.LegacyTx{ | ||
| Nonce: nonce, | ||
| To: args.To, | ||
| Value: args.Value.ToInt(), | ||
| Gas: uint64(*args.Gas), | ||
| GasPrice: args.GasPrice.ToInt(), | ||
| Data: data, | ||
| }) | ||
|
|
||
| signed, err := types.SignTx(tx, evmEmulator.GetDefaultSigner(), w.config.WalletKey) | ||
| if err != nil { | ||
| return nil, fmt.Errorf("error signing EVM transaction: %w", err) | ||
| } | ||
|
|
||
| raw, err := signed.MarshalBinary() | ||
| if err != nil { | ||
| return nil, err | ||
| } | ||
|
|
||
| return &SignTransactionResult{ | ||
| Raw: raw, | ||
| Tx: tx, | ||
| }, nil | ||
| } |
There was a problem hiding this comment.
Add error handling for unset WalletKey.
The SignTransaction function should handle the case where WalletKey is not set to avoid potential runtime errors.
func (w *WalletAPI) SignTransaction(
ctx context.Context,
args TransactionArgs,
) (*SignTransactionResult, error) {
+ if w.config.WalletKey == nil {
+ return nil, fmt.Errorf("wallet key is not set")
+ }
// existing code...
}Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| // SignTransaction will sign the given transaction with the from account. | |
| // The node needs to have the private key of the account corresponding with | |
| // the given from address and it needs to be unlocked. | |
| func (w *WalletAPI) SignTransaction( | |
| ctx context.Context, | |
| args TransactionArgs, | |
| ) (*SignTransactionResult, error) { | |
| nonce := uint64(0) | |
| if args.Nonce != nil { | |
| nonce = uint64(*args.Nonce) | |
| } else { | |
| num := rpc.BlockNumberOrHashWithNumber(rpc.LatestBlockNumber) | |
| n, err := w.net.GetTransactionCount(ctx, *args.From, &num) | |
| if err != nil { | |
| return nil, err | |
| } | |
| nonce = uint64(*n) | |
| } | |
| var data []byte | |
| if args.Data != nil { | |
| data = *args.Data | |
| } | |
| tx := types.NewTx(&types.LegacyTx{ | |
| Nonce: nonce, | |
| To: args.To, | |
| Value: args.Value.ToInt(), | |
| Gas: uint64(*args.Gas), | |
| GasPrice: args.GasPrice.ToInt(), | |
| Data: data, | |
| }) | |
| signed, err := types.SignTx(tx, evmEmulator.GetDefaultSigner(), w.config.WalletKey) | |
| if err != nil { | |
| return nil, fmt.Errorf("error signing EVM transaction: %w", err) | |
| } | |
| raw, err := signed.MarshalBinary() | |
| if err != nil { | |
| return nil, err | |
| } | |
| return &SignTransactionResult{ | |
| Raw: raw, | |
| Tx: tx, | |
| }, nil | |
| } | |
| func (w *WalletAPI) SignTransaction( | |
| ctx context.Context, | |
| args TransactionArgs, | |
| ) (*SignTransactionResult, error) { | |
| if w.config.WalletKey == nil { | |
| return nil, fmt.Errorf("wallet key is not set") | |
| } | |
| nonce := uint64(0) | |
| if args.Nonce != nil { | |
| nonce = uint64(*args.Nonce) | |
| } else { | |
| num := rpc.BlockNumberOrHashWithNumber(rpc.LatestBlockNumber) | |
| n, err := w.net.GetTransactionCount(ctx, *args.From, &num) | |
| if err != nil { | |
| return nil, err | |
| } | |
| nonce = uint64(*n) | |
| } | |
| var data []byte | |
| if args.Data != nil { | |
| data = *args.Data | |
| } | |
| tx := types.NewTx(&types.LegacyTx{ | |
| Nonce: nonce, | |
| To: args.To, | |
| Value: args.Value.ToInt(), | |
| Gas: uint64(*args.Gas), | |
| GasPrice: args.GasPrice.ToInt(), | |
| Data: data, | |
| }) | |
| signed, err := types.SignTx(tx, evmEmulator.GetDefaultSigner(), w.config.WalletKey) | |
| if err != nil { | |
| return nil, fmt.Errorf("error signing EVM transaction: %w", err) | |
| } | |
| raw, err := signed.MarshalBinary() | |
| if err != nil { | |
| return nil, err | |
| } | |
| return &SignTransactionResult{ | |
| Raw: raw, | |
| Tx: tx, | |
| }, nil | |
| } |
| var walletAPI *api.WalletAPI | ||
| if cfg.WalletEnabled { | ||
| walletAPI = api.NewWalletAPI(cfg, blockchainAPI) | ||
| } |
There was a problem hiding this comment.
Add error handling for walletAPI initialization.
The Start function should handle potential errors during walletAPI initialization.
if cfg.WalletEnabled {
walletAPI = api.NewWalletAPI(cfg, blockchainAPI)
+ if walletAPI == nil {
+ return fmt.Errorf("failed to initialize wallet API")
+ }
}Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| var walletAPI *api.WalletAPI | |
| if cfg.WalletEnabled { | |
| walletAPI = api.NewWalletAPI(cfg, blockchainAPI) | |
| } | |
| var walletAPI *api.WalletAPI | |
| if cfg.WalletEnabled { | |
| walletAPI = api.NewWalletAPI(cfg, blockchainAPI) | |
| if walletAPI == nil { | |
| return fmt.Errorf("failed to initialize wallet API") | |
| } | |
| } |
| if walletKey != "" { | ||
| var k, err = gethCrypto.HexToECDSA(walletKey) | ||
| if err != nil { | ||
| return nil, fmt.Errorf("wrong private key for wallet API: %w", err) | ||
| } | ||
|
|
||
| cfg.WalletKey = k | ||
| cfg.WalletEnabled = true | ||
| } |
There was a problem hiding this comment.
Add validation for WalletKey field.
The FromFlags function should validate the WalletKey field to ensure it is properly set.
if walletKey != "" {
var k, err = gethCrypto.HexToECDSA(walletKey)
if err != nil {
return nil, fmt.Errorf("wrong private key for wallet API: %w", err)
}
cfg.WalletKey = k
cfg.WalletEnabled = true
+} else {
+ cfg.WalletEnabled = false
}Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| if walletKey != "" { | |
| var k, err = gethCrypto.HexToECDSA(walletKey) | |
| if err != nil { | |
| return nil, fmt.Errorf("wrong private key for wallet API: %w", err) | |
| } | |
| cfg.WalletKey = k | |
| cfg.WalletEnabled = true | |
| } | |
| if walletKey != "" { | |
| var k, err = gethCrypto.HexToECDSA(walletKey) | |
| if err != nil { | |
| return nil, fmt.Errorf("wrong private key for wallet API: %w", err) | |
| } | |
| cfg.WalletKey = k | |
| cfg.WalletEnabled = true | |
| } else { | |
| cfg.WalletEnabled = false | |
| } |
api/wallet.go
Outdated
| nonce = uint64(*args.Nonce) | ||
| } else { | ||
| num := rpc.BlockNumberOrHashWithNumber(rpc.LatestBlockNumber) | ||
| n, err := w.net.GetTransactionCount(ctx, *args.From, &num) |
There was a problem hiding this comment.
Minor: I know that this is only for development, but *args.From might crash, as the From argument is optional.
| tx := types.NewTx(&types.LegacyTx{ | ||
| Nonce: nonce, | ||
| To: args.To, | ||
| Value: args.Value.ToInt(), |
There was a problem hiding this comment.
Same here, regarding:
args.Valueargs.Gasargs.GasPrice
We should check whether they are set.
| Data: data, | ||
| }) | ||
|
|
||
| signed, err := types.SignTx(tx, evmEmulator.GetDefaultSigner(), w.config.WalletKey) |
There was a problem hiding this comment.
Technically, I think we should be signing with the address in args.From, but since we have one PrivateKey in the config, this is good enough for development.
api/wallet.go
Outdated
| addr common.Address, | ||
| data hexutil.Bytes, | ||
| ) (hexutil.Bytes, error) { | ||
| return nil, errs.ErrNotSupported |
There was a problem hiding this comment.
We can add a minimal implementation for this a well, with something like:
// Transform the given message to the following format:
// keccak256("\x19Ethereum Signed Message:\n"${message length}${message})
hash := accounts.TextHash(data)
// Sign the hash using plain ECDSA operations
signature, err := crypto.Sign(hash, w.config.WalletKey)
if err == nil {
// Transform V from 0/1 to 27/28 according to the yellow paper
signature[64] += 27
}
return signature, err
Closes: #129
This adds Wallet APIs for local development.
This should not be used in production.
Some tools and clients rely on wallet APIs for local development and testing. This implementation enables the APIs by using a provided wallet key for operations through the config.
Summary by CodeRabbit
New Features
wallet-api-keyfor setting private keys in local or testing environments.Enhancements
WalletEnabledandWalletKeysettings.