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.

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

Usage for -ethKeystorePath option is confusing #2691

Closed
joegraviton opened this issue Dec 15, 2022 · 2 comments · Fixed by #2713
Closed

Usage for -ethKeystorePath option is confusing #2691

joegraviton opened this issue Dec 15, 2022 · 2 comments · Fixed by #2713

Comments

@joegraviton
Copy link

Describe the bug

The name and cli help for this option is unclear and misleading.

To Reproduce
Steps to reproduce the behavior:

  1. In cli help:
  -ethKeystorePath string
    	Path for the Eth Key
  1. When I read this, I am not sure what I should provide.

From the name ethKeystorePath, it looks like I should provide a dir path like:

/root/.lpData/keystore

But, from the help msg, it looks like I should provide a file path, like:

/root/.lpData/keystore/UTC--2022-12-03T10-31-30.841732443Z--6710ff8ab1d776097a6892c181964683442507b2`

This is too lengthy, I believe most users would rather not to provide it.

  1. actual behavior

If I provide /root/.lpData/keystore, the actually dir code will look into will be /root/.lpData/, which is unexpcted.
To get this right, I must provide the path with trailing /: /root/.lpData/keystore/.

If I provide the lengthy key file path, code will look into the parent dir, the actual key file name is ignored.
If you have multiple keys in that dir, you will need to specify it again, with -ethAcctAddr. Also unexpected.

related source code:

var keystoreDir string
if _, err := os.Stat(*cfg.EthKeystorePath); !os.IsNotExist(err) {
keystoreDir, _ = filepath.Split(*cfg.EthKeystorePath)
} else {
keystoreDir = filepath.Join(*cfg.Datadir, "keystore")
}

If the path exists, either it's dir or file, the path is split, which is not reasonable.

Expected behavior

This option should be a path to a keystore dir, no matter it has trailing / or not.

@eliteprox
Copy link
Contributor

@joegraviton Let me know what you think of this solution #2713

ethKeystorePath will now accept either a directory or file without issue.

When a file is provided, ethAcctAddr will be set automatically so the extra flag is no longer needed. The parent directory is used as the "keystore path" for additional accounts.

When a folder is provided, the exact directory is used for keystore path and ethAcctAddr can be used to select an account for backwards compatibility.

@joegraviton
Copy link
Author

@eliteprox That's perfect, thank you !

@leszko leszko added status: community working on it in progress and removed status: triage this issue has not been evaluated yet labels Jan 10, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
3 participants