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

gaiacli: Make recovery allow new keys #1477

Merged
merged 5 commits into from
Jun 29, 2018
Merged

gaiacli: Make recovery allow new keys #1477

merged 5 commits into from
Jun 29, 2018

Conversation

ValarDragon
Copy link
Contributor

Closes #1474

  • Updated all relevant documentation in docs
  • Updated all code comments where relevant
  • Wrote tests
  • Updated CHANGELOG.md - n/a, bug fix that was introduced in this PR
  • Updated Gaia/Examples
  • Squashed all commits, uses message "Merge pull request #XYZ: [title]" (coding standards)
  • Added appropriate labels to PR (ex. wip, ready-for-review, docs)

@cwgoes
Copy link
Contributor

cwgoes commented Jun 29, 2018

I think we want to use CreateFundraiserKey here; it just doesn't really fit with the current LCD API.

cc @liamsi

@ValarDragon
Copy link
Contributor Author

Would you prefer reverting CreateFundraiserKey, and making seperate methods "Create Key" and "Create FreshKey"?

@codecov
Copy link

codecov bot commented Jun 29, 2018

Codecov Report

Merging #1477 into develop will not change coverage.
The diff coverage is 0%.

@@           Coverage Diff            @@
##           develop    #1477   +/-   ##
========================================
  Coverage    63.09%   63.09%           
========================================
  Files          118      118           
  Lines         6563     6563           
========================================
  Hits          4141     4141           
  Misses        2151     2151           
  Partials       271      271

@cwgoes
Copy link
Contributor

cwgoes commented Jun 29, 2018

Ah, sorry, I mean that we want to use CreateMnemonic in the usual case and CreateFundraiserKey only for fundraiser keys.

@ValarDragon
Copy link
Contributor Author

Create Mnemonic wouldn't work here, that derives the mnemonic. We need to go from mnemonic to privkey. I don't understand the rationale behind having a seperate CreateFundraiserKey method as opposed to a generalized CreateKey method, since the functionality is exactly the same, and any downstream application can just run CreateKey. (i.e. LCD endpoints could just call CreateKey with the exact same input)

@cwgoes
Copy link
Contributor

cwgoes commented Jun 29, 2018

Create Mnemonic wouldn't work here, that derives the mnemonic. We need to go from mnemonic to privkey. I don't understand the rationale behind having a seperate CreateFundraiserKey method as opposed to a generalized CreateKey method, since the functionality is exactly the same, and any downstream application can just run CreateKey. (i.e. LCD endpoints could just call CreateKey with the exact same input)

Exactly; I don't think we need to expose two separate methods for creating a mnemonic and then deriving a key from the mnemonic, ref #1442. For deriving keys from previously created mnemonics, we can use the Derive method.

@ValarDragon
Copy link
Contributor Author

ValarDragon commented Jun 29, 2018

Decided to keep CreateFundraiserkey, and temporarily create a new CreateKey method just to resolve the #1474 bug. The real solution will happen as a result of #1480

Copy link
Contributor

@cwgoes cwgoes left a comment

Choose a reason for hiding this comment

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

Tested ACK, thanks!

@cwgoes cwgoes merged commit fc3dd56 into develop Jun 29, 2018
@cwgoes cwgoes deleted the dev/fix_recovery branch June 29, 2018 22:47
adrianbrink pushed a commit that referenced this pull request Jul 2, 2018
* gaiacli: Make recovery allow new keys
* Move create key to a temporary method, restore create fundraiser key
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.

gaiacli can't recover from 24 words
2 participants