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

6111 add multisig support #6204

Closed

Conversation

aaronc
Copy link
Member

@aaronc aaronc commented May 12, 2020

@auto-comment
Copy link

auto-comment bot commented May 12, 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! 🚀

message Single {
SignMode mode = 1;
// locale may be used in the future with TEXTUAL signing modes
string locale = 2;
Copy link
Contributor

Choose a reason for hiding this comment

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

Is this really necessary now? I would rather address specifics for the TEXTUAL mode at a later date when we actually begin fleshing them out.

e.g. If textual mode ends up being something more like a cleaner key-value representation (yaml, etc.) then we wouldn't need locale at all, right?

Copy link
Contributor

Choose a reason for hiding this comment

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

Also, locale is not handled at all in the "multi" mode as this PR is written. Another reason why i think we should drop for now.

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 just added it so that we don't collapse Single into just the SignMode enum because we may want other fields describing the mode. I can remove, but Single should be a message not just the enum.

And yes it is handled in the multi mode. Study the Multi message a bit more

// bitarray specifies which keys within the multisig are signing
CompactBitArray bitarray = 1;
// mode_infos is the corresponding modes of the signers of the multisig
// which could include nested multisig public keys
Copy link
Contributor

Choose a reason for hiding this comment

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

This comment is confusing to me. It reads as if nested multisig public keys a possible signing mode, yet I don't see anything corresponding to "nested multisig public keys" in the mode enum.

Copy link
Member Author

Choose a reason for hiding this comment

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

It wouldn't exist in the mode enum. It's a recursive set of ModeInfo messages

Copy link
Member Author

Choose a reason for hiding this comment

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

For the latest design of the actual pub keys see #5997

Copy link
Contributor

Choose a reason for hiding this comment

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

But at the proto level, this would be a flattened list of ModeInfo messages right? If I understand correctly, the purpose for including the ModeInfo list at all is only for accurate gas cost estimations.

Or does repeated have some support for recursive lists?

@aaronc aaronc closed this May 13, 2020
@clevinson clevinson added this to the v0.39 milestone Jun 11, 2020
@alessio alessio deleted the aaronc/6030-update-adr-020-3 branch March 14, 2021 01:45
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.

2 participants