Leonardo/users relationship impl #260

leobragaz · 2020-08-17T08:06:02Z

Description

This PR implements the users' relationships.
Close #168 .

Checklist

Targeted PR against correct branch.
Linked to Github issue with discussion and accepted design OR link to spec that describes this work.
Wrote unit tests.
Wrote integration tests (simulation & CLI).
Updated the documentation.
Added an entry to the CHANGELOG.md file.
Re-reviewed Files changed in the Github PR explorer.

need to review the way of saving/getting relationships

added relationship id fixed keeper and handler methods

fixed lint errors

added relationshipID tests fixed relationship tests

codecov · 2020-08-17T08:07:59Z

Codecov Report

Merging #260 into master will increase coverage by 0.56%.
The diff coverage is 92.47%.

@@            Coverage Diff             @@
##           master     #260      +/-   ##
==========================================
+ Coverage   85.85%   86.42%   +0.56%     
==========================================
  Files          82       95      +13     
  Lines        2970     3174     +204     
==========================================
+ Hits         2550     2743     +193     
- Misses        363      373      +10     
- Partials       57       58       +1

Impacted Files	Coverage Δ
x/posts/simulation/decoder.go	`94.11% <0.00%> (ø)`
x/profiles/keeper/querier.go	`71.79% <0.00%> (ø)`
x/profiles/simulation/decoder.go	`85.71% <0.00%> (ø)`
x/profiles/types/models/keys.go	`0.00% <ø> (ø)`
x/profiles/types/models/pictures.go	`89.47% <ø> (ø)`
x/profiles/types/models/profile.go	`100.00% <ø> (ø)`
x/relationships/types/models/keys.go	`0.00% <0.00%> (ø)`
x/profiles/keeper/handler.go	`93.58% <50.00%> (ø)`
x/relationships/keeper/querier.go	`75.00% <75.00%> (ø)`
x/relationships/simulation/decoder.go	`77.77% <77.77%> (ø)`
... and 29 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 56205de...a720170. Read the comment docs.

added querier tests added cli tx and queries added rest tx and queries added helper functions for cli testing started CLI tests

added new relationships object to genesis

added simulation genesis tests fixed lint errors

…eonardo/users-relationship-impl � Conflicts: � x/posts/keeper/handler_test.go � x/posts/keeper/keeper_test.go � x/posts/types/alias.go � x/posts/types/models/post_response_test.go � x/posts/types/models/post_test.go � x/reports/types/alias.go

fixed some refactoring errors inside posts

improved code

…eonardo/users-relationship-impl � Conflicts: � CHANGELOG.md

removed keeper blank lines merged with master

excluded sim tests from coverage

fixed sim tests fixed cli tests

leobragaz · 2020-08-25T16:57:57Z

@RiccardoM I've made all the changes we discussed during the call.
I've not updated the docs yet, I'll wait for the approval of the code than I'll do it.

RiccardoM

I think you made some refactoring errors. Namely:

All ID references inside strings have been changed to RelationshipID
All the profilesTypes "github.com/desmos-labs/desmos/x/profiles/types" imports have been changed to "github.com/desmos-labs/desmos/x/profiles/types/models"

Can you please revert that so that I have to view less files? Otherwise it would be complicated for me to see actual changes in the code

RiccardoM

The implementation looks good. I left some comments. Also, I would like to discuss the possibility of moving the whole relationship implementation inside their own module instead of leaving it inside the x/profile one. What do you think?

RiccardoM · 2020-08-26T10:37:44Z

x/profiles/client/rest/query.go

@@ -15,6 +15,7 @@ func registerQueryRoutes(cliCtx context.CLIContext, r *mux.Router) {
 	r.HandleFunc("/profiles/parameters", queryProfilesParamsHandlerFn(cliCtx)).Methods("GET")
 	r.HandleFunc("/profiles/{address_or_dtag}", queryProfileHandlerFn(cliCtx)).Methods("GET")
 	r.HandleFunc("/profiles", queryProfilesHandlerFn(cliCtx)).Methods("GET")
+	r.HandleFunc("/relationships/{address}", queryUserRelationships(cliCtx)).Methods("GET")


What about adding a new endpoint to query all the relationship for all the profiles?

Yeah it could be good, Also ok for moving relationships on a new module

RiccardoM · 2020-08-26T10:38:11Z

x/profiles/client/cli/tx.go

@@ -30,6 +31,8 @@ func GetTxCmd(_ string, cdc *codec.Codec) *cobra.Command {
 	profileTxCmd.AddCommand(flags.PostCommands(
 		GetCmdSaveProfile(cdc),
 		GetCmdDeleteProfile(cdc),
+		GetCmdCreateMonoDirectionalRelationship(cdc),
+		GetCmdDeleteUserRelationship(cdc),


What about adding a new command to query all the relationship for all the profiles?

RiccardoM · 2020-08-26T10:40:38Z

x/profiles/keeper/keeper_relationships.go

+}
+
+// GetUsersRelationshipsMap allows to returns the map of all the users and their associated storedRelationships
+func (k Keeper) GetUsersRelationshipsMap(ctx sdk.Context) map[string][]sdk.AccAddress {


I think we can rename this to be simply GetUsersRelationships

RiccardoM · 2020-08-26T10:42:15Z

x/profiles/keeper/keeper_relationships.go

+	for _, addr := range relationships {
+		if addr.Equals(receiver) {
+			return fmt.Errorf("relationship already exists with %s", receiver)
+		}
+	}


Instead of performing this check here, I think we can:

Extract these lines in a new method, maybe something like DoesRelationshipExist

Call the new method from the handler

Inside this method, if the relationship already exists then simply not add it to the array instead of throwing an error

I don't think extracting this is the better choice since we use it only here and, with a method like DoesRelationshipExist, we'll need to access two times to the store instead of one inside the msg Handler.

@bragaz Ok, I see. Then let's keep it that way

to a new module relationships

reviewed query and tx endpoints added docs

RiccardoM

Looks good to me. Can we rename all the MonoDirectionalRelationship referenced to be simply Relationship instead?

RiccardoM · 2020-08-28T07:47:45Z

x/relationships/client/cli/query.go

+// GetCmdQueryUserRelationships queries all the profiles' users' relationships
+func GetCmdQueryUserRelationships(cdc *codec.Codec) *cobra.Command {
+	return &cobra.Command{
+		Use:   "user_relationships [address]",


I think we can rename this to be simply user [address]

RiccardoM

LGTM. Some small fixed and tests additions and it should be ready to be merged 💯

RiccardoM · 2020-08-31T05:30:37Z

docs/developers/msgs/create-relationship.md

@@ -0,0 +1,38 @@
+# `MsgCreateRelationship`
+This message allows you to create a mono directional relationship with a specified user.


Suggested change

This message allows you to create a mono directional relationship with a specified user.

This message allows you to create a relationship between the signer and a specified user.

RiccardoM · 2020-08-31T05:30:45Z

docs/developers/msgs/create-relationship.md

@@ -0,0 +1,38 @@
+# `MsgCreateRelationship`
+This message allows you to create a mono directional relationship with a specified user.
+Mono directional relationships are like the follow of today's social networks.


Suggested change

Mono directional relationships are like the follow of today's social networks.

RiccardoM · 2020-08-31T05:31:28Z

docs/developers/msgs/delete-relationship.md

@@ -0,0 +1,37 @@
+# `MsgDeleteRelationship`
+This message allows you to delete a mono directional relationship with a specified counterparty.


Suggested change

This message allows you to delete a mono directional relationship with a specified counterparty.

This message allows you to delete a relationship existing with a specified counterparty.

RiccardoM · 2020-08-31T05:32:14Z

docs/developers/perform-transactions.md

 * [`MsgSaveProfile`](msgs/save-profile.md): allows you to create or edit an existing profile.
 * [`MsgDeleteProfile`](msgs/delete-profile.md): allows you to delete an existing profile.
 * [`EditParamsProposal`](msgs/edit_param_proposal.md): allows you to open a proposal to change profile's params.

+## Relationships
+* [`MsgCreateMonoDirectionalRelationship`](msgs/create-relationship.md): allows you to create a mono directional relationship.


Suggested change

* [`MsgCreateMonoDirectionalRelationship`](msgs/create-relationship.md): allows you to create a mono directional relationship.

* [`MsgCreateRelationship`](msgs/create-relationship.md): allows you to create a relationship.

RiccardoM · 2020-08-31T05:32:56Z

docs/developers/queries/user_relationships.md

@@ -0,0 +1,18 @@
+## Query user relationships
+This query endpoint allows you to retrieve the details of a relationship where the creator is the given `address`.


Suggested change

This query endpoint allows you to retrieve the details of a relationship where the creator is the given `address`.

This query endpoint allows you to retrieve the details of a relationship where the creator has the given `address`.

RiccardoM · 2020-08-31T05:35:18Z

x/relationships/client/cli/tx.go

+}
+
+// GetCmdDeleteUserRelationship is the CLI command for deleting a relationship
+func GetCmdDeleteUserRelationship(cdc *codec.Codec) *cobra.Command {


I think we can rename this GetCmdDeleteRelationship for simplicity

RiccardoM · 2020-08-31T05:36:37Z

x/relationships/client/rest/tx.go

+	r.HandleFunc("/relationships/create/{address}", createRelationshipHandler(cliCtx)).Methods("POST")
+	r.HandleFunc("/relationships/delete/{address}", deleteRelationshipHandler(cliCtx)).Methods("DELETE")


I think we can delete the /create/ and /delete/ parts of the query, and maybe the /{address} as well, requiring the user to put it into the body of the request instead. What do you think?

Suggested change

r.HandleFunc("/relationships/create/{address}", createRelationshipHandler(cliCtx)).Methods("POST")

r.HandleFunc("/relationships/delete/{address}", deleteRelationshipHandler(cliCtx)).Methods("DELETE")

r.HandleFunc("/relationships", createRelationshipHandler(cliCtx)).Methods("POST")

r.HandleFunc("/relationships", deleteRelationshipHandler(cliCtx)).Methods("DELETE")

I think there will not be much difference. The only concern I have is if it's possibile to have two endpoints with the exact same name

@bragaz Yes, it is cause the REST method will be different. In the first one it will be POST, and the other one it will be DELETE. Removing the /create and /delete parts will make the endpoint follow the REST guidelines (no verbs inside endpoint names, only nouns). And removing the {address} variable will make it coherent with other requests as well

RiccardoM · 2020-08-31T05:39:30Z

x/relationships/keeper/keeper_test.go

+	}
+}
+
+func (suite *KeeperTestSuite) TestKeeper_GetUserRelationshipsMap() {


Can we add a test case where we try to read the relationships with an empty set?

RiccardoM · 2020-08-31T05:39:36Z

x/relationships/keeper/keeper_test.go

+	suite.Equal(relationshipsMap, actualIDsMap)
+}
+
+func (suite *KeeperTestSuite) TestKeeper_GetUserRelationships() {


Can we add a test case where we try to read the relationships with an empty slice?

RiccardoM · 2020-08-31T05:39:59Z

x/relationships/keeper/keeper_test.go

+			storedRelationships: []sdk.AccAddress{addr1},
+			expRelationships:    nil,
+			userToDelete:        addr1,
+		},


Can we add a test case where we try deleting a relationship with len(relationships) == 0?

added test cases edited tx endpoints fixed docs

leobragaz added 10 commits August 6, 2020 16:32

reviewed profiles folders organization

989e47c

added relationship types and methods + tests

321420f

added relationships messages and tests + keys

c468f20

added msg delete relationship + tests

813cce0

started developing keeper methods

1dc2d05

added keeper methods and handler

ff38990

need to review the way of saving/getting relationships

edited the way relations are handled inside keeper,

e44cba4

added relationship id fixed keeper and handler methods

completed handler's and keeper's relationships methods

22740ed

fixed lint errors

fixed msgs and keeper tests

3dff7d7

added handler tests

f20fef7

added relationshipID tests fixed relationship tests

leobragaz added kind/new-feature Propose the addition of a new feature that does not yet exist x/profiles Module that allows to create and manage decentralized social profiles labels Aug 17, 2020

leobragaz self-assigned this Aug 17, 2020

leobragaz added 10 commits August 17, 2020 17:03

added querier method

3aa5a79

added querier tests added cli tx and queries added rest tx and queries added helper functions for cli testing started CLI tests

added cli tests

b0f0ee5

added simulation tests for relationships msgs

d932f30

added methods for genesis

b925022

added new relationships object to genesis

added initialization of new data structures to init genesis

b6da0e0

added simulation genesis tests fixed lint errors

merged with master

493f2a6

fixed some refactoring errors inside posts

fixed sim tests

ac79300

raised gas to make sim tests pass

9f93900

added tests to raise coverage

c4923ca

improved code

leobragaz marked this pull request as ready for review August 24, 2020 08:07

leobragaz added 5 commits August 24, 2020 10:08

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

8236487

…eonardo/users-relationship-impl � Conflicts: � CHANGELOG.md

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

bcfe6dc

…eonardo/users-relationship-impl � Conflicts: � CHANGELOG.md

added docs lines to handler

8f745a4

removed keeper blank lines merged with master

added tests to raise coverage

851e748

excluded sim tests from coverage

fixed relationships as discussed in morning call

479d8dd

leobragaz added 3 commits August 25, 2020 11:29

fixed sims tests

fd9d43d

added relationships response

5735392

fixed sim tests fixed cli tests

raise coverage

536623e

leobragaz requested a review from RiccardoM August 25, 2020 16:58

RiccardoM reviewed Aug 26, 2020

View reviewed changes

leobragaz added 4 commits August 26, 2020 10:17

fixed refactoring errors

7d2ee3f

fixed refactoring errors

359bae7

fixed refactoring errors

91068ab

fixed refactoring errors

7f3a6bf

leobragaz requested a review from RiccardoM August 26, 2020 08:47

RiccardoM reviewed Aug 26, 2020

View reviewed changes

leobragaz added 4 commits August 26, 2020 17:41

moved all relationships related files

90c6679

to a new module relationships

added dontcover to sim files

3d50e7e

added new query for all relationships

31cd7cc

added a query to retrieve all the relationships

cc1366c

reviewed query and tx endpoints added docs

leobragaz requested a review from RiccardoM August 27, 2020 10:33

RiccardoM reviewed Aug 28, 2020

View reviewed changes

renamed query and message

ebc47f6

leobragaz requested a review from RiccardoM August 28, 2020 13:07

RiccardoM reviewed Aug 31, 2020

View reviewed changes

fixed PR's suggestions:

a720170

added test cases edited tx endpoints fixed docs

leobragaz requested a review from RiccardoM August 31, 2020 09:41

leobragaz mentioned this pull request Aug 31, 2020

Implement user blocking #169

Closed

RiccardoM approved these changes Aug 31, 2020

View reviewed changes

RiccardoM merged commit 454c3ab into master Aug 31, 2020

RiccardoM deleted the leonardo/users-relationship-impl branch August 31, 2020 11:05

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Leonardo/users relationship impl #260

Leonardo/users relationship impl #260

leobragaz commented Aug 17, 2020 •

edited

Loading

codecov bot commented Aug 17, 2020 •

edited

Loading

leobragaz commented Aug 25, 2020

RiccardoM left a comment

RiccardoM left a comment

RiccardoM Aug 26, 2020

leobragaz Aug 26, 2020

RiccardoM Aug 26, 2020

RiccardoM Aug 26, 2020

RiccardoM Aug 26, 2020

leobragaz Aug 27, 2020 •

edited

Loading

RiccardoM Aug 28, 2020

RiccardoM left a comment

RiccardoM Aug 28, 2020

RiccardoM left a comment

RiccardoM Aug 31, 2020

RiccardoM Aug 31, 2020

RiccardoM Aug 31, 2020

RiccardoM Aug 31, 2020

RiccardoM Aug 31, 2020

RiccardoM Aug 31, 2020

RiccardoM Aug 31, 2020

leobragaz Aug 31, 2020

RiccardoM Aug 31, 2020

RiccardoM Aug 31, 2020

RiccardoM Aug 31, 2020

RiccardoM Aug 31, 2020

		@@ -0,0 +1,38 @@
		# `MsgCreateRelationship`
		This message allows you to create a mono directional relationship with a specified user.

	This message allows you to create a mono directional relationship with a specified user.
	This message allows you to create a relationship between the signer and a specified user.

		@@ -0,0 +1,37 @@
		# `MsgDeleteRelationship`
		This message allows you to delete a mono directional relationship with a specified counterparty.

	This message allows you to delete a mono directional relationship with a specified counterparty.
	This message allows you to delete a relationship existing with a specified counterparty.

	* [`MsgCreateMonoDirectionalRelationship`](msgs/create-relationship.md): allows you to create a mono directional relationship.
	* [`MsgCreateRelationship`](msgs/create-relationship.md): allows you to create a relationship.

		@@ -0,0 +1,18 @@
		## Query user relationships
		This query endpoint allows you to retrieve the details of a relationship where the creator is the given `address`.

		r.HandleFunc("/relationships/create/{address}", createRelationshipHandler(cliCtx)).Methods("POST")
		r.HandleFunc("/relationships/delete/{address}", deleteRelationshipHandler(cliCtx)).Methods("DELETE")

Leonardo/users relationship impl #260

Leonardo/users relationship impl #260

Conversation

leobragaz commented Aug 17, 2020 • edited Loading

Description

Checklist

codecov bot commented Aug 17, 2020 • edited Loading

Codecov Report

leobragaz commented Aug 25, 2020

RiccardoM left a 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

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

leobragaz Aug 27, 2020 • edited Loading

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

RiccardoM left a 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

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

leobragaz commented Aug 17, 2020 •

edited

Loading

codecov bot commented Aug 17, 2020 •

edited

Loading

leobragaz Aug 27, 2020 •

edited

Loading