R4R: Multisig support #3264

alessio · 2019-01-09T19:03:40Z

Habemus multisig:

New keys add --multisig flag to store multisig keys locally.
New multisign command to generate multisig signatures.
New sign --multisig flag to enable multisig mode.
Add multisig transactions support in ante handler
gaiad add-genesis-account can now take both account addresses and key names

Closes: #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)

codecov · 2019-01-09T19:11:38Z

Codecov Report

Merging #3264 into develop will decrease coverage by 0.12%.
The diff coverage is 24.13%.

@@             Coverage Diff             @@
##           develop    #3264      +/-   ##
===========================================
- Coverage    55.64%   55.52%   -0.13%     
===========================================
  Files          134      134              
  Lines         9746     9764      +18     
===========================================
- Hits          5423     5421       -2     
- Misses        3980     3999      +19     
- Partials       343      344       +1

alessio · 2019-01-10T13:09:53Z

Blocked on tendermint/tendermint#3101

zmanian · 2019-01-10T16:56:47Z

I would recommend we use magic-wormhole https://github.com/warner/magic-wormhole in documentation to move partially signed transactions around.

alessio · 2019-01-11T16:05:10Z

Blocked on #3279

x/auth/ante.go

cwgoes

Workflow looks reasonable to me, several comments.

client/keys/add.go

cwgoes · 2019-01-15T15:15:41Z

cmd/gaia/init/genesis_accts.go

@@ -28,7 +29,15 @@ func AddGenesisAccountCmd(ctx *server.Context, cdc *codec.Codec) *cobra.Command

 			addr, err := sdk.AccAddressFromBech32(args[0])
 			if err != nil {
-				return err
+				kb, err := keys.GetKeyBaseFromDir(viper.GetString(flagClientHome))


OK, this is convenient - I wonder if we should just support named aliases, but that can be a later question...

Yes, I'm doing it just for convenience. But I wouldn't frown upon a change request and I reckon this could we be seen as superfluous.

docs/gaia/gaiacli.md

cwgoes · 2019-01-15T15:17:19Z

docs/gaia/gaiacli.md

+gaiacli keys add --multisig=baz,foo,bar --multisig-threshold=2 multisigaddress
+```
+
+Although keys and threshold parameter used in the above examples are the same, the commands


Can we sort the names? This behaviour seems unusual, unless there's a specific reason we want it.

... actually, can we sort by the public keys? that way different names on different machines will still work

This sounds like a good idea. Ideally order doesn't matter here.

yeah I second this. I can see simple mishaps happening due to this.

Sorted - sorry for the pun.

I've also added the flag --nosort so that a user can pass the keys in the exact order he wants and expect they don't get shuffled.

docs/gaia/gaiacli.md

x/auth/ante.go

x/auth/client/cli/multisign.go

client/keys/add.go

docs/gaia/gaiacli.md

jackzampolin

A couple of small comments 👆 but otherwise this is 💯 🎉 and should be merged.

alexanderbez

Looks good @alessio -- I left some feedback.

client/keys/add.go

client/utils/utils.go

alexanderbez · 2019-01-15T17:20:23Z

docs/gaia/gaiacli.md

+gaiacli keys add --multisig=baz,foo,bar --multisig-threshold=2 multisigaddress
+```
+
+Although keys and threshold parameter used in the above examples are the same, the commands


yeah I second this. I can see simple mishaps happening due to this.

docs/gaia/gaiacli.md

x/auth/client/cli/multisign.go

jackzampolin · 2019-01-15T18:33:13Z

A couple of test failures here @alessio

docs/gaia/gaiacli.md

alexanderbez · 2019-01-15T19:03:05Z

Few more comments @alessio, also integration_tests is failing.

fedekunze

👍 👍 minor comments. Also we'll need to support multisig through gaia lite (maybe other PR)

client/keys/add.go

cmd/gaia/cli_test/cli_test.go

docs/gaia/gaiacli.md

x/auth/client/cli/sign.go

cwgoes · 2019-01-16T12:44:43Z

client/keys/add.go

+
+			// Handle --nosort
+			if !viper.GetBool(flagNoSort) {
+				sort.Strings(multisigKeys)


I wonder if we should instead sort by the keys - that way different aliases on different computers will still result in the same public key.

I second this.

alexanderbez

Looks good @alessio -- thoughts on sorting by pubkey as @cwgoes suggested?

PENDING.md

alexanderbez · 2019-01-16T14:40:12Z

client/keys/add.go

+
+			// Handle --nosort
+			if !viper.GetBool(flagNoSort) {
+				sort.Strings(multisigKeys)


I second this.

- New keys add --multisig flag to store multisig keys locally. - New multisign command to generate multisig signatures. - New sign --multisig flag to enable multisig mode. - Add multisig transactions support in ante handler. - gaiad add-genesis-account can now take both account addresses and key names. Closes: #3198

Comments addressed

alessio requested review from cwgoes, ebuchman and rigelrozanski as code owners January 9, 2019 19:03

alessio force-pushed the alessio/multisig-workflow branch from 15ec441 to 7d50367 Compare January 10, 2019 01:11

alessio added the S:blocked Status: Blocked label Jan 10, 2019

alessio added S:blocked Status: Blocked and removed S:blocked Status: Blocked labels Jan 11, 2019

alessio mentioned this pull request Jan 11, 2019

R4R: Upgrade tendermint to 0.28.0-dev0 #3279

Merged

5 tasks

alessio removed the S:blocked Status: Blocked label Jan 12, 2019

alessio changed the title ~~WIP: Alessio/multisig workflow~~ WIP: Multisig workflow Jan 12, 2019

alessio force-pushed the alessio/multisig-workflow branch 2 times, most recently from ff43ede to b498016 Compare January 15, 2019 00:10

alessio requested review from jackzampolin, alexanderbez, fedekunze, jaekwon, sunnya97 and ValarDragon January 15, 2019 01:49

alessio requested a review from zramsay as a code owner January 15, 2019 02:38

alessio force-pushed the alessio/multisig-workflow branch from 1ba51e3 to 99f11c0 Compare January 15, 2019 05:14

alessio changed the title ~~WIP: Multisig workflow~~ R4R: Multisig workflow Jan 15, 2019

alessio added the ready-for-review label Jan 15, 2019

alessio requested a review from zmanian January 15, 2019 05:17

alessio changed the title ~~R4R: Multisig workflow~~ R4R: Multisig support Jan 15, 2019

zmanian reviewed Jan 15, 2019

View reviewed changes

x/auth/ante.go Outdated Show resolved Hide resolved

cwgoes suggested changes Jan 15, 2019

View reviewed changes

jackzampolin reviewed Jan 15, 2019

View reviewed changes

docs/gaia/gaiacli.md Outdated Show resolved Hide resolved

jackzampolin reviewed Jan 15, 2019

View reviewed changes

docs/gaia/gaiacli.md Show resolved Hide resolved

jackzampolin approved these changes Jan 15, 2019

View reviewed changes

alexanderbez suggested changes Jan 15, 2019

View reviewed changes

alexanderbez reviewed Jan 15, 2019

View reviewed changes

docs/gaia/gaiacli.md Outdated Show resolved Hide resolved

fedekunze reviewed Jan 15, 2019

View reviewed changes

alessio force-pushed the alessio/multisig-workflow branch 5 times, most recently from e6c44da to 1b94623 Compare January 16, 2019 00:59

cwgoes reviewed Jan 16, 2019

View reviewed changes

alexanderbez previously requested changes Jan 16, 2019

View reviewed changes

Alessio Treglia added 2 commits January 16, 2019 15:38

Update PENDING.md

0022513

alessio force-pushed the alessio/multisig-workflow branch from 1b94623 to 0022513 Compare January 16, 2019 15:38

cwgoes approved these changes Jan 16, 2019

View reviewed changes

zmanian approved these changes Jan 16, 2019

View reviewed changes

jackzampolin merged commit 26cb0a1 into develop Jan 16, 2019

jackzampolin deleted the alessio/multisig-workflow branch January 16, 2019 17:31

alessio mentioned this pull request Jan 18, 2019

Support named aliases for multisignature pubkeys #2587

Closed

cwgoes pushed a commit that referenced this pull request Feb 12, 2019

Merge PR #3264: Return an empty TxResponse when nil

22bdc17

alessio mentioned this pull request Mar 16, 2020

Improved multisig handling #5661

Closed

4 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

R4R: Multisig support #3264

R4R: Multisig support #3264

alessio commented Jan 9, 2019 •

edited

Loading

codecov bot commented Jan 9, 2019 •

edited

Loading

alessio commented Jan 10, 2019 •

edited

Loading

zmanian commented Jan 10, 2019

alessio commented Jan 11, 2019

cwgoes left a comment

cwgoes Jan 15, 2019

alessio Jan 15, 2019

cwgoes Jan 15, 2019

cwgoes Jan 15, 2019

jackzampolin Jan 15, 2019

alexanderbez Jan 15, 2019

alessio Jan 15, 2019

alessio Jan 15, 2019

jackzampolin left a comment

alexanderbez left a comment

alexanderbez Jan 15, 2019

jackzampolin commented Jan 15, 2019

alexanderbez commented Jan 15, 2019

fedekunze left a comment

cwgoes Jan 16, 2019

alexanderbez Jan 16, 2019

alexanderbez left a comment

alexanderbez Jan 16, 2019

R4R: Multisig support #3264

R4R: Multisig support #3264

Conversation

alessio commented Jan 9, 2019 • edited Loading

codecov bot commented Jan 9, 2019 • edited Loading

Codecov Report

alessio commented Jan 10, 2019 • edited Loading

zmanian commented Jan 10, 2019

alessio commented Jan 11, 2019

cwgoes left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jackzampolin left a comment

Choose a reason for hiding this comment

alexanderbez left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jackzampolin commented Jan 15, 2019

alexanderbez commented Jan 15, 2019

fedekunze left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

alexanderbez left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

alessio commented Jan 9, 2019 •

edited

Loading

codecov bot commented Jan 9, 2019 •

edited

Loading

alessio commented Jan 10, 2019 •

edited

Loading