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

Generalise auth/types.StdSignature #4507

Merged
merged 3 commits into from
Jun 7, 2019
Merged

Conversation

alessio
Copy link
Contributor

@alessio alessio commented Jun 7, 2019

New Signature interface available in the top level types package.
auth.StdSignature implements such interface. User defined auth module can now
define their own custom signature types.

Work carried out in the context of the following issues:


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 marked this pull request as ready for review June 7, 2019 09:41
@alessio alessio requested review from sabau and fedekunze June 7, 2019 09:41
@codecov
Copy link

codecov bot commented Jun 7, 2019

Codecov Report

Merging #4507 into master will increase coverage by 0.01%.
The diff coverage is 68%.

@@            Coverage Diff             @@
##           master    #4507      +/-   ##
==========================================
+ Coverage   53.17%   53.18%   +0.01%     
==========================================
  Files         259      259              
  Lines       16191    16193       +2     
==========================================
+ Hits         8610     8613       +3     
+ Misses       6935     6934       -1     
  Partials      646      646

Copy link
Contributor

@sabau sabau left a comment

Choose a reason for hiding this comment

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

LGTM

Copy link
Collaborator

@fedekunze fedekunze left a comment

Choose a reason for hiding this comment

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

minor changes requested

.pending/improvements/sdk/4507-New-Signature-g Outdated Show resolved Hide resolved
.pending/improvements/sdk/4507-New-Signature-g Outdated Show resolved Hide resolved
x/auth/types/stdtx_test.go Outdated Show resolved Hide resolved
x/auth/types/stdtx.go Show resolved Hide resolved
@alessio alessio requested a review from fedekunze June 7, 2019 10:54
client/utils/utils_test.go Outdated Show resolved Hide resolved
simSig := StdSignature{PubKey: pubkey}
if len(sig.Signature) == 0 {
if len(sig.GetSignature()) == 0 {
Copy link
Collaborator

Choose a reason for hiding this comment

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

what about creating a new IsEmpty() method to the interface ?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

This is the only occurrence - plus, IsEmpty() might turn out to be ambiguous. What is the emptiness check being carried upon? Should IsEmpty() return true when signature is not present or only when both the sig and the key are nil?

@alessio alessio requested a review from fedekunze June 7, 2019 12:30
New Signature interface available in the top level types package.
auth.StdSignature implements such interface. User defined auth
module can now define their own custom signature types.

Work carried out in the context of the following issues:
- #4488
- #4487
@alessio alessio force-pushed the alessio/generalise-stdsignature branch from 525b6d3 to d11aca7 Compare June 7, 2019 12:45
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.

ACK

x/auth/types/stdtx.go Show resolved Hide resolved
@alexanderbez alexanderbez added ready-for-review T: API Breaking Breaking changes that impact APIs and the SDK only (not state machine). labels Jun 7, 2019
@alessio alessio dismissed fedekunze’s stale review June 7, 2019 13:20

All comments were addressed

@alessio alessio merged commit a32d5a4 into master Jun 7, 2019
@alessio alessio deleted the alessio/generalise-stdsignature branch June 7, 2019 13:21
alexanderbez added a commit that referenced this pull request Jun 7, 2019
alessio pushed a commit that referenced this pull request Jun 8, 2019
larry0x pushed a commit to larry0x/cosmos-sdk that referenced this pull request May 22, 2023
New Signature interface available in the top level types package.
auth.StdSignature implements such interface. User defined auth
module can now define their own custom signature types.

Work carried out in the context of the following issues:
- cosmos#4488
- cosmos#4487
larry0x pushed a commit to larry0x/cosmos-sdk that referenced this pull request May 22, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
T: API Breaking Breaking changes that impact APIs and the SDK only (not state machine).
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants