cmd/clef: list accounts at startup #26082

jmcook1186 · 2022-11-01T10:24:09Z

Reports accounts known to Clef during startup, after master seed is provided by the user.
This addresses a TODO in the Clef README here.

Example output for clef --keystore testdata/keystore (omitting logs prior to user entering master password):

...
## Master Password

Please enter the password to decrypt the master seed
> 
-----------------------
INFO [11-01|10:17:23.779] Starting signer                          chainid=1 keystore=testdata/keystore light-kdf=false advanced=false
DEBUG[11-01|10:17:23.779] FS scan times                            list="48.716µs" set="12.935µs" diff="2.75µs"
DEBUG[11-01|10:17:23.781] Ledger support enabled 
DEBUG[11-01|10:17:23.781] Trezor support enabled via HID 
DEBUG[11-01|10:17:23.781] Trezor support enabled via WebUSB 
INFO [11-01|10:17:23.781] Smartcard socket file missing, disabling err="stat /run/pcscd/pcscd.comm: no such file or directory"
INFO [11-01|10:17:23.781] Audit logs configured                    file=audit.log
DEBUG[11-01|10:17:23.781] IPCs registered                          namespaces=account
INFO [11-01|10:17:23.782] IPC endpoint opened                      url=/home/joe/.clef/clef.ipc

Accounts known to Clef:

0xEcF899FCb2C48340e92d746Ee8F37958A0330678
0x1Aec041B2BdDAF395D474C50283436c0FDD2CC4d

------- Signer info -------
* intapi_version : 7.0.1
* extapi_version : 6.1.0

holiman · 2022-11-01T11:21:19Z

cmd/clef/main.go

+		ui_server := core.NewUIServerAPI(apiImpl)
+		accs, err := ui_server.ListAccounts(c.Context)
+		if err != nil {
+			return err
+		}


This is not correct. We already instantiate core.NewUIServerAPI(apiImpl) at line 569. If we want to maintain access to it, we should just keep it around instead of instantiating a second one here.

Now ,beside that... At this point, we already have a ui. It migth be a console UI, but it might also be a stdioui-ui, i.e.: a UI which uses json-rpc communication over standard-input and standard-output. A proper external UI would use that approach.

That means that we cannot "just print" stuff on stdout. If we need to show information to the user, the idiomatic way of doing it is to tell our ui to do so. The main way of doing that is ui.ShowInfo() -- that will work both for console users and remote-ui users.

If we want to display it only in the console, then we can either ensure to print on stderr (fmt.Fprintf(os.Stderr, "...), or show the info using logging - which is already configured to not disrupt the stdout.

Anyway, I'm not sure I agree with that readme-note any more. The UI can just ask the server about the accounts. If the ui-server is used, as opposed to the external api, then it should not necessitate explicit user approval, so be a very quick operation.

ah ok i see what you mean. Sorry - I think I've fixed this now, but if it is not a desired feature then no worries.

I think that what would be more idiomatic, would be if the cliui itself does this. So instead of clef spitting out the accounts on the console, the ui would ask for the accounts, and then show them to the user.
That's the job of the ui, not the job of the backend. You could hook into the OnSignerStartup, and make that call trigger the UI into doing an account listing.

In order to do that, you will need to 'capture' the backend that we currently ignore

func (ui *CommandlineUI) RegisterUIServer(api *UIServerAPI) { // noop }

and then extend the method here

func (ui *CommandlineUI) OnSignerStartup(info StartupInfo) { fmt.Printf("------- Signer info -------\n") for k, v := range info.Info { fmt.Printf("* %v : %v\n", k, v) } }

…to jc-clef3

holiman · 2022-11-04T15:28:54Z

Still this: #26082 (comment)

jmcook1186 · 2022-11-06T15:44:02Z

Still this: #26082 (comment)

ok I think this is addressed - thanks for the hints! The UI registers an API and the call to listAccounts() now originates inside ui.OnSignerStartup.

Terminal looks like this on startup:

Accounts known to Clef:

Account 0: 0x4f4094BaBd1A8c433e0f52A6ee3B6ff32dEe6a9c at keystore:///home/user/Code/go-ethereum/testdata/keystore/UTC--2022-11-01T17-05-01.517877299Z--4f4094babd1a8c433e0f52a6ee3b6ff32dee6a9c
Account 1: 0x8Ef15919F852A8034688a71d8b57Ab0187364009 at keystore:///home/user/Code/go-ethereum/testdata/keystore/UTC--2022-11-01T17-05-11.100536003Z--8ef15919f852a8034688a71d8b57ab0187364009
Account 2: 0x99A94ac1168B301449970310904fc38c7Bf55037 at keystore:///home/user/Code/go-ethereum/testdata/keystore/UTC--2022-11-02T11-40-55.527987625Z--99a94ac1168b301449970310904fc38c7bf55037

------- Signer info -------
* extapi_version : 6.1.0
* extapi_http : n/a
* extapi_ipc : /home/joe/.clef/clef.ipc
* intapi_version : 7.0.1

fix linting fix linting fix typo: accoutns -> accounts

holiman

Yep, this is roughly what I had in mind!

cmd/clef/main.go

holiman · 2022-11-07T09:18:05Z

signer/core/cliui.go

+	accounts, err := ui.api.ListAccounts(context.Background())
+	if err != nil {
+		fmt.Print("error listing accounts", err)
+	}
+	if len(accounts) == 0 {
+		fmt.Println("No accounts known to Clef")
+	} else {
+		// account info to string for nicer printing
+		var addresses string = "\n"
+		fmt.Println("Accounts known to Clef:")
+		for i, account := range accounts {
+			// concat string to avoid repeating "INFO" on terminal
+			addresses += fmt.Sprintf("Account %v: %s at %s", i, account.Address, account.URL)
+			addresses += "\n"
+		}
+		fmt.Print(addresses)
+		fmt.Println()
+	}


Generally correct, but two nitpicks.

We are in an rpc callback, a remote process has issued a remote procedure call. Generally, it might be waiting for a response (but in this particular case, OnSignerStartup is an answer-less request. Therefore, I think it's a good idea to end this call as soon as possible, and return to the caller and let the caller continue -- instead of inside the handler do a call back for listing accounts.

So, the TLDR; use this method for bootstrapping only, do the account-listing asynchronously by placing this clause inside a go func(){ ... }()

When printing to os.Stdout, there are other things that can happen. There may be Log events happening, and there may be incoming requests prompting the user for action, in different threads/routines.

So it is better to do do the printing only once, to ensure that the chunk of text that we want to display is displayed in one piece, not broken up by other messages/prompts. That can be done either by souping up the lines to write into a []string and then using strings.Join, or by using a strings.Builder.

This is something that I have not done a very good job of myself, in the rest of this file.

OK, 56e46c3 breaks the account listing into a separate func and cuts out whitespace as suggested. adc44ff minimizes print statements by bundling all the account info and titling into a single string.

Actually, ignore the second point. Let's just leave it be, and maybe we can fix all of those at the same time, in a separate PR

signer/core/cliui.go

holiman · 2022-11-07T10:49:51Z

signer/core/cliui.go

@@ -243,25 +243,30 @@ func (ui *CommandlineUI) OnApprovedTx(tx ethapi.SignTransactionResult) {
 	}
 }

-func (ui *CommandlineUI) OnSignerStartup(info StartupInfo) {
+func (ui *CommandlineUI) ShowAccounts() string {


Myeah, sure, that's one way of doing it. If you do this, then you should make it lowercase - so it's a non-public method showAccounts.

But the primary thing I wanted was to make it async. So instead of just calling

addresses := ui.ShowAccounts() fmt.Print("Accounts known to Clef:\n\n") fmt.Printf("%s", addresses)

You would do

go fun(){ addresses := ui.ShowAccounts() fmt.Print("Accounts known to Clef:\n\n") fmt.Printf("%s", addresses) }()

ah, yes ok: c1dbc02

holiman

Almost there now!

holiman · 2022-11-07T12:33:03Z

signer/core/cliui.go

+	go func() {
+		addresses := ui.showAccounts()
+		fmt.Printf("%s", addresses)
+	}()


If you change the semantics of showAccounts so it actually displays the info by itself (and not return anything), then this can be minimized into

Suggested change

go func() {

addresses := ui.showAccounts()

fmt.Printf("%s", addresses)

}()

go ui.showAccounts()

oh, yes that is much nicer. Done in 4131ddd

cmd/clef/main.go

holiman

LGTM

signer/core/cliui.go

fjl · 2022-11-07T15:43:02Z

How does this behave when a user has lots of accounts?

jmcook1186 · 2022-11-07T16:47:00Z

fair point - it will list them all. Might be good idea to restrict listing to < N accounts.

holiman · 2022-11-07T17:21:04Z

I just commented on #26128, but now I saw the discussion here. When a user has lots of accounts, it will show lots of info. Is that a problem?

Because if it is, the same problem will happen basically any time that the user is requested to sign anything...

holiman · 2022-11-07T17:22:27Z

I mean -- the CLI user-interface is not The Ultimate User Experience. It is meant to provide the basics, with as little fuss as possible.

PR #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>

Reports accounts known to Clef during startup, after master seed is provided by the user.

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>

jmcook1186 added 2 commits November 1, 2022 10:18

dumps account info at startup

600780f

fix accidental ctrl-v

d932ac6

jmcook1186 requested a review from holiman as a code owner November 1, 2022 10:24

holiman reviewed Nov 1, 2022

View reviewed changes

jmcook1186 added 2 commits November 1, 2022 18:08

fix account listing

155ea96

Merge branch 'master' of https://github.com/jmcook1186/go-ethereum in…

82df060

…to jc-clef3

+ account details to Startupinfo map for ui to print

1533a58

register API and call listAccounts from cliui

0176fa0

fix linting fix linting fix typo: accoutns -> accounts

holiman reviewed Nov 7, 2022

View reviewed changes

jmcook1186 added 2 commits November 7, 2022 09:47

account listing -> separate func, rm whitespace

56e46c3

minimize print statements by bundling info into str

adc44ff

holiman reviewed Nov 7, 2022

View reviewed changes

make call to showAccounts async

c1dbc02

holiman reviewed Nov 7, 2022

View reviewed changes

minimize go func, rm whitespace

4131ddd

holiman approved these changes Nov 7, 2022

View reviewed changes

holiman added this to the 1.11.0 milestone Nov 7, 2022

jmcook1186 commented Nov 7, 2022

View reviewed changes

signer/core/cliui.go Outdated Show resolved Hide resolved

jmcook1186 commented Nov 7, 2022

View reviewed changes

signer/core/cliui.go Outdated Show resolved Hide resolved

signer/core: minor changes in cliui account display

cffb417

holiman added the pr:merge label Nov 7, 2022

holiman merged commit 55a92fa into ethereum:master Nov 7, 2022

jmcook1186 mentioned this pull request Nov 7, 2022

cmd/clef: only print first N accounts on startup #26128

Merged

shekhirin pushed a commit to shekhirin/go-ethereum that referenced this pull request Jun 6, 2023

cmd/clef: list accounts at startup (ethereum#26082)

cae488c

Reports accounts known to Clef during startup, after master seed is provided by the user.

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

cmd/clef: list accounts at startup #26082

cmd/clef: list accounts at startup #26082

jmcook1186 commented Nov 1, 2022

holiman Nov 1, 2022

jmcook1186 Nov 1, 2022

holiman Nov 2, 2022

holiman Nov 2, 2022

holiman commented Nov 4, 2022

jmcook1186 commented Nov 6, 2022 •

edited

Loading

holiman left a comment

holiman Nov 7, 2022

jmcook1186 Nov 7, 2022

holiman Nov 7, 2022

holiman Nov 7, 2022

jmcook1186 Nov 7, 2022

holiman left a comment

holiman Nov 7, 2022

jmcook1186 Nov 7, 2022

holiman left a comment

fjl commented Nov 7, 2022

jmcook1186 commented Nov 7, 2022

holiman commented Nov 7, 2022

holiman commented Nov 7, 2022

cmd/clef: list accounts at startup #26082

cmd/clef: list accounts at startup #26082

Conversation

jmcook1186 commented Nov 1, 2022

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

holiman commented Nov 4, 2022

jmcook1186 commented Nov 6, 2022 • edited Loading

holiman 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

holiman left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

holiman left a comment

Choose a reason for hiding this comment

fjl commented Nov 7, 2022

jmcook1186 commented Nov 7, 2022

holiman commented Nov 7, 2022

holiman commented Nov 7, 2022

jmcook1186 commented Nov 6, 2022 •

edited

Loading