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

R4R: GaiaCLI - refactor/fix --account and --index #3461

Merged
merged 23 commits into from
Feb 5, 2019

Conversation

jleni
Copy link
Member

@jleni jleni commented Jan 31, 2019

This PR addresses #3427

There are still some usability concerns that might need to be addressed in another issue (maybe #1480)

  • Every time an address is created, a new mnemonic is generated, so hd wallets are not convenient and multiple mnemonics need to be remembered.
  • Given a known mnemonic, it is only possible to generate a few derived addresses by using --recover. This might now be intuitive to users.
  • It is not possible to generate a set of derived address.i.e. HD wallets from 40'/118'/0'/0/[1..10]

bip44path has been completely removed and all addresses follow the format: 44'/118'/[account]'/0/[index]

  • Targeted PR against correct branch (see CONTRIBUTING.md)

  • Linked to github-issue with discussion and accepted design OR link to spec that describes this work.

  • Wrote tests

  • Updated relevant documentation (docs/)

  • Added entries in PENDING.md with issue #

  • rereviewed Files changed in the github PR explorer


For Admin Use:

  • Added appropriate labels to PR (ex. wip, ready-for-review, docs)
  • Reviewers Assigned
  • Squashed all commits, uses message "Merge pull request #XYZ: [title]" (coding standards)

@jleni jleni changed the title WIP: GaiaCLI - refactor/fix --account and --index R4R: GaiaCLI - refactor/fix --account and --index Jan 31, 2019
Copy link
Member

@jackzampolin jackzampolin left a comment

Choose a reason for hiding this comment

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

Some initial comments. This should definately get some more eyes cc @cwgoes @liamsi @ebuchman @alexanderbez @alessio

client/keys/add.go Outdated Show resolved Hide resolved
client/keys/add.go Show resolved Hide resolved
client/keys/add.go Outdated Show resolved Hide resolved
client/keys/mnemonic.go Outdated Show resolved Hide resolved
client/keys/add.go Show resolved Hide resolved
client/keys/utils.go Outdated Show resolved Hide resolved
client/lcd/test_helpers.go Outdated Show resolved Hide resolved
cmd/gaia/cli_test/cli_test.go Show resolved Hide resolved
crypto/keys/hd/hdpath.go Show resolved Hide resolved
crypto/keys/keybase.go Show resolved Hide resolved
client/keys/add.go Outdated Show resolved Hide resolved
client/keys/add.go Outdated Show resolved Hide resolved
client/keys/add.go Outdated Show resolved Hide resolved
client/keys/add.go Outdated Show resolved Hide resolved
@jleni
Copy link
Member Author

jleni commented Feb 1, 2019

Tagging with REST as the PR end up affecting REST in multiple places

@jleni
Copy link
Member Author

jleni commented Feb 1, 2019

mm.. test_cover is running tests in parallel and currently lcd_test.go cannot handle that.. :(

It seems it is time to fix this:

// TODO: Make InitializeTestLCD safe to call in multiple tests at the same time
// InitializeTestLCD starts Tendermint and the LCD in process, listening on

@jleni
Copy link
Member Author

jleni commented Feb 1, 2019

Maybe this is not the case, I need to investigate a bit more

Copy link
Member

@jackzampolin jackzampolin left a comment

Choose a reason for hiding this comment

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

This looks G2G to me. Just one minor nit in my comments. Thanks for taking a stab at cleaning this code up @jleni !

client/keys/add.go Outdated Show resolved Hide resolved
client/types.go Outdated Show resolved Hide resolved
client/types.go Outdated Show resolved Hide resolved
crypto/keys/hd/hdpath.go Show resolved Hide resolved
crypto/keys/hd/hdpath.go Show resolved Hide resolved
@jleni
Copy link
Member Author

jleni commented Feb 1, 2019

This looks G2G to me. Just one minor nit in my comments. Thanks for taking a stab at cleaning this code up @jleni !

Happy to help with this too!

client/keys/mnemonic.go Outdated Show resolved Hide resolved
client/types.go Outdated Show resolved Hide resolved
client/utils/rest.go Outdated Show resolved Hide resolved
@alessio
Copy link
Contributor

alessio commented Feb 5, 2019

Approved - pending conflicts resolution

@jackzampolin
Copy link
Member

@jleni will merge once merge conflicts are fixed!

@jleni
Copy link
Member Author

jleni commented Feb 5, 2019

ok! merged/etc.
It is interesting that this PR never received a test coverage report...
anyway, good to go

@jackzampolin jackzampolin merged commit f5ada58 into cosmos:develop Feb 5, 2019
@jleni jleni deleted the jleni/gaia_args branch February 5, 2019 16:38
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants