feat: implement ADR-20: Post ownership transfer #1145

dadamu · 2023-05-16T08:54:54Z

Description

This PR implements ADR-20: Post ownership transfer.

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-16T09:03:10Z

Codecov Report

Patch coverage: 96.53% and project coverage change: +0.47 🎉

Comparison is base (b9244c0) 80.30% compared to head (ff940ce) 80.77%.

❗ Current head ff940ce differs from pull request most recent head 8c4cccc. Consider uploading reports for the commit 8c4cccc to get more accurate results

Additional details and impacted files

@@            Coverage Diff             @@
##           master    #1145      +/-   ##
==========================================
+ Coverage   80.30%   80.77%   +0.47%     
==========================================
  Files         189      193       +4     
  Lines       16675    17159     +484     
==========================================
+ Hits        13391    13861     +470     
- Misses       2690     2701      +11     
- Partials      594      597       +3

Impacted Files	Coverage Δ
x/posts/module.go	`11.92% <0.00%> (-0.25%)`	⬇️
x/posts/keeper/grpc_query.go	`81.36% <80.00%> (-0.32%)`	⬇️
x/posts/keeper/alias_functions.go	`69.71% <85.00%> (+2.19%)`	⬆️
x/posts/types/genesis.go	`94.62% <90.00%> (-1.14%)`	⬇️
x/posts/keeper/invariants.go	`93.36% <94.44%> (+0.24%)`	⬆️
x/posts/types/models.go	`81.01% <98.18%> (+1.97%)`	⬆️
x/posts/keeper/genesis.go	`95.06% <100.00%> (+0.39%)`	⬆️
x/posts/keeper/msg_server.go	`92.00% <100.00%> (+0.01%)`	⬆️
x/posts/keeper/msg_server_post_owner_transfer.go	`100.00% <100.00%> (ø)`
x/posts/keeper/post_owner_transfer.go	`100.00% <100.00%> (ø)`
... and 6 more

... and 1 file 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-16T12:12:58Z

docs/architecture/adr-020-post-ownership-change.md

-IncomingPostOwnerTransferRequestPrefix | SubspaceID | ReceiverAddress | PostID | -> Protobuf(PostOwnerTransferRequest)
+PostOwnerTransferRequestPrefix | SubspaceID | PostID | -> Protobuf(PostOwnerTransferRequest)
 ```

-This structure enables the receiver to easily manage incoming requests by iterating over all requests with a given subspace ID and receiver address, which will be the most frequently used query.
+This structure enables Desmos to easily manage requests by iterating over all requests with a given subspace ID and post ID when a post is moved or deleted.


@RiccardoM
I have identified an issue with this design: we are unable to determine the post's ownership receiver when it is deleted or moved. This leads to a time complexity of O(n) when deleting a post, as it requires the following steps:

Iterate over all requests by subspace ID.

Find the post ID of the request that matches the post to be deleted.

Delete the corresponding request store key.

To address this issue, I have simplified the store key implementation, resulting in an improved time complexity of O(1) for the post deletion process.

dadamu · 2023-05-18T11:45:26Z

app/params/weights.go

@@ -37,13 +37,6 @@ const (
 	DefaultWeightMsgGrantAllowance              int = 20
 	DefaultWeightMsgRevokeAllowance             int = 5

-	DefaultWeightMsgCreatePost           int = 80


Move to operation.go to be easier management

dadamu · 2023-05-18T11:46:41Z

docs/architecture/adr-020-post-ownership-change.md

+The solution outlined above is fully backwards compatible since it introduces a new __owner__ field for post, but it will require a migration script to update all existing posts. This script will handle the following tasks:
+- migrate all posts to set a new __owner__ field to its __author__.


Adding a new field to proto is fully backwards compatible

RiccardoM · 2023-06-12T16:39:28Z

@dadamu Can you rebase this on the master branch please?

dadamu · 2023-06-13T13:59:02Z

@RiccardoM Rebased

RiccardoM · 2023-06-13T17:17:07Z

proto/desmos/posts/v3/msgs.proto

+//
+// Since: Desmos 6.0.0
+message MsgAcceptPostOwnerTransfer {


I think it's better to name them Msg[Accept/Cancel/Refuse]PostOwnerTransferRequest (adding the missing Request at the end), similar to the Msg[Accept/Cancel/Refuse]DTagTransferRequest we have

RiccardoM · 2023-06-13T17:22:07Z

x/posts/keeper/msg_server_post_owner_transfer.go

+	// Check if the sender has profile
+	if !k.HasProfile(ctx, msg.Sender) {
+		return nil, errors.Wrapf(sdkerrors.ErrInvalidRequest, "you cannot cancel a post owner transfer request without having a profile")
+	}


Is this wanted? If the request was created, it means that the user had the profile at that time. I think we should allow them to cancel the request even if they have deleted them in the meanwhile

Yeah, we should allow all the users to cancel/refuse requests. Updated.

RiccardoM · 2023-06-13T17:22:50Z

x/posts/keeper/msg_server_post_owner_transfer.go

+	// Check if the receiver has profile
+	if !k.HasProfile(ctx, msg.Receiver) {
+		return nil, errors.Wrapf(sdkerrors.ErrInvalidRequest, "you cannot refuse a post owner transfer request without having a profile")
+	}
+


Same as above: we should allow refusing a transfer request even without a profile

RiccardoM · 2023-06-13T17:27:47Z

x/posts/legacy/v7/store.go

+
+	// Set the owner to its author for posts
+	for _, post := range posts {
+		newPost := types.NewOwnerTransfer(post.Author, post.LastEditedDate).Update(post)


I don't think we should use the Update method here. Instead, we should simply update the post's Owner field manually. This is to avoid having to call a method for nothing and thus reducing the overall migration (which can be quite lengthy)

Agree with it, updated

x/posts/keeper/msg_server_post_owner_trasnfer_test.go

RiccardoM

Great job 💯

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

dadamu force-pushed the paul/DCD-320/implement-adr-20 branch from d4809e7 to bdf4c9e Compare May 16, 2023 11:56

github-actions bot added the kind/adr An issue or PR relating to an architectural decision record label May 16, 2023

dadamu commented May 16, 2023

View reviewed changes

dadamu force-pushed the paul/DCD-320/implement-adr-20 branch 3 times, most recently from f2b963c to 0449ce5 Compare May 17, 2023 14:10

dadamu marked this pull request as ready for review May 18, 2023 11:36

dadamu requested a review from a team as a code owner May 18, 2023 11:36

dadamu changed the title ~~feat!: implement ADR-20: Post ownership transfer~~ feat: implement ADR-20: Post ownership transfer May 18, 2023

dadamu commented May 18, 2023

View reviewed changes

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

dadamu force-pushed the paul/DCD-320/implement-adr-20 branch from 6c84b32 to efb2efd Compare June 5, 2023 08:58

dadamu and others added 14 commits June 13, 2023 20:03

feat: add owner field to post and change author check to post

699ee08

feat: add msg types

cb459ee

feat: add post owner transfer request type

2ea9688

docs: modify ADR 20 store part

766c98d

docs: update Swagger definition

5509fbc

feat: implement post owner transfer keeper

6a642fa

feat: add GetPostOwnerTransferRequest method

5de6da7

feat: implement post owner transfer msg server

369ad99

revert: PostUpdate owenr field

469f86d

feat: add grpc query

be73a8d

docs: update Swagger definition

e19a7c7

feat: setup genesis

7eb3495

feat: setup invariant

0e77d8e

feat: setup cli

136ef47

dadamu added 8 commits June 13, 2023 20:15

feat: setup simulation tests

45d9f02

feat: setup migrations

bea6a79

chore: add changeset

879c063

docs: modify ADR

ec2d6db

chore: fix errors and lints

085d98e

tests: fix unit tests

63058a8

chore: fix import name of subspacestypes

82ea25d

chore: add since version to proto

7d64bad

dadamu force-pushed the paul/DCD-320/implement-adr-20 branch from efb2efd to 7d64bad Compare June 13, 2023 12:18

dadamu added 3 commits June 13, 2023 20:43

feat: add owner transfer update struct and update the post move tests

681830c

test: fix cli tests

fef1452

test: fix move post simulation test

eded554

RiccardoM reviewed Jun 13, 2023

View reviewed changes

dadamu added 5 commits June 14, 2023 18:12

fix: add missing request suffix for msgs

5aa8884

fix: remove profile check on cancel/refuse operation

bdcf28f

chore: improve post owner migration by not using update instance

806ba0e

test: update operation tests

0a275a3

chore: fix lint

ff940ce

RiccardoM reviewed Jun 14, 2023

View reviewed changes

RiccardoM added 6 commits June 14, 2023 10:57

Update x/posts/keeper/msg_server_post_owner_trasnfer_test.go

cdf65d1

Update x/posts/keeper/msg_server_post_owner_trasnfer_test.go

2c9e6e2

Update x/posts/keeper/msg_server_post_owner_trasnfer_test.go

0459df6

Update x/posts/keeper/msg_server_post_owner_trasnfer_test.go

f8456a8

Update x/posts/keeper/msg_server_post_owner_trasnfer_test.go

23a8502

Update x/posts/keeper/msg_server_post_owner_trasnfer_test.go

8c4cccc

RiccardoM approved these changes Jun 14, 2023

View reviewed changes

RiccardoM merged commit f0b03d0 into master Jun 14, 2023
34 checks passed

RiccardoM deleted the paul/DCD-320/implement-adr-20 branch June 14, 2023 18:53

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat: implement ADR-20: Post ownership transfer #1145

feat: implement ADR-20: Post ownership transfer #1145

dadamu commented May 16, 2023 •

edited by jira bot

codecov bot commented May 16, 2023 •

edited

dadamu May 16, 2023

dadamu May 18, 2023

dadamu May 18, 2023

RiccardoM commented Jun 12, 2023

dadamu commented Jun 13, 2023

RiccardoM Jun 13, 2023 •

edited

dadamu Jun 14, 2023

RiccardoM Jun 13, 2023

dadamu Jun 14, 2023

RiccardoM Jun 13, 2023

RiccardoM Jun 13, 2023

dadamu Jun 14, 2023

RiccardoM left a comment

		The solution outlined above is fully backwards compatible since it introduces a new __owner__ field for post, but it will require a migration script to update all existing posts. This script will handle the following tasks:
		- migrate all posts to set a new __owner__ field to its __author__.

feat: implement ADR-20: Post ownership transfer #1145

feat: implement ADR-20: Post ownership transfer #1145

Conversation

dadamu commented May 16, 2023 • edited by jira bot

Description

Author Checklist

Reviewers Checklist

codecov bot commented May 16, 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

RiccardoM commented Jun 12, 2023

dadamu commented Jun 13, 2023

RiccardoM Jun 13, 2023 • edited

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

dadamu commented May 16, 2023 •

edited by jira bot

codecov bot commented May 16, 2023 •

edited

RiccardoM Jun 13, 2023 •

edited