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

x/auth: turn sign --validate-signatures into a standalone command #6108

Merged
merged 2 commits into from
May 4, 2020
Merged

x/auth: turn sign --validate-signatures into a standalone command #6108

merged 2 commits into from
May 4, 2020

Conversation

alessio
Copy link
Contributor

@alessio alessio commented Apr 30, 2020 •
edited
Loading

--validate-signatures should not be a flag of the sign command
as the operation performed (transaction signatures verification)
is logically distinct.

cli_test is and has always been an horrible name for package
directory as it's very much Go anti-idiomatic - _test is the
suffix used by test packages, not directories. Plus, CLI test
cases can and should live alongside other testcases that don't
require binaries to be built beforehand. Thus:

x/module/client/cli_test/*.go -> x/module/client/cli/

Test files that require sim{cli,d} shall be tagged with // +build cli_test

With regard to cli test auxiliary functions, they should live in:

x/module/client/testutil/

For contributor use:

  • 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

For admin use:

  • Added appropriate labels to PR (ex. WIP, R4R, docs, etc)
  • Reviewers assigned
  • Squashed all commits, uses message "Merge pull request #XYZ: [title]" (coding standards)

@codecov
Copy link

codecov bot commented Apr 30, 2020 •
edited
Loading

Codecov Report

Merging #6108 into master will increase coverage by 0.18%.
The diff coverage is 0.00%.

@@            Coverage Diff             @@
##           master    #6108      +/-   ##
==========================================
+ Coverage   54.64%   54.83%   +0.18%     
==========================================
  Files         443      439       -4     
  Lines       26655    26492     -163     
==========================================
- Hits        14566    14527      -39     
+ Misses      11092    10968     -124     
  Partials      997      997

@alessio alessio marked this pull request as ready for review April 30, 2020 22:55
@alessio alessio added the R4R label Apr 30, 2020
@alessio alessio requested a review from jgimeno April 30, 2020 22:55
@alessio alessio force-pushed the alessio/turn-validate-signatures-into-standalone-cmd branch from 4f6e86c to e45f99e Compare April 30, 2020 23:20
@jgimeno jgimeno changed the title x/auth: turn sign --validate-sigantures into a standalone command x/auth: turn sign --validate-signatures into a standalone command May 1, 2020
@jgimeno
Copy link
Contributor

jgimeno commented May 1, 2020

Now is a good moment to add a test for the Validate, should not be a difficult one with a hardcoded signature in the test.

alexanderbez
alexanderbez approved these changes May 1, 2020
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

alexanderbez
alexanderbez suggested changes May 1, 2020
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.

Actually, we should add a test for this.

@aaronc
Copy link
Member

aaronc commented May 1, 2020

Yeah l second that. Would be great to leverage the new modular integration tests we have to cover this

@alessio
Copy link
Contributor Author

alessio commented May 1, 2020

@alexanderbez @aaronc please have a look at 3644ad0, I'd appreciate feedback

@alessio alessio requested a review from alexanderbez May 1, 2020 21:47
@aaronc
Copy link
Member

aaronc commented May 1, 2020

@alessio l think that approach makes sense 👍

@alessio alessio requested review from tac0turtle and aaronc May 2, 2020 08:32
@alessio alessio requested a review from clevinson as a code owner May 2, 2020 13:44
@alessio alessio force-pushed the alessio/turn-validate-signatures-into-standalone-cmd branch from a2e2f23 to 86517f9 Compare May 2, 2020 15:17
x/auth: turn sign --validate-sigantures into a standalone command
9586d6f 
--validate-signatures should not be a flag of the sign command
as the operation performed (transaction signatures verification)
is logically distinct.

cli_test is and has always been an horrible name for package
directory as it's very much Go anti-idiomatic - _test is the
suffix used by test packages, not directories. Plus, CLI test
cases can and should live alongside other testcases that don't
require binaries to be built beforehand. Thus:

x/module/client/cli_test/*.go -> x/module/client/cli/

Test files that require sim{cli,d} shall be tagged with // +build cli_test

With regard to cli test auxiliary functions, they should live in:

x/module/client/testutil/
@alessio alessio force-pushed the alessio/turn-validate-signatures-into-standalone-cmd branch from 64409d0 to 9586d6f Compare May 2, 2020 20:23
@alessio
Copy link
Contributor Author

alessio commented May 4, 2020

@alexanderbez bump

alexanderbez
alexanderbez approved these changes May 4, 2020
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

@alexanderbez alexanderbez added the A:automerge Automatically merge PR once all prerequisites pass. label May 4, 2020
@mergify mergify bot merged commit 2414e5b into master May 4, 2020
@mergify mergify bot deleted the alessio/turn-validate-signatures-into-standalone-cmd branch May 4, 2020 13:55
@sahith-narahari sahith-narahari mentioned this pull request May 4, 2020
Closed
11 tasks
@fedekunze fedekunze mentioned this pull request May 6, 2020
Merged
11 tasks
larry0x pushed a commit to larry0x/cosmos-sdk that referenced this pull request May 22, 2023
@alexanderbez
x/auth: turn sign --validate-sigantures into a standalone command (co…
f75ce7a 
…smos#6108)

--validate-signatures should not be a flag of the sign command
as the operation performed (transaction signatures verification)
is logically distinct.

cli_test is and has always been an horrible name for package
directory as it's very much Go anti-idiomatic - _test is the
suffix used by test packages, not directories. Plus, CLI test
cases can and should live alongside other testcases that don't
require binaries to be built beforehand. Thus:

x/module/client/cli_test/*.go -> x/module/client/cli/

Test files that require sim{cli,d} shall be tagged with // +build cli_test

With regard to cli test auxiliary functions, they should live in:

x/module/client/testutil/

Co-authored-by: Alexander Bezobchuk <alexanderbez@users.noreply.github.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@jgimeno jgimeno jgimeno approved these changes

@alexanderbez alexanderbez alexanderbez approved these changes

@fedekunze fedekunze Awaiting requested review from fedekunze

@hschoenburg hschoenburg Awaiting requested review from hschoenburg

@nylira nylira Awaiting requested review from nylira

@tac0turtle tac0turtle Awaiting requested review from tac0turtle

@aaronc aaronc Awaiting requested review from aaronc

@clevinson clevinson Awaiting requested review from clevinson

Assignees

@alessio alessio

Labels
A:automerge Automatically merge PR once all prerequisites pass.
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
4 participants
@alessio @jgimeno @aaronc @alexanderbez