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

multisig: add type and bitarray #6241

Merged
merged 10 commits into from
May 20, 2020
Merged

multisig: add type and bitarray #6241

merged 10 commits into from
May 20, 2020

Conversation

tac0turtle
Copy link
Member

Description

This pr aims to add the multisign from tendermint to the sdk

closes: #XXXX

@auto-comment
Copy link

auto-comment bot commented May 18, 2020

👋 Thanks for creating a PR!

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.


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)

Thank you for your contribution to the Cosmos-SDK! 🚀

@tac0turtle tac0turtle self-assigned this May 18, 2020
@codecov
Copy link

codecov bot commented May 18, 2020

Codecov Report

Merging #6241 into master will increase coverage by 0.13%.
The diff coverage is 73.23%.

@@            Coverage Diff             @@
##           master    #6241      +/-   ##
==========================================
+ Coverage   54.75%   54.89%   +0.13%     
==========================================
  Files         442      446       +4     
  Lines       26960    27158     +198     
==========================================
+ Hits        14763    14908     +145     
- Misses      11162    11193      +31     
- Partials     1035     1057      +22     

@tac0turtle tac0turtle removed the WIP label May 19, 2020
@tac0turtle tac0turtle marked this pull request as ready for review May 19, 2020 10:05
CHANGELOG.md Outdated Show resolved Hide resolved
"strings"
)

// CompactBitArray is an implementation of a space efficient bit array.
Copy link
Contributor

Choose a reason for hiding this comment

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

Remove commented out code.

Copy link
Contributor

Choose a reason for hiding this comment

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

bump

}


// MultiSignature wraps the signatures from a PubKeyMultisigThreshold.
// See cosmos_sdk.tx.v1.ModeInfo.Multi for how to specify which signers signed
// and with which modes
message MultiSignature {
Copy link
Contributor

Choose a reason for hiding this comment

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

So we now have two MultiSignature proto types. This can certainly lead to confusion. What are the differences between the two and when use one over the other? The godocs should reflect this.

Copy link
Member Author

Choose a reason for hiding this comment

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

I brought back the hand-written type as I am not sure how this should be integrated to provide backwards compatability.

Copy link
Contributor

Choose a reason for hiding this comment

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

@aaronc? Sorry, I'm a bit out of touch with the proto work with how it relates to keys...

Copy link
Member

Choose a reason for hiding this comment

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

@alexanderbez are you referring to the ModeInfo.Multi and Multisignature? One is for the mode infos and the other is the actual signatures.... Is that what you mean?

Copy link
Contributor

Choose a reason for hiding this comment

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

No, just Multisignature.

Copy link
Member

Choose a reason for hiding this comment

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

I'm confused. There are two types?

Copy link
Contributor

Choose a reason for hiding this comment

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

Yes -- review the PR here. There are types.Multisignature and multisig.Multisignature

Copy link
Member

Choose a reason for hiding this comment

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

I think it's fine for now. I'm going to need to rework some of this stuff to support multiple signing modes in #6216. In the meantime, maybe we can rename multisig.Multisignature to something different?

crypto/multisig/types.proto Outdated Show resolved Hide resolved
types/compact_bit_array.go Outdated Show resolved Hide resolved
types/compact_bit_array.go Outdated Show resolved Hide resolved
var cdc = amino.NewCodec()

func init() {
cdc.RegisterInterface((*crypto.PubKey)(nil), nil)
Copy link
Contributor

Choose a reason for hiding this comment

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

What is the need to register here for?

Copy link
Member Author

Choose a reason for hiding this comment

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

these should be kept around to provide backwards compatibility. I haven't dug into how this will be done with the recent changes to the ADR.

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.

utACK

types/types.proto Outdated Show resolved Hide resolved
Copy link
Member

@aaronc aaronc left a comment

Choose a reason for hiding this comment

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

This looks like a good start to me. It's going to need to be modified for sure to support the new sign modes in #6216 and also to address legacy addresses. But I can work from this and would prefer we move forward with merging so I have a starting point.

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

@alexanderbez alexanderbez added the A:automerge Automatically merge PR once all prerequisites pass. label May 20, 2020
@mergify mergify bot merged commit a201967 into master May 20, 2020
@mergify mergify bot deleted the marko/add_multisig branch May 20, 2020 20:10
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:Crypto
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants