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

Drop on-disk keybase in favor of keyring #164

Merged
merged 32 commits into from
Nov 18, 2019
Merged

Conversation

alessio
Copy link
Contributor

@alessio alessio commented Oct 17, 2019

This changeset aims to adapt gaia to the changes introduced in
cosmos/cosmos-sdk#5180

Rationale for this work is articulated in


  • Linked to github-issue with discussion and accepted design OR link to spec that describes this work.
  • Wrote tests
  • Updated relevant documentation (docs/)
  • Added a relevant changelog entry to the Unreleased section in CHANGELOG.md
  • Reviewed 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 PR #XYZ: [title]" (coding standards)

@alessio alessio changed the title Alessio/switch to keyring Drop on-disk keybase in favor of keyring Oct 17, 2019
@alessio alessio marked this pull request as ready for review October 17, 2019 21:30
@codecov-io
Copy link

codecov-io commented Oct 17, 2019

Codecov Report

Merging #164 into master will increase coverage by 0.74%.
The diff coverage is 100%.

@@            Coverage Diff             @@
##           master     #164      +/-   ##
==========================================
+ Coverage   65.72%   66.46%   +0.74%     
==========================================
  Files           5        5              
  Lines         493      492       -1     
==========================================
+ Hits          324      327       +3     
+ Misses        138      134       -4     
  Partials       31       31

@alessio
Copy link
Contributor Author

alessio commented Oct 17, 2019

This is ready for preliminary review, although it should not be merged til cosmos/cosmos-sdk#5180 is merged first

@alexanderbez alexanderbez removed the R4R label Oct 18, 2019
@fedekunze
Copy link
Collaborator

@alessio is this R4R?

@alessio
Copy link
Contributor Author

alessio commented Nov 6, 2019

Yes @fedekunze, it is de facto R4R. I didn't tag it yet as such 'cause I was waiting for the SDK's respective PR to be merged first.

EDIT: I actually tagged it, @alexanderbez rightly removed the tag because it was blocked on the SDK's PR. But please feel free to review it, it's in fact ready and should be used to test the new keyring integration.

Copy link
Collaborator

@fedekunze fedekunze left a comment

Choose a reason for hiding this comment

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

Changes LGTM, I have some questions only. Once those are replied/addressed I'll merge the implementation on the SDK

lcd_test/helpers.go Show resolved Hide resolved
lcd_test/helpers_test.go Show resolved Hide resolved
lcd_test/helpers_test.go Show resolved Hide resolved
lcd_test/helpers_test.go Show resolved Hide resolved
cmd/gaiad/testnet.go Show resolved Hide resolved
@alessio
Copy link
Contributor Author

alessio commented Nov 8, 2019

FYI I've carried out some code clean up.

Copy link
Collaborator

@fedekunze fedekunze 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. @alessio mind updating this branch to latest SDK master?

@alessio alessio added R4R and removed WIP labels Nov 15, 2019
@alessio
Copy link
Contributor Author

alessio commented Nov 15, 2019

Will update tests ASAP

@alessio
Copy link
Contributor Author

alessio commented Nov 17, 2019

All sorted @fedekunze

Copy link
Collaborator

@fedekunze fedekunze left a comment

Choose a reason for hiding this comment

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

ACK. Thanks @alessio. We’ll probably have to write down the key migration guides for the next major release (cosmoshub-4) wiki and more docs for each OS.

@fedekunze fedekunze merged commit 71f3b4f into master Nov 18, 2019
@alessio alessio deleted the alessio/switch-to-keyring branch November 18, 2019 18:47
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.

4 participants