WIP on group module spec #13
Conversation
|
@ethanfrey just FYI that I have started working on this here and hope to do a bit more work tomorrow AM before our call. |
x/group/spec/state.md
Outdated
| ## Groups | ||
|
|
||
| Groups are simply aggregations of members. | ||
|
|
||
| ### ID | ||
|
|
||
| They have an auto-incremented ID: | ||
| ```go | ||
| type GroupID uint64 | ||
| ``` | ||
|
|
||
| ### Properties | ||
|
|
||
| #### `Members []Member` | ||
|
|
||
| An array of `Member` structs: | ||
|
|
||
| ```go | ||
| // Member specifies a address and a weight for a group member | ||
| type Member struct { | ||
| // The address of a group member. Can be another group or a contract | ||
| Address sdk.AccAddress `json:"address"` | ||
| // The integral weight of this member with respect to other members and the decision threshold | ||
| Weight sdk.Int `json:"weight"` | ||
| } | ||
| ``` | ||
|
|
||
| #### `Owner sdk.ACcAddress` | ||
|
|
||
| Owner is the account which "owns" the group and has the permission to | ||
| add and remove memers. | ||
|
|
||
| #### `Memo string` | ||
|
|
||
| A single string memo | ||
|
|
||
| #### Indexes | ||
|
|
||
| #### `Member` | ||
|
|
||
| It should be possible to look-up groups by member | ||
|
|
||
| #### `Owner` | ||
|
|
There was a problem hiding this comment.
@ethanfrey can you let me know if this general structure of specifying state makes sense? I'm imagining this is similar to how you would structure an ORM - mapping properties, indexes, etc. rather than structs so to me its most logical to express it this way instead of go structs. I'll try to add more details on each property etc. tomorrow.
There was a problem hiding this comment.
Good start on this doc and it gives a lot more clarity.
With the clarity, come a lot more questions and comments on the details.
These are all meant to refine and enhance the spec. I also think a few more use cases spec'd out will help ground a number of the questions on details (like creator voting, who has permission to do what, quorum, etc).
I left a few comments in here for alex, as he will be reading this and is used to some different terminology.
| ```go | ||
| // Groups get their own GroupID | ||
| type MsgCreateGroup struct { | ||
| Signer sdk.AccAddress `json:"signer"` |
There was a problem hiding this comment.
Why is there a Signer and an Owner?
Either they should be the same, or we don't have to require any Signer (if anyone can create a group)
There was a problem hiding this comment.
A Signer is always required in a cosmos SDK message. One reason for having both is that Owner may actually be a contract - for instance some contract that elects the members of the group.
There was a problem hiding this comment.
I see the purpose of Owner in state.
Signers() can return msg.Owner
If we don't care who signs, no need to return anything.
Again, this is a bit more discussion of best practices of cosmos-sdk... not sure.
| // The members of the group and their associated weight | ||
| Members []Member `json:"members,omitempty"` | ||
| // TODO maybe make this something more specific to a domain name or a claim on identity? or Info leave it generic | ||
| Memo string `json:"memo,omitempty"` |
There was a problem hiding this comment.
If we store this on the group itself, I would call it Description or Title.
I think that makes sense, so they can identify themselves.
I do think storing this makes sense.
If it is just a memo for the transaction itself, we don't need it imo. There is a top-level one.
There was a problem hiding this comment.
Agree, memo is used already in a different context. For UX purpose in clients it may be very useful to store human readable information with a group. Title and description both make sense.
| Signer sdk.AccAddress `json:"signer"` | ||
| // The Owner of the group is allowed to change the group structure. A group account | ||
| // can own a group in order for the group to be able to manage its own members | ||
| Owner sdk.AccAddress `json:"owner"` |
There was a problem hiding this comment.
Chicken and egg issue...
I need to create a group (alex: read "electorate") in order to create a group account (alex: read "election rule").
So, how can I set it as owner on creation of the group.
I would propose a 3 step setup:
- X creates a group and is automatically the owner (owner == signer)
- X creates a group account with a decision policy (eg. 2 of 3) on this group
- X abdicates the thrown and makes the given group account the owner of the group
Those can be 3 messages in one tx and we can do that behind the scenes in the user interface, but in order to avoid some weird causality tangle, I suggest to make them explicitly separate
There was a problem hiding this comment.
This brings up the point that we need a MsgUpdateGroup as well
| Owner sdk.AccAddress `json:"owner"` | ||
| Group GroupID `json:"group"` | ||
| DecisionPolicy DecisionPolicy `json:"decision_policy"` | ||
| Memo string `json:"memo,omitempty"` |
There was a problem hiding this comment.
Again, Memo is something in the Tx that never gets written to state. If we store it, please call it Name, Title, etc.
| Signer sdk.AccAddress `json:"signer"` | ||
| // The Owner of a group account is allowed to change the DecisionPolicy. This can be left nil | ||
| // in order for the group account to "own" itself | ||
| Owner sdk.AccAddress `json:"owner"` |
There was a problem hiding this comment.
Why is nil = group owns itself? Implicit nils are dangerous to me.
I would allow the options of no owner (can never change), self-owner, or external owner (maybe a 1 of N rule to move funds, controlled by a 3 of N rules to update the rules - or change the group).
At least in the state, nil should be no owner, rather than self-owner. Problem in the Msg is you don't know the self-address ahead of time. I would make that explicit somehow here (difference between self-owner and un-owned)
There was a problem hiding this comment.
And if there is an owner... we need a MsgUpdateGroupAccount to change the decision policy
|
|
||
| | Key | Type | Usage | Example | | ||
| |------------------------|-----------------|---------| | ||
| | MaxVotingPeriod | string (time ns) | Proposals older than this time will be garbage-collected | "172800000000000" | |
There was a problem hiding this comment.
okay, in general decision policies can't set a voting timeout, smaller than this garbage collection limit.
also, no need for ns precision with ~5s blocktime. Removing 9 zeros, makes this much more legible (it is 2 days, right)
There was a problem hiding this comment.
Express it in minutes then? I think that precision is for easy conversion to time.Time
x/group/spec/state.md
Outdated
| // powers (the tally of yes, no, abstain, and veto votes) and time (via | ||
| // the block header proposalSubmitTime) | ||
| type DecisionPolicy interface { | ||
| Allow(tally Tally, totalPower sdk.Int, header types.Header, submittedTime time.Time, submittedHeight int64) |
There was a problem hiding this comment.
Okay, so this interface allows us to set a timeout, or enforce a quorum I believe.
Can you make some demo structs for "time limit" or "percentage with quorum" votes?
Quorum is actually trickier as you need to wait for a fixed time before doing the tally and can only vote until then, otherwise the results can still change as new votes come in.
Well, at least time limit should be an example struct, let's think about quorom. It requires a way to enforce a timelimit for recording MsgVote info, not just performing validation inside the MsgTryExecuteProposal
|
|
||
| type MsgTryExecuteProposal struct { | ||
| ProposalID ProposalID `json:"proposal_id"` | ||
| Signer sdk.AccAddress `json:"signer"` |
There was a problem hiding this comment.
Does the signer need to be a member of the group?
If anyone can sign this, we can just remove this field. Only if we need this info to enforce some permissions.
There was a problem hiding this comment.
Anyone could sign, but again in cosmos-sdk the signer needs to be explicit on the sdk.Msg so that Signers() can be implemented
There was a problem hiding this comment.
If anyone can perform this action, then Signers() can return nil. That is valid afaik
x/group/spec/state.md
Outdated
| type Choice int | ||
|
|
||
| const ( | ||
| No Choice = iota |
There was a problem hiding this comment.
For external apis, please define all ints explicitly.
iota is nice when it is never serialized and only used by the same codebase. (not some js client code)
|
@ethanfrey I made some updates to state.md. I think that's mostly what it makes sense to focus on today rather than the messages which should be derived from desired state and behavior. Also state is what relates most to a potential ORM. |
|
A slightly un-related thought is should we allow deposits as a spam prevention mechanism? I've thought about it and it's fairly easy to add in. My hesitation is really - is it actually a good spam prevention mechanism? If it isn't needed min-deposit could always be set to zero. But since it's relatively easy to add in, should we do it so that this is a more complete solution for DAO's? Also just FYI on use cases, Regen does have this concept of "community staking DAO's" in our mainnet launch roadmap so the use case isn't totally hypothetical. |
| // The members of the group and their associated weight | ||
| Members []Member `json:"members,omitempty"` | ||
| // TODO maybe make this something more specific to a domain name or a claim on identity? or Info leave it generic | ||
| Memo string `json:"memo,omitempty"` |
There was a problem hiding this comment.
Agree, memo is used already in a different context. For UX purpose in clients it may be very useful to store human readable information with a group. Title and description both make sense.
| Proposer sdk.AccAddress `json:"proposer"` | ||
| GroupAccount sdk.AccAddress `json:"group"` | ||
| Msgs []sdk.Msg `json:"msgs"` | ||
| // Exec can be set to true in order to attempt to execute the proposal immediately |
There was a problem hiding this comment.
I do understand the motivation but there will be quite some complexity for any UI client that have to show the state or votes. I don't know much about the veto rules, yet but they may add complexity, too.
|
Closed in favor of #15 |
PR for discussion before submitting upstream.