Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

fix(cast): Harden cast send and provide more accurate error messages #4874

Merged
merged 7 commits into from
May 9, 2023
Merged
Show file tree
Hide file tree
Changes from 3 commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
108 changes: 51 additions & 57 deletions cli/src/cmd/cast/send.rs
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")]
Evalir marked this conversation as resolved.
Show resolved Hide resolved
unlocked: bool,
}

#[derive(Debug, Parser)]
Expand Down Expand Up @@ -81,14 +85,59 @@ 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 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 {
// Checking if signer isn't the default value
Evalir marked this conversation as resolved.
Show resolved Hide resolved
// 00a329c0648769A73afAc7F9381E08FB43dBEA72.
if resend {
tx.nonce = Some(provider.get_transaction_count(config.sender, None).await?);
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is duplicated, can we extract a function?

async fn update_nonce(tx: &mut Tx...

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

}

cast_send(
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

cast_send is called twice.
Since cast_send is our final intent, can we refactor to have just a call at the end of the fn block?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yeah the reason this is written like so is that provider actually takes different types depending if you have a signer or not, which will also affect from and the eth_* method used. We want to clearly differentiate when we're falling back to eth_sendTransaction vs using a signer and using eth_sendRawTransaction, and so we have these two code paths. I actually think abstracting it all away so it looks like only one path could make this harder to reason about and could allow misleading error messages due to fall-through cases. Let me know what you think!

provider,
config.sender,
to,
code,
(sig, args),
tx,
chain,
api_key,
cast_async,
confirmations,
to_json,
)
.await
// 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
Expand All @@ -99,8 +148,7 @@ impl SendTxArgs {
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\
corresponds to the sender, or let foundry automatically detect it by not specifying any sender address.\n\
Evalir marked this conversation as resolved.
Show resolved Hide resolved
")
}
}
Expand All @@ -109,19 +157,6 @@ impl SendTxArgs {
tx.nonce = Some(provider.get_transaction_count(from, None).await?);
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Duplicated, see above

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Answered in #4874 (comment) — essentially want to keep these code paths simple instead of further abstracting them.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

+1

}

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(
Expand All @@ -138,47 +173,6 @@ impl SendTxArgs {
to_json,
)
.await
} else if config.sender != Config::DEFAULT_SENDER {
// Checking if signer isn't the default value
// 00a329c0648769A73afAc7F9381E08FB43dBEA72.
if resend {
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
};

cast_send(
provider,
config.sender,
to,
code,
(sig, args),
tx,
chain,
api_key,
cast_async,
confirmations,
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
41 changes: 15 additions & 26 deletions cli/src/opts/wallet/mod.rs
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,13 @@ 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(|| {
Evalir marked this conversation as resolved.
Show resolved Hide resolved
"\
Could not connect to Ledger device.\n\
Make sure it's connected and unlocked, with no other desktop wallet apps open.\
"
.to_string()
})?;

Ok(WalletSigner::Ledger(ledger))
} else if self.trezor {
Expand Down Expand Up @@ -238,8 +244,8 @@ impl Wallet {
--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.\
use the --unlocked flag and either set the `ETH_FROM` environment variable to the address\n\
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 +322,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() {
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Note that this is where compatibility with dapptools on this functionality is broken. Happy to accomodate suggestions on how we want to handle this as keystores do not have the expected address property.

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