Skip to content

Commit

Permalink
fix(cast): Harden cast send and provide more accurate error messages (
Browse files Browse the repository at this point in the history 
#4874)

* feat: add way to specify remote signing to fall back to eth_sendTransaction

* chore: bail on walking folders, add some more message context around ledger/local failures

* chore: light refactor on send command to add more strictness

* chore: change error messages

* chore: lint

* chore: remove dir check from test

* chore: escape trailing newlines
  • Loading branch information
@Evalir
Evalir authored May 9, 2023
1 parent 43974b0 commit f0a42cc
Show file tree
Hide file tree
Showing 2 changed files with 79 additions and 97 deletions.
109 changes: 51 additions & 58 deletions cli/src/cmd/cast/send.rs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -50,6 +50,10 @@ pub struct SendTxArgs {

#[clap(subcommand)]
command: Option<SendTxSubcommands>,

/// Send via `eth_sendTransaction using the `--from` argument or $ETH_FROM as sender
#[clap(long, requires = "from")]
unlocked: bool,
}

#[derive(Debug, Parser)]
Expand Down Expand Up @@ -81,52 +85,39 @@ impl SendTxArgs {
json: to_json,
resend,
command,
unlocked,
} = self;
let config = Config::from(&eth);
let provider = utils::get_provider(&config)?;
let chain = utils::get_chain(config.chain_id, &provider).await?;
let api_key = config.get_etherscan_api_key(Some(chain));
let mut sig = sig.unwrap_or_default();

if let Ok(signer) = eth.wallet.signer(chain.id()).await {
let from = signer.address();

// prevent misconfigured hwlib from sending a transaction that defies
// user-specified --from
if let Some(specified_from) = eth.wallet.from {
if specified_from != from {
eyre::bail!("\
The specified sender via CLI/env vars does not match the sender configured via\n\
the hardware wallet's HD Path.\n\
Please use the `--hd-path <PATH>` parameter to specify the BIP32 Path which\n\
corresponds to the sender.\n\
This will be automatically detected in the future: https://github.com/foundry-rs/foundry/issues/2289\
")
}
}

let code = if let Some(SendTxSubcommands::Create {
code,
sig: constructor_sig,
args: constructor_args,
}) = command
{
sig = constructor_sig.unwrap_or_default();
args = constructor_args;
Some(code)
} else {
None
};

// Case 1:
// Default to sending via eth_sendTransaction if the --unlocked flag is passed.
// This should be the only way this RPC method is used as it requires a local node
// or remote RPC with unlocked accounts.
if unlocked {
if resend {
tx.nonce = Some(provider.get_transaction_count(from, None).await?);
tx.nonce = Some(provider.get_transaction_count(config.sender, None).await?);
}

let code = if let Some(SendTxSubcommands::Create {
code,
sig: constructor_sig,
args: constructor_args,
}) = command
{
sig = constructor_sig.unwrap_or_default();
args = constructor_args;
Some(code)
} else {
None
};

let provider = provider.with_signer(signer);

cast_send(
provider,
from,
config.sender,
to,
code,
(sig, args),
Expand All @@ -138,29 +129,38 @@ impl SendTxArgs {
to_json,
)
.await
} else if config.sender != Config::DEFAULT_SENDER {
// Checking if signer isn't the default value
// 00a329c0648769A73afAc7F9381E08FB43dBEA72.
// Case 2:
// An option to use a local signer was provided.
// If we cannot successfully instanciate a local signer, then we will assume we don't have
// enough information to sign and we must bail.
} else {
// Retrieve the signer, and bail if it can't be constructed.
let signer = eth.wallet.signer(chain.id()).await?;
let from = signer.address();

// prevent misconfigured hwlib from sending a transaction that defies
// user-specified --from
if let Some(specified_from) = eth.wallet.from {
if specified_from != from {
eyre::bail!(
"\
The specified sender via CLI/env vars does not match the sender configured via
the hardware wallet's HD Path.
Please use the `--hd-path <PATH>` parameter to specify the BIP32 Path which
corresponds to the sender, or let foundry automatically detect it by not specifying any sender address."
)
}
}

if resend {
tx.nonce = Some(provider.get_transaction_count(config.sender, None).await?);
tx.nonce = Some(provider.get_transaction_count(from, None).await?);
}

let code = if let Some(SendTxSubcommands::Create {
code,
sig: constructor_sig,
args: constructor_args,
}) = command
{
sig = constructor_sig.unwrap_or_default();
args = constructor_args;
Some(code)
} else {
None
};
let provider = provider.with_signer(signer);

cast_send(
provider,
config.sender,
from,
to,
code,
(sig, args),
Expand All @@ -172,13 +172,6 @@ impl SendTxArgs {
to_json,
)
.await
} else {
Err(eyre::eyre!(
"\
No wallet or sender address provided. Consider passing it via the --from flag or\n\
setting the ETH_FROM env variable or setting in foundry.toml\
"
))
}
}
}
Expand Down
67 changes: 28 additions & 39 deletions cli/src/opts/wallet/mod.rs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -13,7 +13,7 @@ use ethers::{
Address, Signature,
},
};
use eyre::{bail, eyre, Result, WrapErr};
use eyre::{bail, Result, WrapErr};
use foundry_common::fs;
use serde::{Deserialize, Serialize};
use std::{
Expand Down Expand Up @@ -133,7 +133,7 @@ pub struct Wallet {
pub trezor: bool,

/// Use AWS Key Management Service.
#[clap(long, help_heading = "Wallet options - remote")]
#[clap(long, help_heading = "Wallet options - AWS KMS")]
pub aws: bool,
}

Expand Down Expand Up @@ -201,7 +201,11 @@ impl Wallet {
Some(hd_path) => LedgerHDPath::Other(hd_path.clone()),
None => LedgerHDPath::LedgerLive(self.mnemonic_index as usize),
};
let ledger = Ledger::new(derivation, chain_id).await?;
let ledger = Ledger::new(derivation, chain_id).await.wrap_err_with(|| {
"\
Could not connect to Ledger device.
Make sure it's connected and unlocked, with no other desktop wallet apps open."
})?;

Ok(WalletSigner::Ledger(ledger))
} else if self.trezor {
Expand All @@ -211,7 +215,11 @@ impl Wallet {
};

// cached to ~/.ethers-rs/trezor/cache/trezor.session
let trezor = Trezor::new(derivation, chain_id, None).await?;
let trezor = Trezor::new(derivation, chain_id, None).await.wrap_err_with(|| {
"\
Could not connect to Trezor device.
Make sure it's connected and unlocked, with no other conflicting desktop wallet apps open."
})?;

Ok(WalletSigner::Trezor(trezor))
} else if self.aws {
Expand All @@ -230,17 +238,18 @@ impl Wallet {

let maybe_local = self.try_resolve_local_wallet()?;

let local = maybe_local
.ok_or_else(|| eyre::eyre!("\
Error accessing local wallet. Did you set a private key, mnemonic or keystore?\n\
Run `cast send --help` or `forge create --help` and use the corresponding CLI\n\
flag to set your key via:\n\
--private-key, --mnemonic-path, --aws, --interactive, --trezor or --ledger.\n\
\n\
Alternatively, if you're using a local node with unlocked accounts,\n\
use the --unlocked flag and set the `ETH_FROM` environment variable to the address\n\
of the unlocked account you want to use.\
"))?;
let local = maybe_local.ok_or_else(|| {
eyre::eyre!(
"\
Error accessing local wallet. Did you set a private key, mnemonic or keystore?
Run `cast send --help` or `forge create --help` and use the corresponding CLI
flag to set your key via:
--private-key, --mnemonic-path, --aws, --interactive, --trezor or --ledger.
Alternatively, if you're using a local node with unlocked accounts,
use the --unlocked flag and either set the `ETH_FROM` environment variable to the address
of the unlocked account you want to use, or provide the --from flag with the address directly."
)
})?;

Ok(WalletSigner::Local(local.with_chain_id(chain_id)))
}
Expand Down Expand Up @@ -316,35 +325,18 @@ pub trait WalletTrait {
Ok(builder.build()?)
}

/// Attempts to find the actual path of the keystore file.
/// Ensures the path to the keystore exists.
///
/// If the path is a directory then we try to find the first keystore file with the correct
/// sender address
/// if the path is a directory, it bails and asks the user to specify the keystore file
/// directly.
fn find_keystore_file(&self, path: impl AsRef<Path>) -> Result<PathBuf> {
let path = path.as_ref();
if !path.exists() {
bail!("Keystore file `{path:?}` does not exist")
}

if path.is_dir() {
let sender =
self.sender().ok_or_else(|| eyre!("No sender account configured: $ETH_FROM"))?;

let (_, file) = walkdir::WalkDir::new(path)
.max_depth(2)
.into_iter()
.filter_map(Result::ok)
.filter(|e| e.file_type().is_file())
.filter_map(|e| {
fs::read_json_file::<KeystoreFile>(e.path())
.map(|keystore| (keystore, e.path().to_path_buf()))
.ok()
})
.find(|(keystore, _)| keystore.address == sender)
.ok_or_else(|| {
eyre!("No matching keystore file found for {sender:?} in {path:?}")
})?;
return Ok(file)
bail!("Keystore path `{path:?}` is a directory. Please specify the keystore file directly.")
}

Ok(path.to_path_buf())
Expand Down Expand Up @@ -548,9 +540,6 @@ mod tests {
"--from",
"560d246fcddc9ea98a8b032c9a2f474efb493c28",
]);
let file = wallet.find_keystore_file(&keystore).unwrap();
assert_eq!(file, keystore_file);

let file = wallet.find_keystore_file(&keystore_file).unwrap();
assert_eq!(file, keystore_file);
}
Expand Down

0 comments on commit f0a42cc

Please sign in to comment.