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: Make gaiacli keys show multisig-ready #2554

Merged
merged 10 commits into from
Oct 24, 2018
Merged

R4R: Make gaiacli keys show multisig-ready #2554

merged 10 commits into from
Oct 24, 2018

Conversation

alessio
Copy link
Contributor

@alessio alessio commented Oct 22, 2018 •
edited
Loading

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

@alessio alessio changed the title WIP: Make gaiacli keys show multisig-ready R4R: Make gaiacli keys show multisig-ready Oct 22, 2018
@alessio alessio requested a review from zramsay as a code owner October 22, 2018 20:43
@codecov
Copy link

codecov bot commented Oct 22, 2018 •
edited
Loading

Codecov Report

Merging #2554 into develop will decrease coverage by 0.02%.
The diff coverage is n/a.

@@            Coverage Diff             @@
##           develop   #2554      +/-   ##
==========================================
- Coverage    60.32%   60.3%   -0.03%     
==========================================
  Files          150     150              
  Lines         8613    8613              
==========================================
- Hits          5196    5194       -2     
- Misses        3069    3071       +2     
  Partials       348     348

@alessio alessio added the C:CLI label Oct 23, 2018
@alexanderbez alexanderbez changed the title R4R: Make gaiacli keys show multisig-ready R4R: Make gaiacli keys show multisig-ready Oct 23, 2018
alexanderbez
alexanderbez suggested changes Oct 23, 2018
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 @alessio! Left some minor feedback :)

client/keys/show.go Outdated
@@ -18,30 +22,67 @@ const (
FlagPublicKey = "pubkey"
// FlagBechPrefix defines a desired Bech32 prefix encoding for a key.
FlagBechPrefix = "bech"

flagMultiSigThreshold = "multisig-threshold"
Copy link
Contributor

Choose a reason for hiding this comment

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

Is there a case where we'd want this exposed?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I there is presently no case for it to be exposed.

client/keys/show.go Outdated Show resolved Hide resolved
client/keys/show.go
)

var _ keys.Info = (keys.Info)(nil)

type multiSigKey struct {
Copy link
Contributor

Choose a reason for hiding this comment

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

Will multiSigKey be used elsewhere, besides just via show? If so, may I suggest to moving it to it's own file, even though it's fairly small.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

It's presently not used elsewhere.

client/keys/show.go Outdated Show resolved Hide resolved
client/keys/show.go Outdated Show resolved Hide resolved
sunnya97
sunnya97 reviewed Oct 24, 2018
View reviewed changes
docs/sdk/clients.md Outdated
gaiacli show -m K key1 key2...keyK
```

`K` is the minimum number of private keys that must have signed the transactions that carry the generated public key.
Copy link
Member

Choose a reason for hiding this comment

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

Should be minimum weight, and each shown key should show its own weight.

alexanderbez
alexanderbez approved these changes Oct 24, 2018
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.

utACK 👍

cwgoes
cwgoes approved these changes Oct 24, 2018
View reviewed changes
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.

utACK

I think this is useful, and we can merge it, but I don't think this is sufficient. I think we'll also want users to be able to store named aliases of multisignature pubkeys in their local account DB - for easy tracking, minimal risk of mistakes, etc. - worth discussing the exact UX we want in a future issue.

@cwgoes cwgoes merged commit c577831 into develop Oct 24, 2018
@cwgoes cwgoes deleted the alessio/multisig-show-cmd branch October 24, 2018 13:24
@cwgoes cwgoes mentioned this pull request Oct 24, 2018
Closed
@alessio
Copy link
Contributor Author

alessio commented Oct 24, 2018

One more thing to keep in mind: this does not handle multisig keys recursively, e.g. keyA = k{B + C} with C = k{D + E}, though I doubt that that would be helpful

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@fedekunze fedekunze fedekunze left review comments

@sunnya97 sunnya97 sunnya97 left review comments

@cwgoes cwgoes cwgoes approved these changes

@alexanderbez alexanderbez alexanderbez approved these changes

@ebuchman ebuchman Awaiting requested review from ebuchman

@rigelrozanski rigelrozanski Awaiting requested review from rigelrozanski

@zramsay zramsay Awaiting requested review from zramsay

@jaekwon jaekwon Awaiting requested review from jaekwon

Assignees
No one assigned
Labels
C:CLI
Projects
None yet
Milestone
No milestone
Development

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