Add Group module #154
Add Group module #154
Conversation
Codecov Report
@@ Coverage Diff @@
## master #154 +/- ##
===========================================
- Coverage 65.68% 15.35% -50.33%
===========================================
Files 30 48 +18
Lines 1457 11388 +9931
===========================================
+ Hits 957 1749 +792
- Misses 406 9429 +9023
- Partials 94 210 +116 |
|
@aaronc @robert-zaremba @anilcse could you have another look? Thanks. |
Co-authored-by: Anil Kumar Kammari <anil@vitwit.com>
| groupMember := group.GroupMember{GroupId: req.GroupId, | ||
| Member: &group.Member{ | ||
| Address: req.MemberUpdates[i].Address, | ||
| Weight: req.MemberUpdates[i].Weight, | ||
| Metadata: req.MemberUpdates[i].Metadata, | ||
| }, | ||
| } |
There was a problem hiding this comment.
let's move this to handle add + update
There was a problem hiding this comment.
we need it before in "handle delete" or did you mean something else?
There was a problem hiding this comment.
just it seems like it's initialized too early in the code
| if newMemberWeight.Cmp(apd.New(0, 0)) == 0 { | ||
| // Handle delete for members with zero weight. | ||
| if newMemberWeight.IsZero() { | ||
| // We can't delete a group member that doesn't already exist. | ||
| if !found { | ||
| return sdkerrors.Wrap(orm.ErrNotFound, "unknown member") |
There was a problem hiding this comment.
There's no harm in ignoring this case right?
There was a problem hiding this comment.
yeah I guess groupMemberTable.Delete below would just fail in this case, but that would be an unnecessary access to the store given that we already know that it should fail, wdyt?
There was a problem hiding this comment.
We just wouldn't even execute this block... but maybe let's keep it as is because we would need to adjust tests and I don't want to further churn on this PR.
Co-authored-by: Aaron Craelius <aaron@regen.network>
Co-authored-by: Aaron Craelius <aaron@regen.network>
| rpc GroupMembers(QueryGroupMembersRequest) returns (QueryGroupMembersResponse); | ||
|
|
||
| // GroupsByAdmin queries groups by admin address. | ||
| rpc GroupsByAdmin(QueryGroupsByAdminRequest) returns (QueryGroupsByAdminResponse); |
There was a problem hiding this comment.
It seams we are trying to implement relational DB - this queries should be handled by an off-chain indexer, or only added later when needed for state machine operations.
There was a problem hiding this comment.
That's a bigger design issue. This decision was made a long time ago
| rpc GroupsByAdmin(QueryGroupsByAdminRequest) returns (QueryGroupsByAdminResponse); | ||
|
|
||
| // GroupAccountsByGroup queries group accounts by group id. | ||
| rpc GroupAccountsByGroup(QueryGroupAccountsByGroupRequest) returns (QueryGroupAccountsByGroupResponse); |
There was a problem hiding this comment.
IMHO, would be better to remove Group prefix to avoid name clashes:
groupClient.GroupAccountsByGroup
There was a problem hiding this comment.
we can deal with that later... can you open an issue or PR if you think there's something that can be improved
There was a problem hiding this comment.
@blushi - do you have an opinion about naming? If you also find that we could improve naming then I'm happy to create a PR.
There was a problem hiding this comment.
@robert-zaremba you mean having AccountsByGroup instead for instance? I don't have a strong preference, although I think keeping the Group prefix makes it more explicit.
There was a problem hiding this comment.
I think keeping the Group prefix makes it more explicit.
This context is already provided by a client / server object, right?
groupClient.AccountsByGroup
vs
groupClient.GroupAccountsByGroup // we mention group 3 times, and it also sounds that we are grouping by (like in SQL)
Closes #136
Closes #145
Closes #198
Closes regen-network/cosmos-modules#69
Closes regen-network/cosmos-modules#72
Proposed Changes
types.proto,tx.protoandgenesis.proto(no queries implemented for now).events.proto) for group create/update and group account create/update.Msgservice.sdk.Routerfor now for the PoC. Further down the road we'll want to replace it with ADR 033 approach (Implement ADR 033 #157).Msgservice.Marshalerto unpackAnys properly (GetCachedValue()returnednilforAnys returned from the ORM).app.go.Follow-ups
[x/group] Add integration tests #197 Add integration tests.
[x/group] Add CLI support #209 Add CLI support.
Currently, we are using different endpoints for updating a group whether we wanna update its members, admin or comments. It would be better UX-wise to just have one update endpoint if possible.
Implement group account update [x/group] Implement GroupAccount updates #224
Add other event types [x/group] Events #215
Update
GenesisState[x/group] Genesis state and import/export #214As described above, we're still using
sdk.Routerto route messages to their handlers. This means that we cannot use the group module right now to route messages from thedataorecocreditmodules, unless we implement some kind of temporary handlers for them. Eventually, the group module should follow ADR 033 approach (each group account would be a sub module account according to ADR 028, then using the group’sModuleKey, the proposal handler would execute messages usingModuleKey.Invoke).Replace
x/group/types/conditions.gowith ADR 028 Upgrade codebase to ADR 028 addresses #211Remaining issues from cosmos-modules repo should be sorted priority-wise and migrated to this repo @clevinson.
Feel free to submit a Draft Pull Request as soon as you start working on something. Before you mark your PR as Ready For Review, make sure you've checked off the following boxes or indicated N/A (not applicable):
CHANGELOG.mdfollowing https://keepachangelog.comThanks!