feat: Add server implementation of Group module #10570

likhita-809 · 2021-11-18T17:09:15Z

Description

Closes: #9897
Closes: #9905

Adds server implementation of Group module and wires it up in simapp

Author Checklist

All items are required. Please add a note to the item if the item is not applicable and
please add links to any relevant follow up issues.

I have...

Reviewers Checklist

All items are required. Please add a note if the item is not applicable and please add
your handle next to the items reviewed if you only reviewed selected items.

I have...

confirmed the correct type prefix in the PR title
confirmed ! in the type prefix if API or client breaking change
confirmed all author checklist items have been addressed
reviewed state machine logic
reviewed API design and naming
reviewed documentation is accurate
reviewed tests and test coverage
manually tested (if applicable)

…khita/group-server-implementation

types/module/server/derived_module_key.go

types/module/server/id.go

types/module/server/manager.go

types/module/server/module.go

types/module/server/module_key.go

types/module/server/router.go

types/module/server/testutil.go

x/group/errors/orm.go

x/group/keeper/keeper_test.go

x/group/keeper/proposal_executor.go

x/group/keeper/msg_server.go

types/module/server/address.go

…khita/group-server-implementation

codecov · 2021-11-23T11:59:33Z

Codecov Report

Merging #10570 (81dd664) into master (cf6ace5) will increase coverage by 0.37%.
The diff coverage is 67.32%.

❗ Current head 81dd664 differs from pull request most recent head f03ac5a. Consider uploading reports for the commit f03ac5a to get more accurate results

@@            Coverage Diff             @@
##           master   #10570      +/-   ##
==========================================
+ Coverage   65.03%   65.40%   +0.37%     
==========================================
  Files         612      634      +22     
  Lines       57798    60651    +2853     
==========================================
+ Hits        37590    39670    +2080     
- Misses      18101    18681     +580     
- Partials     2107     2300     +193

Impacted Files	Coverage Δ
baseapp/baseapp.go	`66.96% <ø> (-0.15%)`	⬇️
client/flags/flags.go	`20.33% <0.00%> (-1.48%)`	⬇️
client/query.go	`30.58% <0.00%> (-0.74%)`	⬇️
client/tx/factory.go	`25.36% <0.00%> (-2.01%)`	⬇️
types/tx/msgs.go	`0.00% <ø> (ø)`
types/tx/types.go	`0.00% <0.00%> (ø)`
x/auth/client/cli/tips.go	`0.00% <0.00%> (ø)`
x/auth/client/cli/tx_multisign.go	`0.00% <0.00%> (ø)`
x/auth/client/cli/validate_sigs.go	`0.00% <0.00%> (ø)`
x/auth/tx/builder.go	`80.98% <ø> (ø)`
... and 65 more

…khita/group-server-implementation

…os/cosmos-sdk into likhita/group-server-implementation

amaury1093

lgtm, I looked more closely into these parts:

Exec() server impl
proto defs

blushi · 2021-12-07T14:06:13Z

@anilcse @robert-zaremba @aaronc As approvers of the corresponding PR on regen-ledger: regen-network/regen-ledger#154, I'm requesting your reviews here in particular to try to make the review process faster for the group module migration to cosmos-sdk.

Here are some important changes that have been made since the original PR though:

use MsgServiceRouter instead of LegacyRouter for executing proposal msgs

use ADR 028 for group account address

use AppModuleBasic and AppModule for module definition (instead of our custom Modules on regen-ledger)

up @anilcse @robert-zaremba @aaronc could you have a look so we can move forward with this PR please? It's blocking some follow-up group module PRs. Thanks.

x/group/keeper/msg_server.go

robert-zaremba · 2021-12-07T23:33:42Z

x/group/keeper/msg_server.go

+		}
+
+		accountDerivationKey = buf.Bytes()
+		accountAddr = address.Module(group.ModuleName, accountDerivationKey)


are we 100% sure we won't have any other account type in the group module? If there will be other one (eg other type of groups, or some helper accounts), then we should use derivation:

parentAcc := address.Module(group.ModuleName, groupTablePrefix) groupAddr := address.Derive(parentAcc, bytes_id)

Yeah I was hesitating on that but I think it's probably safest to use derivation indeed.
But I don't get why we would use groupTablePrefix here, maybe groupAccountTablePrefix would be more appropriate in this particular case of group account address creation.

@robert-zaremba it's now using derivation, lmk what you think

robert-zaremba · 2021-12-07T23:35:40Z

x/group/keeper/msg_server.go

+		}
+
+		acc := k.accKeeper.NewAccount(ctx, &authtypes.ModuleAccount{
+			BaseAccount: &authtypes.BaseAccount{


Do we really need a BaseAccount for groups?

what would we use instead? It's a ModuleAccount btw, not just a BaseAccount

robert-zaremba · 2021-12-07T23:41:29Z

After thinking more about the addessables and ids, I think that each group should be identified by an address, not id uint64.
The ID is not needed, we can only keep the internal counter to generate new addresses. The we can use that address to compose the group directly with anything, without doing an additional index nor having a need for GroupInfo. Also it will normalize queries: We can use address everywhere and think about it as an account, without having an associated BaseAccount.

anilcse

lgtm, I agree with @robert-zaremba on replacing group-ids

blushi · 2021-12-09T08:42:44Z

lgtm, I agree with @robert-zaremba on replacing group-ids

A group is just an aggregation of accounts with associated weights, it doesn't have any decision or execution power without a decision policy, while a group with a decision policy = a group account does and has an address.
You can double check the docs: https://docs.regen.network/modules/group/01_concepts.html#group that outlines the current design:

Group accounts are abstracted from groups because a single group may have multiple decision policies for different types of actions. Managing group membership separately from decision policies results in the least overhead and keeps membership consistent across different policies.

I partially worked on this PR so I can't really review it

blushi · 2021-12-10T09:39:11Z

@robert-zaremba I'll put automerge on so we can move forward with this and unblock the rest of the group PRs. Please create a separate issue if there's anything that you'd like to be addressed in a follow-up PR.

faddat · 2021-12-12T07:54:28Z

If we look at the go.sum file here, this brings the trouble that left us in v0.44.5 back in.

cosmos/gaia#1099

faddat · 2021-12-12T13:40:44Z

I might have been reacting to nothing?

Or maybe not?

https://go.dev/ref/mod#minimal-version-selection

via @marbar3778

## Description Closes: cosmos#9897 Closes: cosmos#9905 Adds server implementation of Group module and wires it up in simapp --- ### Author Checklist *All items are required. Please add a note to the item if the item is not applicable and please add links to any relevant follow up issues.* I have... - [x] included the correct [type prefix](https://github.com/commitizen/conventional-commit-types/blob/v3.0.0/index.json) in the PR title - [ ] added `!` to the type prefix if API or client breaking change - [x] targeted the correct branch (see [PR Targeting](https://github.com/cosmos/cosmos-sdk/blob/master/CONTRIBUTING.md#pr-targeting)) - [x] provided a link to the relevant issue or specification - [x] followed the guidelines for [building modules](https://github.com/cosmos/cosmos-sdk/blob/master/docs/building-modules) - [x] included the necessary unit and integration [tests](https://github.com/cosmos/cosmos-sdk/blob/master/CONTRIBUTING.md#testing) - [ ] added a changelog entry to `CHANGELOG.md` - [x] included comments for [documenting Go code](https://blog.golang.org/godoc) - [ ] updated the relevant documentation or specification - [x] reviewed "Files changed" and left comments if necessary - [ ] confirmed all CI checks have passed ### Reviewers Checklist *All items are required. Please add a note if the item is not applicable and please add your handle next to the items reviewed if you only reviewed selected items.* I have... - [ ] confirmed the correct [type prefix](https://github.com/commitizen/conventional-commit-types/blob/v3.0.0/index.json) in the PR title - [ ] confirmed `!` in the type prefix if API or client breaking change - [ ] confirmed all author checklist items have been addressed - [ ] reviewed state machine logic - [ ] reviewed API design and naming - [ ] reviewed documentation is accurate - [ ] reviewed tests and test coverage - [ ] manually tested (if applicable)

likhita-809 added 9 commits October 29, 2021 14:47

remove Request suffix from Msg types

32cb63a

wip

fe6a8d5

Merge branch 'master' of https://github.com/cosmos/cosmos-sdk into li…

246484e

…khita/group-server-implementation

wip

e274069

wip

4203f24

Merge branch 'master' of https://github.com/cosmos/cosmos-sdk into li…

743520c

…khita/group-server-implementation

wip

3f4179e

Merge branch 'master' of https://github.com/cosmos/cosmos-sdk into li…

acad315

…khita/group-server-implementation

wip

be27cba

likhita-809 changed the title ~~Add server implementation of Group module~~ feat: Add server implementation of Group module Nov 18, 2021

likhita-809 added 2 commits November 18, 2021 22:42

run go mod tidy

71c2dee

small change

a864359

blushi reviewed Nov 19, 2021

View reviewed changes

x/group/keeper/msg_server.go Outdated Show resolved Hide resolved

blushi reviewed Nov 19, 2021

View reviewed changes

types/module/server/address.go Outdated Show resolved Hide resolved

wip

18794db

github-actions bot added the C:x/feegrant label Nov 22, 2021

likhita-809 added 11 commits November 22, 2021 17:09

Merge branch 'master' of https://github.com/cosmos/cosmos-sdk into li…

3851900

…khita/group-server-implementation

wip

36b07b3

fix something

efb22ee

wip

1edaf8c

Merge branch 'master' of https://github.com/cosmos/cosmos-sdk into li…

3d5c37d

…khita/group-server-implementation

wip

87054b8

wip

164a153

run go mod tidy

0f6028b

wip

d4156a2

wip

c473327

fix tests

491ff91

Merge branch 'master' of https://github.com/cosmos/cosmos-sdk into li…

8bcbbf7

…khita/group-server-implementation

blushi added 2 commits December 2, 2021 12:41

Merge branch 'likhita/group-server-implementation' of github.com:cosm…

de9584d

…os/cosmos-sdk into likhita/group-server-implementation

tidy

b6442dc

blushi requested review from amaury1093 and anilcse December 2, 2021 12:17

amaury1093 approved these changes Dec 2, 2021

View reviewed changes

Merge branch 'master' into likhita/group-server-implementation

48c10a1

robert-zaremba reviewed Dec 7, 2021

View reviewed changes

blushi added 2 commits December 8, 2021 12:37

Use BigEndian

7cdeccc

Merge branch 'master' into likhita/group-server-implementation

9bbcaba

anilcse approved these changes Dec 8, 2021

View reviewed changes

Merge branch 'master' into likhita/group-server-implementation

f911eda

blushi added 2 commits December 9, 2021 10:50

Use derivation for group acc addr

81dd664

Merge branch 'master' into likhita/group-server-implementation

9f0b1e9

github-actions bot added the C:Simulations label Dec 10, 2021

blushi added A:automerge Automatically merge PR once all prerequisites pass. and removed C:Simulations C:x/feegrant labels Dec 10, 2021

Rm sim

f03ac5a

github-actions bot added the C:x/feegrant label Dec 10, 2021

mergify bot merged commit 3495691 into master Dec 10, 2021

mergify bot deleted the likhita/group-server-implementation branch December 10, 2021 11:02

faddat mentioned this pull request Dec 12, 2021

Import repair cosmos/gaia#1099

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat: Add server implementation of Group module #10570

feat: Add server implementation of Group module #10570

likhita-809 commented Nov 18, 2021 •

edited by blushi

Loading

codecov bot commented Nov 23, 2021 •

edited

Loading

amaury1093 left a comment

blushi commented Dec 7, 2021 •

edited

Loading

robert-zaremba Dec 7, 2021

blushi Dec 8, 2021

blushi Dec 9, 2021

robert-zaremba Dec 7, 2021

blushi Dec 8, 2021

robert-zaremba commented Dec 7, 2021

anilcse left a comment

blushi commented Dec 9, 2021

blushi commented Dec 10, 2021

faddat commented Dec 12, 2021 •

edited

Loading

faddat commented Dec 12, 2021

feat: Add server implementation of Group module #10570

feat: Add server implementation of Group module #10570

Conversation

likhita-809 commented Nov 18, 2021 • edited by blushi Loading

Description

Author Checklist

Reviewers Checklist

codecov bot commented Nov 23, 2021 • edited Loading

Codecov Report

amaury1093 left a comment

Choose a reason for hiding this comment

blushi commented Dec 7, 2021 • edited Loading

robert-zaremba Dec 7, 2021

Choose a reason for hiding this comment

blushi Dec 8, 2021

Choose a reason for hiding this comment

blushi Dec 9, 2021

Choose a reason for hiding this comment

robert-zaremba Dec 7, 2021

Choose a reason for hiding this comment

blushi Dec 8, 2021

Choose a reason for hiding this comment

robert-zaremba commented Dec 7, 2021

anilcse left a comment

Choose a reason for hiding this comment

blushi commented Dec 9, 2021

blushi commented Dec 10, 2021

faddat commented Dec 12, 2021 • edited Loading

faddat commented Dec 12, 2021

likhita-809 commented Nov 18, 2021 •

edited by blushi

Loading

codecov bot commented Nov 23, 2021 •

edited

Loading

blushi commented Dec 7, 2021 •

edited

Loading

faddat commented Dec 12, 2021 •

edited

Loading