feat(`cast wallet list`) issue #6958: Include HW wallets in cast wallet ls #7123

grandizzy · 2024-02-14T16:48:11Z

added new options for list command, default list local keystore

Options:
      --dir [<DIR>]                List all the accounts in the keystore directory. Default keystore directory is used if no path provided
  -l, --ledger                     List accounts from a Ledger hardware wallet
  -t, --trezor                     List accounts from a Trezor hardware wallet
  -m, --max-senders <MAX_SENDERS>  Max number of addresses to display from hardware wallets [default: 3]
      --aws                        List accounts from AWS KMS
      --all                        List all configured accounts
  -h, --help                       Print help

for ledger display 3 addresses for each ledger live and legacy HD
for trezor display 3 addresses for trezor live HD path
for AWS display all keys configured
for local keystore list files in dir
unit tests

Note: I don't have Trezor / AWS configured

Things to discuss:

chain id is hardcoded to 0 and not determined from rpc url, should it be?
number of accounts to display are hardcoded to 2, should this be configurable?
for local accounts - rn it creates the keystore directory if it doesn't exist, I'd argue this shouldn't be part of list command?

Motivation

Solution

mattsse

this is great.

I want to make this more like a utility function that this command then consumes

crates/cast/bin/cmd/wallet/mod.rs

mattsse · 2024-02-14T22:34:00Z

crates/cast/bin/cmd/wallet/mod.rs

-                                    Some(Ok(path))
-                                } else {
-                                    None
+            WalletSubcommands::List { dir, ledger, trezor, aws, all } => {


I think this would be useful in the wallet crate @klkvr

like a struct that looks up all configured sources and returns the results

so I think we want to do this a bit differently,
the helper struct should gather all the info and then we print them here in the command

makes sense, happy to accommodate such after things moved to new crate or leave up to @klkvr lmk what you guys decide

@grandizzy we should merge #7141 soon which will change structure of foundry-wallets, so I think for now this PR is somewhat blocked, but feel free to take a look at the code structure and entities in #7141 as those are unlikely to change much

once its merged I believe we can extend WalletSigner enum from there with async fn available_senders(&self, max: usize) -> Vec<Address> and then use it for the list command

you should be able to obtain WalletSigners either by calling utils functions directly or by constructing MultiWalletOpts with hw wallets opts + keystore paths, and then calling get_multi_wallet on it

thanks, checked and makes sense (I might need to derive builder for MultiWalletOpts), will prepare changes and make a PR once #7141 in master

I redid PR with new wallet structure, pretty tidy now imo, please review

mattsse

few more nits.

pending @klkvr and @DaniPopes

mattsse · 2024-02-20T18:53:34Z

crates/wallets/Cargo.toml

@@ -24,6 +24,7 @@ foundry-common.workspace = true

 async-trait = "0.1"
 clap = { version = "4", features = ["derive", "env", "unicode", "wrap_help"] }
+derive_builder = "0.20.0"


I'm personally okay with this

any objections @DaniPopes ?

that's fine

mattsse · 2024-02-20T18:54:36Z

crates/cast/bin/cmd/wallet/list.rs

+            .private_keys(None)
+            .private_key(None)
+            .mnemonics(None)
+            .mnemonic_passphrases(None)
+            .hd_paths(None)
+            .keystore_paths(None)
+            .keystore_account_names(None)
+            .keystore_passwords(None)
+            .keystore_password_files(None)


I don't think these are required

per builder doc The build method returns Result<T, E>, where T is the struct you started with and E is a generated builder error type. It returns Err if you didn’t initialize all fields and no default values were provided. I removed the code and made builder defaults as annotations

mattsse · 2024-02-20T18:58:05Z

crates/wallets/src/wallet_signer.rs

@@ -57,6 +57,37 @@ impl WalletSigner {
        Ok(Self::Local(wallet))
    }

+    pub async fn available_senders(&self, max: usize) -> Result<Vec<ethers_core::types::Address>> {


please add one line doc

done. Also updated impl to handle Local signer too and to return Ledger addresses for legacy derivation path as well

mattsse · 2024-02-20T19:00:05Z

crates/cast/bin/cmd/wallet/list.rs

+        let keystore_path = self.dir.clone().unwrap_or_default();
+        let keystore_dir = if keystore_path.is_empty() {
+            let default_dir = Config::foundry_keystores_dir()
+                .ok_or_else(|| eyre::eyre!("Could not find the default keystore directory."))?;


this should be if let Some else instead

I simplified handle logic for creating default dir, need the unwrap_or_default as this code handles the cast wallet ls command where dir is None

mattsse · 2024-02-20T19:03:31Z

crates/cast/bin/cmd/wallet/list.rs

+impl ListArgs {
+    pub async fn run(self) -> Result<()> {
+        // list local accounts as files in keystore dir, no need to unlock / provide password
+        if self.dir.is_some() || self.all || !self.ledger && !self.trezor && !self.aws {


hmm, this will always be true if any leder,trezor,aws argument is missing

should this be || (!self.ledger && !self.trezor && !self.aws) instead?

yep, changed

- use annotations for builder defaults - handle Local signer in available_senders, return Ledger addresses for legacy derivation, add doc - fix condition to list files in keystore dir - simplify creation of keystore default directory

klkvr

thank you, lgtm! just a nit

klkvr · 2024-02-21T14:08:53Z

crates/cast/bin/cmd/wallet/list.rs

+        // List ledger accounts
+        match list_opts.ledgers().await {
+            Ok(signers) => {
+                self.list_senders(signers.unwrap_or_default(), max_senders, "Ledger").await?
+            }
+            Err(e) => {
+                if !self.all {
+                    println!("{}", e)
+                }
+            }
+        }
+
+        // List Trezor accounts
+        match list_opts.trezors().await {
+            Ok(signers) => {
+                self.list_senders(signers.unwrap_or_default(), max_senders, "Trezor").await?
+            }
+            Err(e) => {
+                if !self.all {
+                    println!("{}", e)
+                }
+            }
+        }
+
+        // List AWS accounts
+        match list_opts.aws_signers().await {
+            Ok(signers) => {
+                self.list_senders(signers.unwrap_or_default(), max_senders, "AWS").await?
+            }
+            Err(e) => {
+                if !self.all {
+                    println!("{}", e)
+                }
+            }
+        }
+


would be nice if we could reduce duplication here via macro/fn

yep, good point, changed in c4ce461

DaniPopes · 2024-02-21T16:48:49Z

crates/cast/bin/cmd/wallet/list.rs

+    aws: bool,
+
+    /// List all configured accounts.
+    #[clap(long, help_heading = "List all accounts")]


Remove help_headings, these don't need sections for each option

removed in 38fc356

DaniPopes · 2024-02-21T16:49:13Z

crates/cast/bin/cmd/wallet/list.rs

+            dunce::canonicalize(keystore_path)?
+        };
+
+        match std::fs::read_dir(keystore_dir) {


remove match, use ? as it already exists

removed in 38fc356

DaniPopes · 2024-02-21T16:49:24Z

crates/cast/bin/cmd/wallet/list.rs

+        Ok(())
+    }
+
+    async fn list_local_senders(&self) -> Result<()> {


doesn't need async

updated in 38fc356

DaniPopes · 2024-02-21T16:50:08Z

crates/cast/bin/cmd/wallet/list.rs

+            .expect("build multi wallet");
+
+        // max number of senders to be shown for ledger and trezor signers
+        let max_senders = 3;


should this be an argument?

yep, made this a wallet list argument, grouped with hardware wallets and all option 38fc356

DaniPopes · 2024-02-21T16:54:18Z

crates/wallets/src/wallet_signer.rs

+        match self {
+            WalletSigner::Local(local) => {
+                senders.push(local.address());
+                Ok(senders)


move Ok(senders) to the bottom outside of the match

done in 38fc356

DaniPopes · 2024-02-21T16:54:31Z

crates/wallets/src/wallet_signer.rs

+    /// - the result for Ledger signers includes addresses available for both LedgerLive and Legacy
+    ///   derivation paths
+    /// - for Local and AWS signers the result contains a single address
+    pub async fn available_senders(&self, max: usize) -> Result<Vec<ethers_core::types::Address>> {


This can't fail

changed list_senders fn to reflect this in 38fc356

DaniPopes · 2024-02-21T16:55:42Z

crates/cast/bin/cmd/wallet/list.rs

+
+    async fn list_senders(
+        &self,
+        signers: Vec<WalletSigner>,


Suggested change

signers: Vec<WalletSigner>,

signers: &[WalletSigner],

~~not sure exactly what to change here, can you pls elaborate?~~ changed in 4ecb726 but not needed anymore as moved list_senders logic in macro 6474ca3

DaniPopes · 2024-02-21T16:57:55Z

crates/wallets/Cargo.toml

@@ -24,6 +24,7 @@ foundry-common.workspace = true

 async-trait = "0.1"
 clap = { version = "4", features = ["derive", "env", "unicode", "wrap_help"] }
+derive_builder = "0.20.0"


that's fine

- remove help_headings - remove match and use ? as dir already exists - remove async from list_local_senders fn - move Ok(senders) at the bottom of available_senders fn - list_senders doesn't need match as available_senders cannot fail - make max_senders arg for ls command , default 3

mattsse

last style nit

otherwise lgtm
pending @DaniPopes

mattsse · 2024-02-22T11:23:56Z

crates/cast/bin/cmd/wallet/list.rs

+        let label = "Ledger";
+        let signers = list_opts.ledgers();
+        list_senders!(signers, label);
+
+        let label = "Trezor";
+        let signers = list_opts.trezors();
+        list_senders!(signers, label);
+
+        let label = "AWS";
+        let signers = list_opts.aws_signers();
+        list_senders!(signers, label);


Suggested change

let label = "Ledger";

let signers = list_opts.ledgers();

list_senders!(signers, label);

let label = "Trezor";

let signers = list_opts.trezors();

list_senders!(signers, label);

let label = "AWS";

let signers = list_opts.aws_signers();

list_senders!(signers, label);

list_senders!(list_opts.ledgers(), "Ledger");

list_senders!(list_opts.trezors(), "Trezor");

list_senders!(list_opts.aws_signers(), "AWS");

mattsse · 2024-02-22T11:24:07Z

crates/cast/bin/cmd/wallet/list.rs

+
+        // macro to print senders for a list of signers
+        macro_rules! list_senders {
+            ($signers:ident, $label: ident) => {


Suggested change

($signers:ident, $label: ident) => {

($signers:expr, $label:literal) => {

grandizzy · 2024-02-22T11:52:54Z

last style nit

otherwise lgtm pending @DaniPopes

yep, nicer :) added in a9cd6ad

DaniPopes

Thanks!

grandizzy mentioned this pull request Feb 14, 2024

Include HW wallets in cast wallet ls #6958

Closed

grandizzy changed the title ~~issue #6958: Include HW wallets in cast wallet ls~~ feat(cast wallet list) issue #6958: Include HW wallets in cast wallet ls Feb 14, 2024

mattsse requested changes Feb 14, 2024

View reviewed changes

mattsse requested a review from klkvr February 14, 2024 22:35

grandizzy force-pushed the wallet-list branch from 2d72592 to b8f8c01 Compare February 15, 2024 11:34

issue foundry-rs#6958: Include HW wallets in cast wallet ls

e8a8bf5

grandizzy force-pushed the wallet-list branch from b8f8c01 to e8a8bf5 Compare February 20, 2024 17:54

grandizzy marked this pull request as ready for review February 20, 2024 18:11

grandizzy requested review from DaniPopes and Evalir as code owners February 20, 2024 18:11

grandizzy requested a review from mattsse February 20, 2024 18:11

mattsse requested changes Feb 20, 2024

View reviewed changes

Changes after review:

981b966

- use annotations for builder defaults - handle Local signer in available_senders, return Ledger addresses for legacy derivation, add doc - fix condition to list files in keystore dir - simplify creation of keystore default directory

grandizzy requested a review from mattsse February 21, 2024 08:11

klkvr approved these changes Feb 21, 2024

View reviewed changes

Changes after review: use list_signers macro

c4ce461

DaniPopes requested changes Feb 21, 2024

View reviewed changes

grandizzy requested a review from DaniPopes February 21, 2024 19:05

grandizzy added 2 commits February 22, 2024 08:11

Nit

4ecb726

Remove list_senders fn, move logic in macro

6474ca3

mattsse approved these changes Feb 22, 2024

View reviewed changes

Nit macro

a9cd6ad

DaniPopes approved these changes Feb 22, 2024

View reviewed changes

DaniPopes merged commit 57815e0 into foundry-rs:master Feb 22, 2024
21 checks passed

This was referenced Apr 26, 2024

Reset to upstream phylaxsystems/phoundry#12

Merged

feat/forge bin as lib phylaxsystems/phoundry#13

Merged

	($signers:ident, $label: ident) => {
	($signers:expr, $label:literal) => {

feat(cast wallet list) issue #6958: Include HW wallets in cast wallet ls #7123

feat(cast wallet list) issue #6958: Include HW wallets in cast wallet ls #7123

Conversation

grandizzy commented Feb 14, 2024 • edited Loading

Motivation

Solution

mattsse left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

klkvr Feb 16, 2024 • edited Loading

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

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

Choose a reason for hiding this comment

klkvr left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

DaniPopes Feb 21, 2024 • edited Loading

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

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

grandizzy Feb 21, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mattsse left a comment • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

grandizzy commented Feb 22, 2024

DaniPopes left a comment

Choose a reason for hiding this comment

feat(`cast wallet list`) issue #6958: Include HW wallets in cast wallet ls #7123

feat(`cast wallet list`) issue #6958: Include HW wallets in cast wallet ls #7123

grandizzy commented Feb 14, 2024 •

edited

Loading

klkvr Feb 16, 2024 •

edited

Loading

DaniPopes Feb 21, 2024 •

edited

Loading

grandizzy Feb 21, 2024 •

edited

Loading

mattsse left a comment •

edited

Loading