feat: implement ADR-019: Subspace post migration #1157

dadamu · 2023-05-23T09:27:54Z

Description

This PR implements ADR-019: Subspace post migration.

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)

codecov · 2023-05-23T09:36:49Z

Codecov Report

Patch coverage: 100.00% and project coverage change: +0.17 🎉

Comparison is base (fdb18ed) 80.12% compared to head (6482721) 80.30%.

Additional details and impacted files

@@            Coverage Diff             @@
##           master    #1157      +/-   ##
==========================================
+ Coverage   80.12%   80.30%   +0.17%     
==========================================
  Files         189      189              
  Lines       16526    16675     +149     
==========================================
+ Hits        13242    13391     +149     
  Misses       2690     2690              
  Partials      594      594

Impacted Files	Coverage Δ
x/posts/keeper/msg_server.go	`91.99% <100.00%> (+1.57%)`	⬆️
x/posts/keeper/polls.go	`97.89% <100.00%> (+0.19%)`	⬆️
x/posts/types/models.go	`79.03% <100.00%> (+1.66%)`	⬆️
x/posts/types/msgs.go	`97.53% <100.00%> (+0.32%)`	⬆️

... and 3 files with indirect coverage changes

☔ View full report in Codecov by Sentry.
📢 Do you have feedback about the report comment? Let us know in this issue.

dadamu · 2023-05-30T09:44:15Z

x/posts/keeper/msg_server.go

+	k.SetNextAttachmentID(ctx, msg.TargetSubspaceID, newPostID, newAttachmentID+1)
+
+	// Delete the post
+	k.Keeper.DeletePost(ctx, msg.SubspaceID, msg.PostID)


DeletePost calls the PostDeletedHook, which removes all the incompatibility references managed by other modules already, so no need to implement the PostMovedHook.

Note to self: DeletePost calls DeleteAttachment which removes the poll from the active queue (if it was there).

RiccardoM · 2023-06-03T15:43:19Z

proto/desmos/posts/v3/msgs.proto

@@ -41,6 +41,9 @@ service Msg {
  //
  // Since: Desmos 5.0.0
  rpc UpdateParams(MsgUpdateParams) returns (MsgUpdateParamsResponse);
+
+   // MovePost allows users to move their own posts to another subspace


We should probably add a Since: Desmos 5.1.0 here

Maybe Desmos 6.0.0 is better since it would be Desmos v6 feature on roadmap

proto/desmos/posts/v3/msgs.proto

x/posts/keeper/msg_server.go

RiccardoM · 2023-06-03T15:46:19Z

x/posts/keeper/msg_server.go

+	}
+
+	// Check the permission to move the post
+	canMove := post.Author == msg.Owner && k.HasPermission(ctx, msg.TargetSubspaceID, msg.TargetSectionID, msg.Owner, types.PermissionWrite)


Probably the post.Author == msg.Owner check should be split, so that the user can know exactly what's wrong (they are not they author of the post)

x/posts/keeper/msg_server.go

Co-authored-by: Riccardo <riccardo.montagnin@gmail.com>

x/posts/client/cli/tx.go

RiccardoM · 2023-06-08T20:44:46Z

x/posts/client/cli/cli_test.go

+		{
+			name: "invalid subspace id returns error",
+			args: []string{
+				"", "1", "2", "1",


We should add the --from flag to all these tests in order to make sure that they don't fail due to the invalid signer address. Otherwise we will not catch errors like the one highlighted above

x/posts/keeper/msg_server.go

x/posts/types/models.go

x/posts/keeper/msg_server.go

x/posts/types/models.go

RiccardoM · 2023-06-08T20:54:25Z

x/posts/keeper/msg_server.go

+	newAttachmentID := uint32(0)
+	k.IteratePostAttachments(ctx, msg.SubspaceID, msg.PostID, func(attachment types.Attachment) (stop bool) {
+		newAttachmentID++
+		attachmentMove := types.NewAttachmentMove(msg.TargetSubspaceID, newPostID, newAttachmentID)
+		updatedAttachment := attachmentMove.Update(attachment)
+		k.SaveAttachment(ctx, updatedAttachment)
+
+		if types.IsActivePoll(updatedAttachment) {
+			k.InsertActivePollQueue(ctx, updatedAttachment)
+		}
+		return false
+	})


Why is this being done? Why can't we preserve the attachment ids that were used inside the old post instead? They are unique to the post itself anyway. If we preserve them we have less writes (which means less gas used) and we can simply set the NextAttachmentID to be max(post attachment ids) + 1 which is way simpler. This way we would also make it easier for external tools that could simply update the post id instead of having to delete the attachments and re-storing them as well

Agree with you, we should preserve the original attachment id is correct, and if anything wrong invariant will alert when simulation test. I was confused that we should reset id or apply the original id here, finally I decided to reset the id because it will be error if ids are wrongly set.

By the way, I don't think it brings less writes since it actually adds some keys for moved attachments then deleted the original ones. The storage side is performing the same operations like:

store.Set(movedAttachmentKey, movedAttachment) store.Delete(oldAttachmentKey, oldAttachment)

RiccardoM · 2023-06-08T20:55:50Z

x/posts/types/models.go

+// IsActivePoll tells whether the given attachment represents a active poll or not
+func IsActivePoll(attachment Attachment) bool {


Instead of relying on a check of FinalTallyResulsts I think it's best to check the storage instead, running through the list of active polls. This would make sure that if there is any bug with the setting of the tally results we do not end up with a bug here as well

Co-authored-by: Riccardo <riccardo.montagnin@gmail.com>

RiccardoM

Great job! 💯

RiccardoM · 2023-06-12T16:22:15Z

x/posts/keeper/msg_server.go

+	k.SetNextAttachmentID(ctx, msg.TargetSubspaceID, newPostID, newAttachmentID+1)
+
+	// Delete the post
+	k.Keeper.DeletePost(ctx, msg.SubspaceID, msg.PostID)


Note to self: DeletePost calls DeleteAttachment which removes the poll from the active queue (if it was there).

dadamu requested a review from a team as a code owner May 23, 2023 09:27

dadamu marked this pull request as draft May 23, 2023 09:28

dadamu force-pushed the paul/DCD-324/impl-post-transfer branch 2 times, most recently from bdd7f42 to 6a812ba Compare May 29, 2023 07:43

github-actions bot added the x/CLI label May 29, 2023

dadamu added 7 commits May 30, 2023 16:50

feat: implement types

24ccbdd

feat: implement msg server

58198b2

chore: fix typos

2ffac13

feat: implement post attachments migration

181adcf

test: add missing tests

c1a8cdf

feat: add cli

540c7f1

chore: improve move post msg validate

3df8da9

dadamu force-pushed the paul/DCD-324/impl-post-transfer branch from 5e644dd to 3df8da9 Compare May 30, 2023 08:54

dadamu added 3 commits May 30, 2023 17:22

feat: add simulation tests

01f7d96

fix: fix next post id of the target subspace

e8324f4

chore: add changeset entry

531f071

dadamu marked this pull request as ready for review May 30, 2023 09:41

dadamu commented May 30, 2023

View reviewed changes

RiccardoM reviewed Jun 3, 2023

View reviewed changes

dadamu and others added 3 commits June 5, 2023 13:13

Update proto/desmos/posts/v3/msgs.proto

8e64ab4

Co-authored-by: Riccardo <riccardo.montagnin@gmail.com>

Update x/posts/keeper/msg_server.go

8a35d0e

Co-authored-by: Riccardo <riccardo.montagnin@gmail.com>

chore: apply suggestions

3b52776

dadamu force-pushed the paul/DCD-324/impl-post-transfer branch from 965b5e9 to 3b52776 Compare June 5, 2023 07:23

dadamu added 2 commits June 5, 2023 16:38

chore: improve parameters of IteratePostAttachments

6bbed63

chore: removed merged entries

9526664

dadamu requested a review from RiccardoM June 5, 2023 14:28

RiccardoM reviewed Jun 8, 2023

View reviewed changes

dadamu and others added 2 commits June 12, 2023 14:24

Update x/posts/keeper/msg_server.go

12284c2

Co-authored-by: Riccardo <riccardo.montagnin@gmail.com>

Update x/posts/keeper/msg_server.go

3e32b47

Co-authored-by: Riccardo <riccardo.montagnin@gmail.com>

dadamu and others added 5 commits June 12, 2023 14:26

Update x/posts/types/models.go

54d1c01

Co-authored-by: Riccardo <riccardo.montagnin@gmail.com>

Update x/posts/types/models.go

e932796

Co-authored-by: Riccardo <riccardo.montagnin@gmail.com>

Update x/posts/client/cli/tx.go

e9c1c5c

Co-authored-by: Riccardo <riccardo.montagnin@gmail.com>

chore: apply suggestions on keeper

eec8c50

chore: apply suggestions on cli

6482721

dadamu requested a review from RiccardoM June 12, 2023 07:56

RiccardoM approved these changes Jun 12, 2023

View reviewed changes

RiccardoM merged commit b9244c0 into master Jun 12, 2023
35 checks passed

RiccardoM deleted the paul/DCD-324/impl-post-transfer branch June 12, 2023 16:31

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat: implement ADR-019: Subspace post migration #1157

feat: implement ADR-019: Subspace post migration #1157

dadamu commented May 23, 2023 •

edited by jira bot

codecov bot commented May 23, 2023 •

edited

dadamu May 30, 2023

RiccardoM Jun 12, 2023

RiccardoM Jun 3, 2023

dadamu Jun 5, 2023

RiccardoM Jun 3, 2023

RiccardoM Jun 8, 2023

RiccardoM Jun 8, 2023

dadamu Jun 12, 2023

RiccardoM Jun 8, 2023

RiccardoM left a comment

RiccardoM Jun 12, 2023

		// IsActivePoll tells whether the given attachment represents a active poll or not
		func IsActivePoll(attachment Attachment) bool {

feat: implement ADR-019: Subspace post migration #1157

feat: implement ADR-019: Subspace post migration #1157

Conversation

dadamu commented May 23, 2023 • edited by jira bot

Description

Author Checklist

Reviewers Checklist

codecov bot commented May 23, 2023 • edited

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

RiccardoM left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

dadamu commented May 23, 2023 •

edited by jira bot

codecov bot commented May 23, 2023 •

edited