feat: ADR-012: Subspaces sections #866

RiccardoM · 2022-05-17T12:09:49Z

Description

This PR implements the subspaces sections as described inside ADR-012.

Closes: #856

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)

Signed-off-by: Riccardo Montagnin <[email protected]>

codecov · 2022-05-18T07:51:16Z

Codecov Report

Merging #866 (986bad3) into master (8379c67) will increase coverage by 0.82%.
The diff coverage is 87.02%.

❗ Current head 986bad3 differs from pull request most recent head c6e4946. Consider uploading reports for the commit c6e4946 to get more accurate results

@@            Coverage Diff             @@
##           master     #866      +/-   ##
==========================================
+ Coverage   81.82%   82.65%   +0.82%     
==========================================
  Files         110      114       +4     
  Lines        9316    10157     +841     
==========================================
+ Hits         7623     8395     +772     
- Misses       1373     1429      +56     
- Partials      320      333      +13

Impacted Files	Coverage Δ
x/relationships/keeper/hooks.go	`66.66% <0.00%> (-5.34%)`	⬇️
x/subspaces/types/models.go	`95.05% <ø> (+13.36%)`	⬆️
x/subspaces/types/msgs.go	`99.00% <ø> (+0.47%)`	⬆️
x/subspaces/types/permissions.go	`73.52% <ø> (ø)`
x/subspaces/legacy/v2/models.go	`30.76% <30.76%> (ø)`
x/posts/keeper/subspaces_hooks.go	`57.69% <41.66%> (-16.00%)`	⬇️
x/posts/types/models.go	`71.28% <50.00%> (-0.87%)`	⬇️
x/posts/keeper/alias_functions.go	`58.78% <51.28%> (-7.22%)`	⬇️
x/subspaces/types/codec.go	`53.65% <64.28%> (+5.51%)`	⬆️
x/subspaces/legacy/v3/store.go	`73.91% <73.91%> (ø)`
... and 41 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 8379c67...c6e4946. Read the comment docs.

Signed-off-by: Riccardo Montagnin <[email protected]>

…subspaces-sections-implementation

Signed-off-by: Riccardo Montagnin <[email protected]>

…ementation' into riccardo/subspaces-sections-implementation

Signed-off-by: Riccardo Montagnin <[email protected]>

x/subspaces/keeper/alias_functions.go

x/subspaces/keeper/genesis.go

x/relationships/keeper/hooks.go

x/subspaces/keeper/hooks.go

x/subspaces/keeper/msg_server.go

x/subspaces/keeper/sections_test.go

leobragaz · 2022-05-18T20:38:31Z

x/subspaces/types/genesis.go

 // ValidateGenesis validates the given genesis state and returns an error if something is invalid
 func ValidateGenesis(data *GenesisState) error {
 	// Make sure the initial subspace id is valid
-	if data.InitialSubspaceID <= uint64(len(data.Subspaces)) {
+	if data.InitialSubspaceID == 0 {


I suggest also to switch from 0 to a DefaultSubspace or RootSubspace constant

I don't agree with naming the subspace id 0 with DefaultSubspace or RootSubspace since this is an invalid value for pretty much any case (except user blocks). Those naming would somehow tell the user that it should be a valid value, when it really should not

Ah ok, gotcha. Do we have a RootSubspace concept? Like a default one?

No, there is not a default or root subspace. Every content will need to be placed inside a subspace that DApp developers need to create beforehand. They will not be able to create content if a non-zero subspace is not used.

The only subspace that is going to be different is going to be the one with id 0. This will be used from users to block people they don't want to be bothered by with DTag transfer requests. But it will not allow to create content inside it.

All the other ones will allow all the operations (content creations, blockage, etc).

leobragaz · 2022-05-18T20:43:30Z

x/subspaces/types/hooks.go

@@ -17,13 +17,16 @@ type SubspacesHooks interface {
 	AfterSubspaceSaved(ctx sdk.Context, subspaceID uint64)   // Must be called when a subspace is saved
 	AfterSubspaceDeleted(ctx sdk.Context, subspaceID uint64) // Must be called when a subspace is deleted

+	AfterSubspaceSectionSaved(ctx sdk.Context, subspaceID uint64, sectionID uint32)   // Must be called when a subspace section is saved
+	AfterSubspaceSectionDeleted(ctx sdk.Context, subspaceID uint64, sectionID uint32) // Must be called when a subspace section is deleted
+


I think we should we also add AfterSubspaceSectionMoved

I don't think it's worth it. That would require to have a custom keeper method that deals with moving sections. Instead, the current implementation will simply call AfterSubspaceSectionSaved when a section is moved (since it's just about changing its ParentID value). The module using this hook will simply be able to check the previously stored data and react properly if they are interested in doing so.

x/subspaces/types/keys_test.go

Co-authored-by: Leonardo Bragagnolo <[email protected]>

Signed-off-by: Riccardo Montagnin <[email protected]>

…ementation' into riccardo/subspaces-sections-implementation

Signed-off-by: Riccardo Montagnin <[email protected]>

…subspaces-sections-implementation

Signed-off-by: Riccardo Montagnin <[email protected]>

dadamu · 2022-05-31T11:30:56Z

x/subspaces/simulation/operations_groups.go

+	account = *acc
+
+	// Make sure the user can change this group's permissions
+	if subspace.Owner != account.Address.String() && k.IsMemberOfGroup(ctx, subspaceID, groupID, account.Address.String()) {


It can be replaced with k.HasPermission.

Thanks for pointing this out. I actually forgot to add this check inside the MsgMoveUserGroup handler function: we shouldn't allow users that are part of a group to manage that group as they might use this to get more permissions than the ones that were assigned to them. I've now added this check inside the handler as well.

x/subspaces/types/msgs.go

x/subspaces/simulation/operations_sections.go

dadamu · 2022-05-31T11:41:53Z

x/subspaces/types/genesis_test.go

-				nil,
-			),
+			name:      "invalid user returns no error",
+			entry:     types.NewUserPermission(1, 0, "", types.PermissionWrite),
 			shouldErr: true,
 		},


Missing invalid permission test case.

dadamu · 2022-05-31T11:53:14Z

Overall looks great to me. Just some minor things can be improved.

Co-authored-by: Paul <[email protected]>

Signed-off-by: Riccardo Montagnin <[email protected]>

x/subspaces/keeper/msg_server.go

dadamu · 2022-05-31T14:07:11Z

x/subspaces/keeper/msg_server.go

+	}
+
+	// Make sure that the user is not part of the group they want to change the permissions for, unless they are the owner
+	if subspace.Owner != msg.Signer && k.IsMemberOfGroup(ctx, msg.SubspaceID, msg.GroupID, signer.String()) {


In the following case, the moderator who has the ManageGroups permission in A section can move the B section group to the root.

Root | A | B

Instead of this check, It can be solved by k.HasPermission(ctx, group.SubspaceID, msg.NewSectionID, signer.String(), types.PermissionManageGroups), to check permission in the new section.

Updated the checks to include both PermissionManageGroups and PermissionSetUserPermissions for the destination section 👍

Co-authored-by: Paul <[email protected]>

Signed-off-by: Riccardo Montagnin <[email protected]>

dadamu

Ready to go to me

…subspaces-sections-implementation � Conflicts: � x/posts/keeper/genesis_test.go

Signed-off-by: Riccardo Montagnin <[email protected]>

RiccardoM added 3 commits May 16, 2022 14:07

started implementing the subspace sections

0e636aa

Signed-off-by: Riccardo Montagnin <[email protected]>

started fixing some tests

d7ac1c8

Signed-off-by: Riccardo Montagnin <[email protected]>

fixed tests

1472715

Signed-off-by: Riccardo Montagnin <[email protected]>

github-actions bot added x/CLI x/relationships x/subspaces Issue on the x/subspaces module labels May 17, 2022

RiccardoM added 2 commits May 17, 2022 15:23

fixing keeper tests

eba7f9e

Signed-off-by: Riccardo Montagnin <[email protected]>

fixed unit tests

85c693e

Signed-off-by: Riccardo Montagnin <[email protected]>

RiccardoM and others added 9 commits May 18, 2022 10:02

added missing decoder cases and tests

b407788

Signed-off-by: Riccardo Montagnin <[email protected]>

fixed sim genesis generation

4ab4b07

Signed-off-by: Riccardo Montagnin <[email protected]>

updated Swagger config to use subspaces v2

b98ddf8

Signed-off-by: Riccardo Montagnin <[email protected]>

Merge branch 'master' of github.com:desmos-labs/desmos into riccardo/…

272b89b

…subspaces-sections-implementation

Updated Swagger definition

923b838

updated swagger definition

97e783e

Signed-off-by: Riccardo Montagnin <[email protected]>

Merge remote-tracking branch 'origin/riccardo/subspaces-sections-impl…

f50c74c

…ementation' into riccardo/subspaces-sections-implementation

added sections simulation tests

ff639cc

Signed-off-by: Riccardo Montagnin <[email protected]>

added changeset entry

94c2f90

Signed-off-by: Riccardo Montagnin <[email protected]>

RiccardoM changed the title ~~feat: ADR-012: Subspaces sections~~ feat!: ADR-012: Subspaces sections May 18, 2022

RiccardoM requested review from dadamu and leobragaz May 18, 2022 09:08

RiccardoM marked this pull request as ready for review May 18, 2022 09:08

fixed unit test

f0770d1

Signed-off-by: Riccardo Montagnin <[email protected]>

leobragaz reviewed May 18, 2022

View reviewed changes

RiccardoM and others added 5 commits May 19, 2022 08:26

Update x/subspaces/keeper/alias_functions.go

0898cfe

Co-authored-by: Leonardo Bragagnolo <[email protected]>

Update x/subspaces/keeper/sections_test.go

ed64682

Co-authored-by: Leonardo Bragagnolo <[email protected]>

added RootSectionID definition

37192e9

Signed-off-by: Riccardo Montagnin <[email protected]>

Merge remote-tracking branch 'origin/riccardo/subspaces-sections-impl…

5ae167d

…ementation' into riccardo/subspaces-sections-implementation

added missing SplitUserAddressPermissionKey tests

ab12957

Signed-off-by: Riccardo Montagnin <[email protected]>

RiccardoM requested a review from leobragaz May 19, 2022 07:13

RiccardoM added 4 commits May 31, 2022 10:26

Merge branch 'master' of github.com:desmos-labs/desmos into riccardo/…

7b860b6

…subspaces-sections-implementation

fixed lint

33ad9ae

Signed-off-by: Riccardo Montagnin <[email protected]>

added missing test check

1709cb2

Signed-off-by: Riccardo Montagnin <[email protected]>

fixed simulation tests

67a88d0

Signed-off-by: Riccardo Montagnin <[email protected]>

github-actions bot added the x/profiles Module that allows to create and manage decentralized social profiles label May 31, 2022

RiccardoM requested a review from dadamu May 31, 2022 09:04

dadamu reviewed May 31, 2022

View reviewed changes

x/subspaces/types/msgs.go Outdated Show resolved Hide resolved

dadamu reviewed May 31, 2022

View reviewed changes

x/subspaces/types/msgs.go Outdated Show resolved Hide resolved

dadamu reviewed May 31, 2022

View reviewed changes

x/subspaces/simulation/operations_sections.go Outdated Show resolved Hide resolved

dadamu reviewed May 31, 2022

View reviewed changes

x/subspaces/simulation/operations_sections.go Outdated Show resolved Hide resolved

dadamu reviewed May 31, 2022

View reviewed changes

RiccardoM and others added 5 commits May 31, 2022 14:15

Update x/subspaces/types/msgs.go

f564878

Co-authored-by: Paul <[email protected]>

Update x/subspaces/simulation/operations_sections.go

30769cc

Co-authored-by: Paul <[email protected]>

Update x/subspaces/types/msgs.go

3405b36

Co-authored-by: Paul <[email protected]>

Update x/subspaces/simulation/operations_sections.go

97460cf

Co-authored-by: Paul <[email protected]>

added missing check inside MoveUserGroup msg handler

39125ab

Signed-off-by: Riccardo Montagnin <[email protected]>

RiccardoM requested a review from dadamu May 31, 2022 12:40

RiccardoM added the automerge Automatically merge PR once all prerequisites pass label May 31, 2022

dadamu reviewed May 31, 2022

View reviewed changes

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

dadamu reviewed May 31, 2022

View reviewed changes

RiccardoM and others added 2 commits May 31, 2022 17:31

Update x/subspaces/keeper/msg_server.go

a65a7d2

Co-authored-by: Paul <[email protected]>

updated permissions checks inside MsgMoveUserGroup message handler

986bad3

Signed-off-by: Riccardo Montagnin <[email protected]>

RiccardoM requested a review from dadamu May 31, 2022 16:15

dadamu approved these changes Jun 1, 2022

View reviewed changes

RiccardoM added 2 commits June 1, 2022 07:15

Merge branch 'master' of github.com:desmos-labs/desmos into riccardo/…

32ef4ff

…subspaces-sections-implementation � Conflicts: � x/posts/keeper/genesis_test.go

fixed conflicts

c6e4946

Signed-off-by: Riccardo Montagnin <[email protected]>

mergify bot merged commit d9a708d into master Jun 1, 2022

mergify bot deleted the riccardo/subspaces-sections-implementation branch June 1, 2022 05:21

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat: ADR-012: Subspaces sections #866

feat: ADR-012: Subspaces sections #866

RiccardoM commented May 17, 2022 •

edited

Loading

codecov bot commented May 18, 2022 •

edited

Loading

leobragaz May 18, 2022

RiccardoM May 19, 2022

leobragaz May 19, 2022

RiccardoM May 19, 2022

leobragaz May 18, 2022

RiccardoM May 19, 2022

dadamu May 31, 2022

RiccardoM May 31, 2022

dadamu May 31, 2022

RiccardoM May 31, 2022

dadamu commented May 31, 2022

dadamu May 31, 2022 •

edited

Loading

RiccardoM May 31, 2022

dadamu left a comment

feat: ADR-012: Subspaces sections #866

feat: ADR-012: Subspaces sections #866

Conversation

RiccardoM commented May 17, 2022 • edited Loading

Description

Author Checklist

Reviewers Checklist

codecov bot commented May 18, 2022 • edited Loading

Codecov Report

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

dadamu commented May 31, 2022

dadamu May 31, 2022 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

dadamu left a comment

Choose a reason for hiding this comment

RiccardoM commented May 17, 2022 •

edited

Loading

codecov bot commented May 18, 2022 •

edited

Loading

dadamu May 31, 2022 •

edited

Loading