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

Update auth cli commands #6717

Merged
merged 72 commits into from
Jul 29, 2020
Merged

Update auth cli commands #6717

merged 72 commits into from
Jul 29, 2020

Conversation

sahith-narahari
Copy link
Contributor

Description

Ref: #6213
This is part of breakdown of #6216 into smaller PRs


Before we can merge this PR, please make sure that all the following items have been
checked off. If any of the checklist items are not applicable, please leave them but
write a little note why.

  • Targeted PR against correct branch (see CONTRIBUTING.md)
  • Linked to Github issue with discussion and accepted design OR link to spec that describes this work.
  • Code follows the module structure standards.
  • Wrote unit and integration tests
  • Updated relevant documentation (docs/) or specification (x/<module>/spec/)
  • Added relevant godoc comments.
  • Added a relevant changelog entry to the Unreleased section in CHANGELOG.md
  • Re-reviewed Files changed in the Github PR explorer
  • Review Codecov Report in the comment section below once CI passes

@aaronc aaronc mentioned this pull request Jul 14, 2020
27 tasks
@codecov
Copy link

codecov bot commented Jul 14, 2020

Codecov Report

Merging #6717 into master will increase coverage by 6.04%.
The diff coverage is 57.74%.

@@            Coverage Diff             @@
##           master    #6717      +/-   ##
==========================================
+ Coverage   55.60%   61.64%   +6.04%     
==========================================
  Files         457      511      +54     
  Lines       27440    31893    +4453     
==========================================
+ Hits        15257    19662    +4405     
+ Misses      11083    10676     -407     
- Partials     1100     1555     +455     

@lgtm-com
Copy link

lgtm-com bot commented Jul 15, 2020

This pull request introduces 1 alert when merging d1eb8c1 into 6f928e1 - view on LGTM.com

new alerts:

  • 1 for Useless assignment to local variable

@blushi blushi mentioned this pull request Jul 16, 2020
9 tasks
@aaronc
Copy link
Member

aaronc commented Jul 29, 2020

One comment I would make is that it has been suggested that we should use different encoding and application types and convert between the two. SignatureV2 is an example of that and it is a little more ergonomic in go code than the protobuf generated SignatureDescriptor. So having these two types is an attempt to practice this separation of types. I have also questioned the value of this separation and the overhead of that approach is I think evident here.

@aaronc
Copy link
Member

aaronc commented Jul 29, 2020

One other subtle difference is between the PubKey interface and PublicKey type. Just that part alone would make moving from SignatureV2 to SignatureDescriptor in APIs quite cumbersome.

Copy link
Contributor

@amaury1093 amaury1093 left a comment

Choose a reason for hiding this comment

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

it has been suggested that we should use different encoding and application types and convert between the two

OK, this point also makes sense. in this case I'll approve, the migration to use the new TxConfig looks good 🎉

I still think some work can be done in terms of clarity with all these signing types, though maybe it's as simple as better naming. I can take a look in the next couple of days, after this PR gets merged.

@aaronc
Copy link
Member

aaronc commented Jul 29, 2020

I still think some work can be done in terms of clarity with all these signing types, though maybe it's as simple as better naming. I can take a look in the next couple of days, after this PR gets merged.

Thanks @amaurymartiny. That would be great.

client/cmd.go Outdated Show resolved Hide resolved
// SignatureDataToSignatureDescriptorData converts a SignatureData to SignatureDescriptor_Data.
// SignatureDescriptor_Data is considered an encoding type whereas SignatureData is used for
// business logic.
func SignatureDataToSignatureDescriptorData(data SignatureData) *SignatureDescriptor_Data {
Copy link
Contributor

Choose a reason for hiding this comment

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

I do agree here. I'd go with what @amaurymartiny stated or at the very least, change the nomenclature because it's confusing and the types clobber each other.

x/auth/client/cli/decode.go Outdated Show resolved Hide resolved
x/auth/client/cli/decode.go Outdated Show resolved Hide resolved
x/auth/client/cli/encode.go Outdated Show resolved Hide resolved
x/auth/client/cli/tx_sign.go Outdated Show resolved Hide resolved
x/auth/client/cli/tx_sign.go Outdated Show resolved Hide resolved
x/auth/client/cli/tx_sign.go Show resolved Hide resolved
@lgtm-com
Copy link

lgtm-com bot commented Jul 29, 2020

This pull request introduces 1 alert when merging 423cbcb into 2da954f - view on LGTM.com

new alerts:

  • 1 for Useless assignment to local variable

@lgtm-com
Copy link

lgtm-com bot commented Jul 29, 2020

This pull request introduces 1 alert when merging 0136c11 into 2da954f - view on LGTM.com

new alerts:

  • 1 for Useless assignment to local variable

@alexanderbez alexanderbez mentioned this pull request Jul 29, 2020
9 tasks
x/auth/client/cli/encode.go Outdated Show resolved Hide resolved
@aaronc
Copy link
Member

aaronc commented Jul 29, 2020

@alexanderbez I think we've addressed your comments here.

@aaronc aaronc added the A:automerge Automatically merge PR once all prerequisites pass. label Jul 29, 2020
@mergify mergify bot merged commit 72ebafe into master Jul 29, 2020
@mergify mergify bot deleted the sahith/update-auth-cli branch July 29, 2020 22:33
@clevinson clevinson mentioned this pull request Oct 2, 2020
8 tasks
larry0x pushed a commit to larry0x/cosmos-sdk that referenced this pull request May 22, 2023
* add utils

* update sign cmd

* update multisign cmd

* update sign batch cmd

* update genutil cmd

* add wrap tx builder

* update gentx cli

* update validate sigs cmd

* fix lint

* add flag reader to cli

* update marshaler for batchscan

* add register query server

* update to master

* remove depricated methods

* fix keyring issue

* update wraptx

* Fix batch scan

* fix register interfaces issue

* update printOutput

* Update Validate Sigs test

* WIP on signature JSON

* Cleanup

* Cleanup

* Test fixes

* Test fixes

* Fixes

* WIP on tests

* Fix gov tests

* fix lint

* fix MultiSign tests

* fix tests

* refactor tests

* Cleanup

* Address review comments

* Update encode

* Test fix

* Rename SignData func

* Fix lint

* Fix lint

* Revert ReadTxCommandFlags

Co-authored-by: Aaron Craelius <aaron@regen.network>
Co-authored-by: Aaron Craelius <aaronc@users.noreply.github.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A:automerge Automatically merge PR once all prerequisites pass. C:x/auth
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants