Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
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
ADR-042: Group module #9089
ADR-042: Group module #9089
Changes from 7 commits
02633ab
8582590
2ccc701
f9a16b3
217ca16
64f87bc
992a86b
432b93f
abe6245
08dcd47
16578be
180eb50
ce4510b
8031db1
90dd35d
9969a6f
bc20dcc
d2be7ff
f13b343
146ea46
ee0567f
9897020
d6a0654
b74b534
783d51d
9afc938
ce103bf
1afa664
5d609c6
adf1525
e9c39b1
c3bf028
08d9be5
File filter
Filter by extension
Conversations
Jump to
There are no files selected for viewing
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This code implements adr033, correct?
Another note, the coding style could cause some confusion since we have a recommended module layout, the groups module implements something new that seems to lack documentation as well.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
yes indeed, we're currently having a broader conversion about ADR 033 module wiring here: regen-network/regen-ledger#301 so this needs to be figured out before implementing the group module as part of the sdk
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Will this be able to be a
GroupAccountInfo.Address
i.e. does the groups module support nested groupsThere was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes it's theoretically possible, same for the group admin account.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Or module addresses?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
A group account is a module account itself so this works in this case but how could a regular module account vote?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Might want to create an automated voting module or offload to cosmwasm.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
What do you imagine a use case for this sort of meta data to be?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Here's a use case in the context of regen-ledger: this could represent a hash of some data from the
x/data
module which is used for timestamping, signing, and storing data.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
BTW, re audit notes - we may remove
Group
prefix from all type names. It's already provided by a package.cc: @aaronc
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This pattern is very nice to highlight
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why do we have
Info
suffix? Based on the paragraph above, I would call thisPolicyAccount
.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It was in order to be consistent with x/ecocredit from regen-ledger which uses this suffix too, but I'd be ok with dropping it.
We use
Group
prefix because it's related to a given group and I believe it feels more natural as part of thex/group
module.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
So, how about
PolicyGroup
?There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
But then you lose info about the fact that it's an actual account. I kinda like the current
GroupAccount
naming but don't have a strong preference either. It'd be good to have some thoughts from @aaronc who's the primarily designer of the group module afaik.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Then, how about
PolicyAccount
? I don't think we need to putGroup
everywhere.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I find this suggestion is a bit redundant "A decision policy is an interface for voting policies..."
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
How many decision policies have you guys experimented with? I'm just wondering how flexible this interface actually is. One wouldn't be able to create a DecisionPolicy for quadratic voting right?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
afaik, we've only had the
ThresholdDecisionPolicy
.I guess that for quadratic voting, it wouldn't be only about the decision policy but also how the members weights are managed.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is it possible for a
DecisionPolicy
to keep some state or is it ideal for it to be stateless. I guess another way of putting this is did you guys consider instead of giving the decision policy the entire tally that you just give it the vote. In this regard the decision policy would manage the tallyThere was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I feel this adds a great deal of inflexibility. If for example a proposal lasts two weeks, then not only must arriving and leaving members wait until the proposal is finished but member weights can not be updates until after the proposal is complete. I'm guessing a default way of assigning weights is based on the amount of tokens a member contributes. By freezing this, I feel we take away a lot of sovereignty for group members and usability of groups. This will be even more problematic in large groups with potentially thousands of members.
Have you considered taking a snapshot of the member weights at the moment the proposal is submitted and using that as the reference for the decision policy? Perhaps this is more of a future improvement
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I agree that this is very inflexible. It works quite well for the multisig use case, or even very-infrequently-updated committees. But is completely unsuited for frequently changing groups (like groups based on stake).
However, it is much more efficient for this (quite useful) use case. And adding snapshotting to a company multisig might be overkill. It should be clear what the scope of the group module is.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Right. That makes sense in the case where groups hardly change. I guess I was coming more from the angle of governance. Perhaps this could be handled by the
DecisionPolicy
. That way groups pick a decision policy that matches their use case. For relatively static groups, the decision policy voids a proposal when the group changes. For more dynamic groups, the decision policy takes a snapshot of the weights of all members at the submission of the proposalThere was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Group Account is a limited DAO contract. I don't think we should expect from the Group Account same complexity and same features as we would from a fully fledged DAO.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This whole issue is addressed more thoroughly in #9066. Briefly, I believe that when weights are calculated should be a pluggable part of
DecisionPolicy
. For many use cases, the final weights are ideal IMHO and tallying can simply be done at the end of voting. In other use cases, we need a snapshot at the start of voting and this brings other technical complexities we will need to address (see #9066 (reply in thread))