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.

Sign up for GitHub

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

Jump to bottom

Include HW wallets in cast wallet ls #6958

Closed
mattsse opened this issue Jan 30, 2024 · 6 comments
Closed

Include HW wallets in cast wallet ls #6958

mattsse opened this issue Jan 30, 2024 · 6 comments
Labels
good first issue Good for newcomers

Comments

@mattsse
Copy link
Member

mattsse commented Jan 30, 2024 

          I mostly use cast with hardware wallets and something like `cast wallet ls` for them would be extremely useful.

Originally posted by @lightclient in #656 (comment)
@mattsse mattsse added the good first issue Good for newcomers label Jan 30, 2024
@PanGan21
Copy link
Contributor

PanGan21 commented Feb 4, 2024

Hey @mattsse looking at this issue, what is the difference with the current implementation of cast wallet ls as described here?
https://book.getfoundry.sh/reference/cast/cast-wallet-list

@mattsse
Copy link
Member Author

mattsse commented Feb 4, 2024

I believe the limitation @lightclient ran into was that wallet ls only targets the local keystores.

maybe we add an --all option to also check connected wallets, if any?

@mattsse mattsse changed the title Add cast wallet ls Include HW wallets in cast wallet ls Feb 4, 2024
@grandizzy
Copy link
Collaborator

I believe the limitation @lightclient ran into was that wallet ls only targets the local keystores.

maybe we add an --all option to also check connected wallets, if any?

can take this one, I propose following options for cast wallet list

  --dir (alias to default)
      List all the accounts in the keystore default directory
  -l, --ledger
      List accounts from Ledger hardware wallet
  -t, --trezor
      List accounts from Trezor hardware wallet
  --aws
      List configured AWS accounts
  --all
      List accounts from keystore default directory, hardware wallets and AWS

with an all option output like

Keystore default directory
 - test1
 - test2
Ledger
 LedgerLive HD path
  - 0x001...
  - 0x002...
  ...
 Legacy HD path
  - 0x003...
  - 0x004...
  ...
Trezor
 - 0x005...
 - 0x006...
 ...
AWS
 - 0x007...

(could also add option for the number of addresses and specific HD path to show)

@lightclient
Copy link
Contributor

This makes sense to me.

grandizzy added a commit to grandizzy/foundry that referenced this issue Feb 14, 2024
@grandizzy
issue foundry-rs#6958: Include HW wallets in cast wallet ls
5c3f9a8 
- added new options for list command, default list local keystore

  --dir (alias to default)
      List all the accounts in the keystore default directory
  -l, --ledger
      List accounts from Ledger hardware wallet
  -t, --trezor
      List accounts from Trezor hardware wallet
  --aws
      List configured AWS accounts
  --all
      List accounts from keystore default directory, hardware wallets and AWS

- for ledger display 2 addresses for each ledger live and legacy HD
- for trezor display 2 addresses for trezor live HD path
- for AWS display all keys configured

- unit tests
@grandizzy
Copy link
Collaborator

This makes sense to me.

made a draft PR #7123 looking for comments

grandizzy added a commit to grandizzy/foundry that referenced this issue Feb 20, 2024
@grandizzy
issue foundry-rs#6958: Include HW wallets in cast wallet ls
e8a8bf5
DaniPopes pushed a commit that referenced this issue Feb 22, 2024
@grandizzy
feat(cast wallet list) issue #6958: Include HW wallets in cast wall…
57815e0 
…et ls (#7123)

* issue #6958: Include HW wallets in cast wallet ls

* Changes after review:
- 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

* Changes after review: use list_signers macro

* Changes after review:
- 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

* Nit

* Remove list_senders fn, move logic in macro

* Nit macro
@grandizzy
Copy link
Collaborator

fixed in #7123 @mattsse this can be closed

@mattsse mattsse closed this as completed Apr 16, 2024
This was referenced Apr 26, 2024
Merged
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees
No one assigned
Labels
good first issue Good for newcomers
Projects
No open projects
Foundry
Status: Todo
Milestone
No milestone
Development

No branches or pull requests
4 participants
@lightclient @mattsse @grandizzy @PanGan21