cmd/clef: only print first N accounts on startup #26128
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
holiman
reviewed
Nov 7, 2022
|
I think this is a good change. Showing the accounts is mostly useful for people new to clef. If someone is using it for a production setup with lots of keys, it would be annoying to get a huge output dump on startup when you're just trying to debug something. |
Co-authored-by: Martin Holst Swende <martin@swende.se>
holiman
reviewed
Nov 8, 2022
signer/core/cliui.go
Outdated
| @@ -245,6 +245,9 @@ func (ui *CommandlineUI) OnApprovedTx(tx ethapi.SignTransactionResult) { | |||
|
|
|||
| func (ui *CommandlineUI) showAccounts() { | |||
| accounts, err := ui.api.ListAccounts(context.Background()) | |||
| var limit int = 20 //max N accounts to print | |||
| var msg string = fmt.Sprintf("\nFirst %d accounts listed (%d more available).\n", limit, len(accounts)-limit) | |||
There was a problem hiding this comment.
Seems a bit strange that the "you have more than 20 accounts" is the default, which you initialize here. Feels more natural to have the empty-case be the default, var msg string, and further below have the clause
if limit := 20; len(accounts) > limit {
fmt.Fprintf(out, "\nFirst ...
accounts = accounts[:limit]
...You can probably simplify that a bit too, e.g. by just shortening the accounts to be llimit long. And then the code outside of that clause doesn't need to know that a limit even exists
shekhirin
pushed a commit
to shekhirin/go-ethereum
that referenced
this pull request
Jun 6, 2023
PR ethereum#26082 added account listing to OnSignerStartup but did not consider the case where a user has a large number of accounts which would be annoying to display. This PR updates showAccounts() so that if there are more than 20 accounts available the user sees the first 20 displayed in the console followed by: First 20 accounts listed (N more available). Co-authored-by: Martin Holst Swende <martin@swende.se>
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
PR #26082 added account listing to
OnSignerStartupbut did not consider the case where a user has a large number of accounts which would be annoying to display.This PR updates
showAccounts()so that if there are more than 20 accounts available the user sees the first 20 displayed in the console followed by:First 20 accounts listed (N more available).For example for a keystore with > 20 accounts the startup info looks as follows:
For <20 accounts the console info is the same as before.
(NB 20 was a pretty arbitrary choice, I just thought it was a manageable number).