Group module spec #5236

aaronc · 2019-10-23T19:56:12Z

@alexanderbez @jackzampolin this is a placeholder for discussion of the proposed group module spec. This is almost identical to the proposal discussed in the gist and in the ICF grant (I had changed it but changed it back after discussing with @ethanfrey)

I have split this draft spec into three topical sections:

groups - the basis mechanics of aggregating groups of accounts
group accounts - the actual association of groups to accounts with a decision policy and a balance. Groups can have multiple accounts with different decision policies and the msg delegation feature can be used to assign different permissions to different accounts
proposals - the ability to open proposals and vote on them asynchronously. Note that this part is not required to be enabled for other group stuff to work! Groups can execute multisig transactions aggregated offline without proposals!

codecov · 2019-10-23T20:09:45Z

Codecov Report

Merging #5236 into master will not change coverage.
The diff coverage is n/a.

@@           Coverage Diff           @@
##           master    #5236   +/-   ##
=======================================
  Coverage   53.51%   53.51%           
=======================================
  Files         290      290           
  Lines       17770    17770           
=======================================
  Hits         9509     9509           
  Misses       7511     7511           
  Partials      750      750

fedekunze · 2019-10-24T12:03:51Z

Thanks @aaronc. tbh I don't think this belongs to the core modules. It should live in the https://github.com/cosmos/modules repo

aaronc · 2019-10-24T20:27:31Z

Okay, good to know. I wonder if @alexanderbez and @jackzampolin have a similar opinion.

If that's the case, my next question is would a pared-down version of the group module be wanted in SDK core?

The simpliest version of this group module would be something that aggregates multiple keys as a multi-sig account with the main differences from the existing multi-sig accounts being:

the sdk.Address is not based on any keys address but generated (likely from an auto-increment integer)
signing keys and thresholds can be changed

My understanding is that there is desire for such much a module from the community - in particular, validators who want to have a master account not tied to a single unchangeable private key. As I recall @dlguddus was even proposing that there be support for migrating existing validator keys to this key group.

For projects like Regen, we want more full group support with proposals as in this spec and as originally proposed after the hackathon in the ICF grant, but maybe that's too much for the hub...

alexanderbez · 2019-10-30T15:33:26Z

I believe work should continue on this spec and the module should start/resume implementation. However, I believe the module should (for now) exist in cosmos/modules where the regen team are owners. Feedback and collaboration should continue there when and where the SDK team's input is needed 👍

/cc @ebuchman

ethanfrey

This is getting close, but I added a number of detail questions to refine this.

Also, I cannot comment but on spec/proposal.md line 58, you refer to Vote bool, but there is already a defined type Vote int. Why not use Vote Vote there?

x/group/spec/group.md

ethanfrey · 2019-11-06T09:48:06Z

x/group/spec/group.md

-    DecisionProtocol DecisionProtocol `json:"decision_protocol"`
+	Admin  sdk.AccAddress `json:"admin"`
+	Group  GroupID `json:"group"`
+	NewAdmin  sdk.AccAddress `json:"new_admin"`


Please define behavior if NewAdmin is unset (I assume don't change Admin).

We can either make it a no-op like you're proposing or this would seal the group and make it impossible to change? Is that desirable? I think that would probably be unexpected behavior so for now I will define it as a no-op. If there is a use case for that, this could be a separate "seal group" message.

ethanfrey · 2019-11-06T09:50:10Z

x/group/spec/group.md

+	Admin  sdk.AccAddress `json:"admin"`
+	Group  GroupID `json:"group"`
+	NewAdmin  sdk.AccAddress `json:"new_admin"`
+	Description string `json:"description,omitempty"`


Same here.. I assume this is optional to change it? Maybe use *string to be clear there - otherwise there is no way to unset it.

It seems like this operation is supposed to be a patch, and to do so properly, we need to allow all values (including zero-value if legal) as well as "unset". In my experience with go-json, this is typically done by using pointer fields and checking for nil (which can be missing from json or explicitly set as null, either way, ignored)

An alternative to a single update message is to have a separate update message for each field, so MsgUpdateGroupDescription, MsgUpdateGroupAdmin, etc. Would that be preferable? For now I am changing the spec to your approach.

I think separate messages is bad.

Either define POST behavior - overwrite current content with msg.
Or PUT - only include some fields, only update those that are include.

I assume PUT is desired, but this should be made explicit and consistent.

x/group/spec/group.md

ethanfrey · 2019-11-06T09:54:07Z

x/group/spec/group.md

@@ -131,9 +67,8 @@ to authorize messages send back to the router.
 ```go
 type GroupKeeper interface {
  IterateGroupsByMember(member sdk.Address, fn func (group sdk.AccAddress) (stop bool))
-  IterateGroupsByOwner(member sdk.Address, fn func (group sdk.AccAddress) (stop bool))
+  IterateGroupsByAdmin(member sdk.Address, fn func (group sdk.AccAddress) (stop bool))


Why return the group sdk.AccAddress? Which means you have to then query the group itself? Seems easier to pass group Group into the callback.

Also, I note you never defined the Group struct that is stored in the kvstore above.

This part was intentional. I didn't define a group struct because I don't want to return all members at once. See how it is in the changed spec and we can discuss if maybe there should be a GroupMetadata struct like this:

type GroupMetadata struct { Group GroupID TotalPower sdk.Int Description string Admin sdk.AccAddress }

Ahh... Okay, then can you mark this in the spec as "not yet defined", but make sure some comment is there, not just an oversight.

ethanfrey · 2019-11-06T09:55:49Z

x/group/spec/group_account.md

+
+## Messages
+
+### `MsgCreateGroupAccount


This name is a bit unwieldy. I don't have a great alternative, but should be refined. Group makes clear sense and is what it says. GroupAccount is not clear.

Yes, GroupAccount is a bit unwieldy but I also was unable to come up with a better name. I am open to proposals.

I will give one when I have one.... wish I was good at naming

ethanfrey · 2019-11-06T09:56:19Z

x/group/spec/group_account.md

+}
+```
+
+*Returns:*  a new auto-generated `sdk.AccAddress` for this group.


s/group./group account./

ethanfrey · 2019-11-06T09:58:10Z

x/group/spec/group_account.md

+type MsgUpdateGroupAccount struct {
+	Admin  sdk.AccAddress `json:"admin"`
+	GroupAccount  sdk.AccAddress `json:"group_account"`
+	NewAdmin  sdk.AccAddress `json:"new_admin"`


Same question as above about omitted fields and zero-values - do we set to zero value, do we leave them unchanged? Both have issues, maybe consider pointers, even if a bit funny looking here, they render to the same json.

I want to suggest two functionality to MsgUpdateGroupAccount.

There can be a GroupAccount without Admin

Members of the GroupAccount can vote for MsgUpdateGroupAccount

This way, we can create a GroupAccount which ownership is fully decentralized but modifiable.

Another question.
Why MsgUpdateGroupAccount does not have a field Members to be updated?

questions resolved. thank you @aaronc

aaronc · 2019-11-06T20:08:06Z

@ethanfrey I update the spec based on your comments and responded with questions about things that I don't have a clear solution for. Let me know what you think.

alexanderbez · 2019-11-07T19:19:25Z

friendly PSA: please close this PR and move to cosmos/modules.

fedekunze · 2019-12-02T18:57:53Z

bump @aaronc

aaronc added 3 commits October 21, 2019 15:57

WIP on new group module spec

9df4207

Added proposal spec

baa8337

Markdown fix

02748b2

Updates to spec

f7461b2

Revert from group policy -> group account design after design discussion

0e1d676

Merge branch 'master' into regen-network/group-spec

72de7af

Hyung-bharvest mentioned this pull request Nov 4, 2019

validator control key migration to a group key during hub-3 -> hub-4 chain upgrade cosmos/gaia#178

Closed

4 tasks

ethanfrey reviewed Nov 6, 2019

View reviewed changes

Update group spec based on PR review

8fb79a8

fedekunze added the will-close label Dec 2, 2019

aaronc closed this Dec 2, 2019

aaronc mentioned this pull request Jan 15, 2020

Any progress in Subkeys Spec #5516

Closed

aaronc mentioned this pull request Oct 22, 2020

Add Group Module #7633

Closed

4 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Group module spec #5236

Group module spec #5236

aaronc commented Oct 23, 2019 •

edited

Loading

codecov bot commented Oct 23, 2019 •

edited

Loading

fedekunze commented Oct 24, 2019

aaronc commented Oct 24, 2019

alexanderbez commented Oct 30, 2019 •

edited

Loading

ethanfrey left a comment

ethanfrey Nov 6, 2019

aaronc Nov 6, 2019

ethanfrey Nov 7, 2019

ethanfrey Nov 6, 2019

aaronc Nov 6, 2019

ethanfrey Nov 7, 2019

ethanfrey Nov 6, 2019

aaronc Nov 6, 2019

ethanfrey Nov 7, 2019

ethanfrey Nov 6, 2019

aaronc Nov 6, 2019

ethanfrey Nov 7, 2019

ethanfrey Nov 6, 2019

ethanfrey Nov 6, 2019

Hyung-bharvest Nov 26, 2019 •

edited

Loading

Hyung-bharvest Nov 26, 2019

aaronc commented Nov 6, 2019

alexanderbez commented Nov 7, 2019

fedekunze commented Dec 2, 2019

Group module spec #5236

Group module spec #5236

Conversation

aaronc commented Oct 23, 2019 • edited Loading

codecov bot commented Oct 23, 2019 • edited Loading

Codecov Report

fedekunze commented Oct 24, 2019

aaronc commented Oct 24, 2019

alexanderbez commented Oct 30, 2019 • edited Loading

ethanfrey 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

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

Hyung-bharvest Nov 26, 2019 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

aaronc commented Nov 6, 2019

alexanderbez commented Nov 7, 2019

fedekunze commented Dec 2, 2019

aaronc commented Oct 23, 2019 •

edited

Loading

codecov bot commented Oct 23, 2019 •

edited

Loading

alexanderbez commented Oct 30, 2019 •

edited

Loading

Hyung-bharvest Nov 26, 2019 •

edited

Loading