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

Evalir · 2023-05-04T05:53:01Z

Motivation

Closes #4831.

cast send right now assumes that if a signer can't be constructed, it must fall back to eth_sendTransaction in all cases. This is not exactly true, and will result in misleading error messages depending on the combination of flags passed in for the wanted signing method. Particular highlights were:

If a path to a folder that contains a keystore was passed in, it would try and walk the directory to try and find it. However, keystores no longer contain the address field that was being used to identify the correct keystore, so they were never found. This caused cast send to fall back to eth_sendTransaction hoping the remote RPC had the unlocked account to sign the tx.
If the ledger option was passed in but the ledger was not available or connected, cast send would again fall back to eth_sendTransaction. This would happen similarly with other available local signing methods.
If a --from address is passed in and no other information is provided, cast send would again fall back to eth_sendTransaction. This is actually "correct" behavior, but the user is kept in the dark as to why this is and is not given enough context.

Solution

The send command was lightly refactored and commented for future reference. The main changes are:

Defaulting to eth_sendTransaction is now only possible by passing the --unlocked flag, matching forge script's behavior. This flag also requires the --from flag to be passed, to ensure enough information is passed in by the user.
If not defaulting to eth_sendTransaction, we must now be able to construct a signer with the flags passed in, or we will bail and inform the user something is wrong.
THIS BREAKS COMPATIBILITY WITH DAPPTOOLS: We now don't walk directories looking for keystores if only a directory is passed in with the --keystore flag, and instead bail and tell the user to provide the keystore file directly. This is because keystores are no longer generated with the address inside of them. (This is a big discussion point, and while I favor being more strict, we could also try and decrypt all the keystores found and use the first one to succeed, but this has edge cases that could lead to bad scenarios).
If a ledger is not available for whatever reason, instead of defaulting to eth_sendTransaction, we bail and inform the user that they must only have the ledger unlocked and connected with no other conflicting wallet apps open.

EDIT: @DaniPopes @0xMelkor can't seem to be able to add you as reviewers, but as always, feel free to review!

…action

…ledger/local failures

Evalir · 2023-05-04T05:56:39Z

cli/src/opts/wallet/mod.rs

-    /// 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() {


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.

0xMelkor · 2023-05-04T11:40:47Z

EDIT: @DaniPopes @0xMelkor can't seem to be able to add you as reviewers, but as always, feel free to review!

On it

0xMelkor

@Evalir ty, this is my review

cli/src/opts/wallet/mod.rs

cli/src/cmd/cast/send.rs

0xMelkor · 2023-05-04T12:36:05Z

cli/src/cmd/cast/send.rs

+            // Checking if signer isn't the default value
+            // 00a329c0648769A73afAc7F9381E08FB43dBEA72.
+            if resend {
+                tx.nonce = Some(provider.get_transaction_count(config.sender, None).await?);


This is duplicated, can we extract a function?

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

see #4874 (comment)

0xMelkor · 2023-05-04T12:40:08Z

cli/src/cmd/cast/send.rs

+                tx.nonce = Some(provider.get_transaction_count(config.sender, None).await?);
+            }
+
+            cast_send(


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?

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!

0xMelkor · 2023-05-04T12:40:48Z

cli/src/cmd/cast/send.rs

@@ -109,19 +157,6 @@ impl SendTxArgs {
                tx.nonce = Some(provider.get_transaction_count(from, None).await?);


Duplicated, see above

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

mattsse

changes lgtm, haven't tested it though,

pending @0xMelkor for final approval

mattsse · 2023-05-04T15:35:40Z

cli/src/cmd/cast/send.rs

@@ -109,19 +157,6 @@ impl SendTxArgs {
                tx.nonce = Some(provider.get_transaction_count(from, None).await?);


cli/src/cmd/cast/send.rs

Since foundry-rs/foundry#4874 the `--from` flag only works in combination with the `--unlocked` flag. This commit adds the missing `--unlocked` flag to an example that is using `--from`.

Evalir added 3 commits May 3, 2023 10:14

feat: add way to specify remote signing to fall back to eth_sendTrans…

5d1f696

…action

chore: bail on walking folders, add some more message context around …

a84ad35

…ledger/local failures

chore: light refactor on send command to add more strictness

0eca4ff

Evalir requested review from gakonst and mattsse May 4, 2023 05:53

Evalir commented May 4, 2023

View reviewed changes

0xMelkor reviewed May 4, 2023

View reviewed changes

Evalir added the C-cast Command: cast label May 4, 2023

Evalir added 3 commits May 4, 2023 09:59

chore: change error messages

c511f56

chore: lint

10fab16

chore: remove dir check from test

327d7b2

mattsse approved these changes May 4, 2023

View reviewed changes

cli/src/cmd/cast/send.rs

@@ -109,19 +157,6 @@ impl SendTxArgs {

tx.nonce = Some(provider.get_transaction_count(from, None).await?);

Copy link

Member

mattsse May 4, 2023

Choose a reason for hiding this comment

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

+1

Evalir requested a review from 0xMelkor May 4, 2023 16:08

DaniPopes requested changes May 5, 2023

View reviewed changes

cli/src/cmd/cast/send.rs Outdated Show resolved Hide resolved

Evalir mentioned this pull request May 6, 2023

cast send: add --bump-fee flag that resends with iteratively increasing gas price #4454

Open

chore: escape trailing newlines

7da515e

Evalir requested a review from DaniPopes May 6, 2023 14:19

DaniPopes approved these changes May 6, 2023

View reviewed changes

Evalir requested a review from mattsse May 8, 2023 22:39

mattsse approved these changes May 9, 2023

View reviewed changes

mattsse merged commit f0a42cc into master May 9, 2023

mattsse deleted the evalir/cast_send_issue branch May 9, 2023 10:02

agostbiro mentioned this pull request Jun 12, 2023

fix(docs): add missing flag to example foundry-rs/book#918

Merged

Evalir mentioned this pull request Jun 12, 2023

cast send help text incorrect and incomplete #2296

Closed

2 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

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

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

Evalir commented May 4, 2023 •

edited

Loading

Evalir May 4, 2023

0xMelkor commented May 4, 2023

0xMelkor left a comment

0xMelkor May 4, 2023

Evalir May 4, 2023

0xMelkor May 4, 2023

Evalir May 4, 2023

0xMelkor May 4, 2023

Evalir May 4, 2023

mattsse May 4, 2023

mattsse left a comment

mattsse May 4, 2023

		@@ -109,19 +157,6 @@ impl SendTxArgs {
		tx.nonce = Some(provider.get_transaction_count(from, None).await?);

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

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

Conversation

Evalir commented May 4, 2023 • edited Loading

Motivation

Solution

Choose a reason for hiding this comment

0xMelkor commented May 4, 2023

0xMelkor left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mattsse left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

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

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

Evalir commented May 4, 2023 •

edited

Loading