Skip to content

Fix issue with geth account new#15529

Merged
fjl merged 5 commits into
ethereum:masterfrom
holiman:account_fix_2
Nov 20, 2017
Merged

Fix issue with geth account new#15529
fjl merged 5 commits into
ethereum:masterfrom
holiman:account_fix_2

Conversation

@holiman
Copy link
Copy Markdown
Contributor

@holiman holiman commented Nov 20, 2017

This PR makes geth account new operate directly on a keystore file, instead of using a complete keystore.

The first commit should only involve the geth account new command, whereas the second commit removes some code which is also used in other circumstances. As far as I could tell, it should be the same functionality, but the ephemeral string has been removed.

return key, nil
}

func StoreKey(dir, auth string, scryptN, scryptP int) (error, common.Address) {
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

dox pls :)

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

And reverse the return parameters. Error should always be the last return.

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Please use

func StoreKey(dir, auth string, scryptN, scryptP int) (common.Address, error) {
    _, a, err := storeNewKey(&keyStorePassphrase{dir, scryptN, scryptP}, crand.Reader, passphrase)
   return a.Address, err
}

}
fullpath := filepath.Join(dir, keyFileName(key.Address))
err = writeKeyFile(fullpath, keyjson)
if err != nil {
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

A bit nicer would be if err = writeKeyFile(fullpath, keyjson); err != nil {

Comment thread cmd/geth/accountcmd.go Outdated
// accountCreate creates a new account into the keystore defined by the CLI flags.
func accountCreate(ctx *cli.Context) error {
stack, _ := makeConfigNode(ctx)

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pls no empty line after a method header.

Comment thread node/config.go Outdated

func makeAccountManager(conf *Config) (*accounts.Manager, string, error) {
// ResolveAccountConfig determines the settings for scrypt and keydirectory
func ResolveAccountConfig(conf *Config) (int, int, string, error) {
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why not just make ResolveAccountConfig a method on Config? Also I'd rename to simply AccountConfig.

Comment thread node/config.go Outdated
}
}
return accounts.NewManager(backends...), ephemeral, nil
return accounts.NewManager(backends...), keydir, nil
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Take care with this! The return parameter is the path only for ephemeral stores that the node will delete on shutting down!

return key, nil
}

func StoreKey(dir, auth string, scryptN, scryptP int) (error, common.Address) {
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Please use

func StoreKey(dir, auth string, scryptN, scryptP int) (common.Address, error) {
    _, a, err := storeNewKey(&keyStorePassphrase{dir, scryptN, scryptP}, crand.Reader, passphrase)
   return a.Address, err
}

Comment thread cmd/geth/accountcmd.go Outdated
func accountCreate(ctx *cli.Context) error {
stack, _ := makeConfigNode(ctx)

cfg := gethConfig{}
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Please add Node: defaultNodeConfig(). That's the one that initializes the default datadir.

Comment thread cmd/geth/accountcmd.go

if err != nil {
utils.Fatalf("Failed to create account: %v", err)
}
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

"Failed to create account" ?

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sloppy paste error..

Copy link
Copy Markdown
Member

@karalabe karalabe left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

@karalabe karalabe added this to the 1.7.3 milestone Nov 20, 2017
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants