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: CLI keybase-sig -> identity #1818

Merged
merged 2 commits into from
Jul 25, 2018
Merged

R4R: CLI keybase-sig -> identity #1818

merged 2 commits into from
Jul 25, 2018

Conversation

rigelrozanski
Copy link
Contributor

also fixes #1766

  • Linked to github-issue with discussion and accepted design
  • Updated all relevant documentation (docs/)
  • Updated all relevant code comments
  • [n/a] Wrote tests
  • Added entries in PENDING.md
  • Updated cmd/gaia and examples/

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)

@rigelrozanski rigelrozanski added T:Docs Changes and features related to documentation. ready-for-review T: API Breaking Breaking changes that impact APIs and the SDK only (not state machine). labels Jul 25, 2018
@codecov
Copy link

codecov bot commented Jul 25, 2018

Codecov Report

Merging #1818 into develop will not change coverage.
The diff coverage is n/a.

@@           Coverage Diff            @@
##           develop    #1818   +/-   ##
========================================
  Coverage    63.44%   63.44%           
========================================
  Files          117      117           
  Lines         6937     6937           
========================================
  Hits          4401     4401           
  Misses        2281     2281           
  Partials       255      255

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.

LGTM 👌

@ValarDragon
Copy link
Contributor

ValarDragon commented Jul 25, 2018

We may need to plan the abstraction from keybase to any form of identity a bit more.

Do we need domain seperators? If not, how would end services tell what service its for (e.g. voyager)? Maybe its fine for the cases of uport and keybase (idk how uport authenticates), but I can easily imagine situations where its unclear, or even worse, situations where that identity corresponds to different identities on the different services.

@rigelrozanski
Copy link
Contributor Author

@ValarDragon Identity should be verified outside of cosmos - we simply want to provide a generic space for third party identity verification. We don't want to fork for every single new identity verification technique

@ValarDragon
Copy link
Contributor

I don't think supporting multiple forms of identity is that useful unless we have an ICS standard for verification techniques, or require that all verification identifiers have some domain seperation. (Which I'm unsure if keybase does?)

@cwgoes
Copy link
Contributor

cwgoes commented Jul 25, 2018

I don't think supporting multiple forms of identity is that useful unless we have an ICS standard for verification techniques, or require that all verification identifiers have some domain seperation. (Which I'm unsure if keybase does?)

We don't "support" or "not support" any forms of identity with this change - it's just a string, which validators can use to link to whatever external identity they want. UI conventions may emerge on top, but it's preferable that those don't need to depend on the state machine.

@ValarDragon
Copy link
Contributor

ValarDragon commented Jul 25, 2018

Hrmm. I feel like we should come out with a UI convention here pretty soon. (We don't really want this to be hard to change later due to competing UI conventions)

We could just use amino encoding w/ prefixes.

This doesn't need to block this PR, but I think its an ICS standard which we should write soon. (Granted this isn't the correct place for ICS standard discussions)

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

@cwgoes cwgoes merged commit d6cd0d4 into develop Jul 25, 2018
@cwgoes cwgoes deleted the rigel/keybasesig2identity branch July 25, 2018 18:43
@cwgoes
Copy link
Contributor

cwgoes commented Jul 25, 2018

This doesn't need to block this PR, but I think its an ICS standard which we should write soon. (Granted this isn't the correct place for ICS standard discussions)

Agreed - let's think about and/or draft such an ICS standard. I think we also want input from potential validators & delegators.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
C:x/staking T: API Breaking Breaking changes that impact APIs and the SDK only (not state machine). T:Docs Changes and features related to documentation.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Glitch in sample command gaiacli stake edit-validator
4 participants