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.

Sign up for GitHub

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

Jump to bottom

R4R: Support adding offline pubkeys to gaiacli keys #3202

Closed
wants to merge 6 commits into from
Closed

R4R: Support adding offline pubkeys to gaiacli keys #3202

wants to merge 6 commits into from

Conversation

zmanian
Copy link
Member

@zmanian zmanian commented Dec 29, 2018 •
edited by alexanderbez
Loading

Support adding offline public keys to gaiacli.

Closes #3197

  • 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)

@codecov
Copy link

codecov bot commented Dec 29, 2018

Codecov Report

Merging #3202 into develop will decrease coverage by 0.03%.
The diff coverage is 0%.

@@            Coverage Diff             @@
##           develop   #3202      +/-   ##
==========================================
- Coverage    54.83%   54.8%   -0.04%     
==========================================
  Files          133     133              
  Lines         9559    9568       +9     
==========================================
+ Hits          5242    5244       +2     
- Misses        3996    4003       +7     
  Partials       321     321

@codecov
Copy link

codecov bot commented Dec 29, 2018 •
edited
Loading

Codecov Report

Merging #3202 into develop will decrease coverage by 0.05%.
The diff coverage is 0%.

@@             Coverage Diff             @@
##           develop    #3202      +/-   ##
===========================================
- Coverage    54.83%   54.78%   -0.06%     
===========================================
  Files          133      133              
  Lines         9559     9568       +9     
===========================================
  Hits          5242     5242              
- Misses        3996     4005       +9     
  Partials       321      321

@alexanderbez alexanderbez changed the title (WIP) Support adding offline pubkeys to gaiacli keys R4R: Support adding offline pubkeys to gaiacli keys Jan 2, 2019
alexanderbez
alexanderbez suggested changes Jan 2, 2019
View reviewed changes
Copy link
Contributor

@alexanderbez alexanderbez left a comment

Choose a reason for hiding this comment

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

testedACK -- left some minor feedback 👍

client/keys/add.go Outdated
@@ -7,8 +7,8 @@ import (
"net/http"
"os"

"github.com/cosmos/go-bip39"
"github.com/gorilla/mux"
"github.com/cosmos/go-bip39"
Copy link
Contributor

Choose a reason for hiding this comment

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

Invalid formatting here.

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
alexanderbez and others added 2 commits January 2, 2019 11:53
@alexanderbez @zmanian
Update client/keys/add.go
fa1924f 
Co-Authored-By: zmanian <zaki@manian.org>
@alexanderbez @zmanian
Update client/keys/add.go
544c51c 
Co-Authored-By: zmanian <zaki@manian.org>
alessio
alessio reviewed Jan 2, 2019
View reviewed changes
Copy link
Contributor

@alessio alessio left a comment

Choose a reason for hiding this comment

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

Few comments, nothing really major.
It'd be nice to see a CLI test case as well

client/keys/add.go Outdated Show resolved Hide resolved
client/keys/add.go Show resolved Hide resolved
@jackzampolin
Copy link
Member

Ok addressed comments @alexanderbez and @alessio

alexanderbez
alexanderbez suggested changes Jan 3, 2019
View reviewed changes
Copy link
Contributor

@alexanderbez alexanderbez left a comment

Choose a reason for hiding this comment

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

Thanks @jackzampolin -- one more minor bit of feedback.

client/keys/add.go Outdated Show resolved Hide resolved
client/keys/add.go Outdated Show resolved Hide resolved
alexanderbez
alexanderbez reviewed Jan 3, 2019
View reviewed changes
Copy link
Contributor

@alexanderbez alexanderbez left a comment •
edited
Loading

Choose a reason for hiding this comment

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

LGTM 👍

alexanderbez
alexanderbez suggested changes Jan 3, 2019
View reviewed changes
Copy link
Contributor

@alexanderbez alexanderbez left a comment •
edited
Loading

Choose a reason for hiding this comment

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

Changes LGTM but CI is failing.

@alessio
Copy link
Contributor

alessio commented Jan 3, 2019

It'd be nice to have a command that, given N local keys, a threshold parameter K, and a name NAME, it constructs a multisig key and store it as NAME. I could take care of it in another PR though

alessio
alessio suggested changes Jan 3, 2019
View reviewed changes
client/keys/add.go
@@ -7,7 +7,6 @@ import (
"net/http"
"os"

"github.com/cosmos/go-bip39"
Copy link
Contributor

Choose a reason for hiding this comment

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

I think this should be reverted

@jackzampolin jackzampolin mentioned this pull request Jan 4, 2019
Merged
3 tasks
@jackzampolin
Copy link
Member

This PR is running into import path issues locally for me. I've moved these changes over to a branch on this repo. Closing in favor of #3224

@cwgoes cwgoes closed this Jan 4, 2019
@jackzampolin jackzampolin deleted the zaki/add_offline_pubkeys branch January 7, 2019 16:16
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@jackzampolin jackzampolin jackzampolin left review comments

@alexanderbez alexanderbez alexanderbez requested changes

@alessio alessio alessio requested changes

@cwgoes cwgoes Awaiting requested review from cwgoes

@ebuchman ebuchman Awaiting requested review from ebuchman

@rigelrozanski rigelrozanski Awaiting requested review from rigelrozanski

Assignees
No one assigned
Labels
C:CLI C:Keys Keybase, KMS and HSMs
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
5 participants
@zmanian @jackzampolin @alessio @alexanderbez @cwgoes