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: Make StdSignature turn into interface #3283

Closed
wants to merge 1 commit into from

Conversation

alessio
Copy link
Contributor

@alessio alessio commented Jan 12, 2019

Ref #3198

  • 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 force-pushed the alessio/stdsignature-as-interface branch 2 times, most recently from 39eea1a to 1de2615 Compare January 12, 2019 02:59
@alessio alessio requested a review from zramsay as a code owner January 12, 2019 03:16
@codecov
Copy link

codecov bot commented Jan 12, 2019

Codecov Report

Merging #3283 into develop will increase coverage by 0.06%.
The diff coverage is 84.21%.

@@             Coverage Diff             @@
##           develop    #3283      +/-   ##
===========================================
+ Coverage    55.18%   55.25%   +0.06%     
===========================================
  Files          134      134              
  Lines         9526     9531       +5     
===========================================
+ Hits          5257     5266       +9     
+ Misses        3938     3934       -4     
  Partials       331      331

@alessio alessio changed the title WIP: Make StdSignature turn into interface R4R: Make StdSignature turn into interface Jan 12, 2019
Add new StdSingleSignature concrete structure.

Ref #3198
@alessio alessio force-pushed the alessio/stdsignature-as-interface branch from a24fc5e to 64070a3 Compare January 12, 2019 03:28
@ValarDragon
Copy link
Contributor

RIP 4 more bytes in every tx

@alessio alessio requested a review from ValarDragon January 12, 2019 04:31
@sunnya97
Copy link
Member

Hmm, upon looking at this, I'm actually reconsidering. When I proposed this idea, I didn't realize that

  1. The method for verifying a signature is pubKey.VerifyBytes(signBytes []byte, sigatureBytes []byte) rather than a method on the StdSignature itself.

  2. And also I didn't realize that the multisig package already has a multisignature type.

I didn't realize that a lot of this functionality was already implemented by the multisig package. Given this, I'm not sure that the interface is necessary. Thoughts?

@alessio
Copy link
Contributor Author

alessio commented Jan 13, 2019

I'm fine with that @sunnya97

@alessio alessio closed this Jan 13, 2019
@alessio alessio deleted the alessio/stdsignature-as-interface branch January 13, 2019 12:05
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants