Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Refuse and Cancel features for DTag #305

Merged
merged 21 commits into from
Oct 21, 2020

Conversation

leobragaz
Copy link
Contributor

@leobragaz leobragaz commented Oct 16, 2020

Description

This PR implement the possibility from both the sender and receiver of a DTag request to cancel or refuse it.
Closes #297 .

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.

@leobragaz 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 Oct 16, 2020
@leobragaz leobragaz self-assigned this Oct 16, 2020
Copy link
Contributor Author

@leobragaz leobragaz left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

re-reviewed

Copy link
Contributor

@RiccardoM RiccardoM left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looking at the code and the documentation of the different DTag transfer-related message, I think that the current naming can create some confusion.

Currently there are two names: current_owner and sender, but I think that instead we should use sender and receiver.

So, I propose to change the different messages as:

type ... struct {
	Receiver sdk.AccAddress `json:"receiver" yaml:"receiver"`
	Sender  sdk.AccAddress `json:"sender" yaml:"sender"`
}

Also, I think we should rename the event attributes to request_sender and request_receiver for better understanding.

This would create a naming convention that makes it easier to understand what the different fields represent. What do you think?

@@ -707,8 +707,8 @@ func TestDesmosCLIPostsReactions(t *testing.T) {
subspace := "4e188d9c17150037d5199bbdb91ae1eb2a78a15aca04cb35530cccb81494b36e"
reactions := types.Reactions{
types.Reaction{":earth:", "http://earth.jpg", subspace, fooAddr},
types.Reaction{":blush:", "https://gph.is/2p19Zai", subspace, fooAddr},
types.Reaction{":thumbsdown:", "https://gph.is/2phybnt", subspace, fooAddr},
types.Reaction{":plot:", "https://gph.is/2p19Zai", subspace, fooAddr},
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can we revert all the changes that were made to this file?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The changes I made were to fix some errors inside tests to exclude them from the possible causes that make the whole suite fail, should I revert them anyway?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@bragaz Yes please, they were part of tests that have always been passing so I don't think they consitute any problem

x/posts/keeper/invariants_test.go Outdated Show resolved Hide resolved
x/posts/types/models/post_response_test.go Outdated Show resolved Hide resolved
x/profiles/types/msgs/msgs.go Outdated Show resolved Hide resolved
x/profiles/types/msgs/msgs.go Outdated Show resolved Hide resolved
x/profiles/types/msgs/msgs.go Outdated Show resolved Hide resolved
x/reports/types/models/report.go Outdated Show resolved Hide resolved
x/reports/types/models/report_response_test.go Outdated Show resolved Hide resolved
Comment on lines 17 to 20
r.HandleFunc("/profiles/dtag/request/{receiverAddress}", requestDTagTransferHandler(cliCtx)).Methods("POST")
r.HandleFunc("/profiles/dtag/accept/{senderAddress}", acceptTransferRequestHandler(cliCtx)).Methods("POST")
r.HandleFunc("/profiles/dtag/refuse/{senderAddress}", refuseDTagTransferRequestHandler(cliCtx)).Methods("POST")
r.HandleFunc("/profiles/dtag/cancel/{receiverAddress}", cancelDTagTransferRequestHandler(cliCtx)).Methods("POST")
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think that these can be further improved by writing them as

Suggested change
r.HandleFunc("/profiles/dtag/request/{receiverAddress}", requestDTagTransferHandler(cliCtx)).Methods("POST")
r.HandleFunc("/profiles/dtag/accept/{senderAddress}", acceptTransferRequestHandler(cliCtx)).Methods("POST")
r.HandleFunc("/profiles/dtag/refuse/{senderAddress}", refuseDTagTransferRequestHandler(cliCtx)).Methods("POST")
r.HandleFunc("/profiles/dtag/cancel/{receiverAddress}", cancelDTagTransferRequestHandler(cliCtx)).Methods("POST")
r.HandleFunc("/profiles/dtagrequests", requestDTagTransferHandler(cliCtx)).Methods("POST")
r.HandleFunc("/profiles/dtagrequests/{senderAddress}/acceptances", acceptTransferRequestHandler(cliCtx)).Methods("POST")
r.HandleFunc("/profiles/dtagrequests/{senderAddress}", refuseDTagTransferRequestHandler(cliCtx)).Methods("DELETE")
r.HandleFunc("/profiles/dtagrequests/{receiverAddress}", cancelDTagTransferRequestHandler(cliCtx)).Methods("DELETE")

Copy link
Contributor

@RiccardoM RiccardoM left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🚀

@RiccardoM RiccardoM merged commit 681001c into master Oct 21, 2020
@RiccardoM RiccardoM deleted the leonardo/dtag-request-reject-cancel branch October 21, 2020 07:08
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
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
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Reject DTag transfers
2 participants