Implement new DeleteName Message Type by aayushijain23 · Pull Request #92 · cosmos/sdk-tutorials

aayushijain23 · 2019-06-03T22:23:16Z

Added a new message called DeleteName to this tutorial. The following changes have been made:

Added a new message type for DeleteName
Updated the keeper with a new method for deleting the Whois struct for a particular name.
Updated the Handler with what actions to be taken for a DeleteName message.
Registered the DeleteName interface in Codec.
Added a cli command for the DeleteName transaction and added corresponding changes in the module client.
Finally added a REST endpoint for the same.

- Added a new message type for DeleteName. - Added a method for deleting the Whois struct for a particular name in the keeper. - Added a corresponding handler for DeleteName messages. - Registered the DeleteName message interface in codec. - Added a cli command for DeleteName transaction and made necessary changes in the module client. - Added a rest endpoint for the same.

…to aayushijain23/add-new-msg-type-delete

…ew-msg-type-delete

- Tested the code by running the cli commands - Updated the documentation for the new message deletename

…ew-msg-type-delete - After Marko's merge, some of the code got refactored. Hence I made changes accordingly in the code for DeleteName. - While testing the code via REST APIs, I also realized that the Delete endpoint need not have the restName. Hence I removed that too.

…ew-msg-type-delete

…s of how to delete a name.

…ew-msg-type-delete

alexanderbez

Thanks @aayushijain23 -- your first contribution 🎉

Left some general feedback, but overall looks good!

alexanderbez · 2019-06-19T00:56:43Z

+				return err
+			}
+
+			cliCtx.PrintResponse = true


Why do we need this?

I thought this prints the response in the cli, when we test the code via cli commands. Will remove it.

This still needs to be removed.

alexanderbez · 2019-06-19T01:24:00Z

 }

+// Deletes the entire Whois metadata struct for a name
+func (k Keeper) DeleteWhois(ctx sdk.Context, name string) {


How is this different that DeleteName? Why not just have a single method DeleteWhois or DeleteName. We should also return an error if the name doesn't exist.

You're right. There isn't any difference between the two methods. Actually, Getter and Setter methods were defined in the form of SetWhoIs and GetWhoIs. And then additional methods were added for getting specific parameters from the store based on the name, such as: SetOwner(), GetOwner() etc. I thought of maintaining consistency by following the same structure. But yeah in the case of deleting the name, there aren't any additional methods that can be specified. Will keep DeleteWhoIs. And yeah I'll return an error if the name doesn't exist.

Co-Authored-By: Alexander Bezobchuk <alexanderbez@users.noreply.github.com>

The comment for explaining the deletion operation is not needed. Co-Authored-By: Alexander Bezobchuk <alexanderbez@users.noreply.github.com>

Co-Authored-By: Alexander Bezobchuk <alexanderbez@users.noreply.github.com>

- Added an errors.go file to handle errors custom to the module.

…ew-msg-type-delete

…ted does not exist in the store.

…ew-msg-type-delete

alexanderbez

ACK, just a few nitpicks on comments

alexanderbez · 2019-06-21T17:07:17Z

 func handleMsgDeleteName(ctx sdk.Context, keeper Keeper, msg MsgDeleteName) sdk.Result {
-	if !msg.Owner.Equals(keeper.GetOwner(ctx, msg.Name)) { // Checks if the the msg sender is the same as the current owner
-		return sdk.ErrUnauthorized("Incorrect Owner").Result() // If not, throw an error
+	// Checks if the name is present in the store or not


This comment is redundant imho

alexanderbez · 2019-06-21T17:07:24Z

 	}
-	keeper.DeleteWhois(ctx, msg.Name) // If so, delete the name specified in the msg.
-	return sdk.Result{}              // return
+	// Checks if the the msg sender is the same as the current owner


alexanderbez · 2019-06-21T17:07:35Z

+	if !msg.Owner.Equals(keeper.GetOwner(ctx, msg.Name)) { 
+		return sdk.ErrUnauthorized("Incorrect Owner").Result() 
+	}
+	// If so, delete the name specified in the msg.


alexanderbez · 2019-06-21T17:08:16Z

 // Handle a message to delete name
 func handleMsgDeleteName(ctx sdk.Context, keeper Keeper, msg MsgDeleteName) sdk.Result {
-    // Checks if the the msg sender is the same as the current owner
+	// Checks if the name is present in the store or not


alexanderbez · 2019-06-21T17:08:22Z

+	if !keeper.IsNamePresent(ctx, msg.Name) {
+		return types.ErrNameDoesNotExist(types.DefaultCodespace).Result()
+	}
+    // Checks if the msg sender is the same as the current owner


Cool, will remove the unnecessary comments.

alexanderbez · 2019-06-21T17:54:39Z

+				return err
+			}
+
+			cliCtx.PrintResponse = true


This still needs to be removed.

alexanderbez · 2019-06-21T17:55:21Z

+```go
+// MsgDeleteName defines a DeleteName message
+type MsgDeleteName struct {
+	Name  string		 `json:"name"`


Formatting is off.

Didn't quite understand this one. I followed the same format as buy-name.md and set-name.md. Everywhere I have noticed that the json names are structured in a column, one after the other. Hence the code looks as follows:

type MsgDeleteName struct { Name string `json:"name"` Owner sdk.AccAddress `json:"owner"` }

Since sdk.AccAddress as a term takes more space than string, the term json:"name" is pushed more towards the right to align with json:"owner"

The formatting of Go is not correct. See: https://github.com/cosmos/sdk-application-tutorial/blob/88709734312254089c3c1e3ff5935b436fce4cba/tutorial/delete-name.md

The field tags need to be aligned. You can just copy-paste this from the actual gocode.

Oh I thought you meant the alignment of the field tags in the code is not right. Yeah I couldn't see the preview before pushing the code. I'll copy it from there.

On a side note, found the preview option in vscode.

…ew-msg-type-delete

…ew-msg-type-delete Resolved merge conflicts in tutorial/cli.md

aayushijain23 added 2 commits June 3, 2019 16:07

Merge remote-tracking branch 'origin/hans/generalization-refactor' in…

89600ed

…to aayushijain23/add-new-msg-type-delete

aayushijain23 requested review from alexanderbez, jackzampolin and okwme June 3, 2019 22:25

alexanderbez changed the base branch from hans/generalization-refactor to master June 4, 2019 17:49

aayushijain23 and others added 7 commits June 6, 2019 16:13

Merge remote-tracking branch 'origin/master' into aayushijain23/add-n…

0b33a33

…ew-msg-type-delete

merge fix for missing staking keeper errors

694d91f

Merge remote-tracking branch 'origin/master' into aayushijain23/add-n…

4b52429

…ew-msg-type-delete

Tested the cli commands and updated the documentation

ab9d4de

- Tested the code by running the cli commands - Updated the documentation for the new message deletename

Merge remote-tracking branch 'origin/master' into aayushijain23/add-n…

b60c2b8

…ew-msg-type-delete

Merge remote-tracking branch 'origin/master' into aayushijain23/add-n…

6612995

…ew-msg-type-delete

alexanderbez changed the title ~~Added a new message type - DeleteName~~ Implement new DeleteName Message Type Jun 18, 2019

aayushijain23 added 2 commits June 18, 2019 18:47

Updated the tutorial, mainly build-run.md and run-rest.md with detail…

df08540

…s of how to delete a name.

Merge remote-tracking branch 'origin/master' into aayushijain23/add-n…

da507ad

…ew-msg-type-delete

aayushijain23 marked this pull request as ready for review June 18, 2019 22:52

alexanderbez suggested changes Jun 19, 2019

View reviewed changes

aayushijain23 and others added 9 commits June 19, 2019 13:56

Update x/nameservice/handler.go

ce7a0df

Co-Authored-By: Alexander Bezobchuk <alexanderbez@users.noreply.github.com>

Update x/nameservice/handler.go

220409f

The comment for explaining the deletion operation is not needed. Co-Authored-By: Alexander Bezobchuk <alexanderbez@users.noreply.github.com>

Update x/nameservice/handler.go

3060898

Co-Authored-By: Alexander Bezobchuk <alexanderbez@users.noreply.github.com>

Update x/nameservice/handler.go

e0c6b7f

Co-Authored-By: Alexander Bezobchuk <alexanderbez@users.noreply.github.com>

Update x/nameservice/client/rest/rest.go

5f3f5bf

Co-Authored-By: Alexander Bezobchuk <alexanderbez@users.noreply.github.com>

Update tutorial/cli.md

86516d1

Co-Authored-By: Alexander Bezobchuk <alexanderbez@users.noreply.github.com>

Update tutorial/cli.md

9bb5e44

Co-Authored-By: Alexander Bezobchuk <alexanderbez@users.noreply.github.com>

Incorporated review comments.

1a27802

- Added an errors.go file to handle errors custom to the module.

Merge remote-tracking branch 'origin/master' into aayushijain23/add-n…

bc782ea

…ew-msg-type-delete

aayushijain23 requested a review from alexanderbez June 20, 2019 16:09

aayushijain23 added 2 commits June 20, 2019 15:21

Incorporated error handling in the scenario where the name to be dele…

d7ce723

…ted does not exist in the store.

Merge remote-tracking branch 'origin/master' into aayushijain23/add-n…

627189e

…ew-msg-type-delete

Merge remote-tracking branch 'origin/master' into aayushijain23/add-n…

b8c6562

…ew-msg-type-delete

alexanderbez reviewed Jun 21, 2019

View reviewed changes

Removed unnecessary comments.

8870973

aayushijain23 requested a review from alexanderbez June 21, 2019 17:23

alexanderbez suggested changes Jun 21, 2019

View reviewed changes

aayushijain23 added 2 commits June 21, 2019 14:35

Incorporated review comments.

f679763

Resolved a minor formatting issue.

673d228

aayushijain23 requested a review from alexanderbez June 21, 2019 18:46

aayushijain23 added 2 commits June 24, 2019 12:42

Merge remote-tracking branch 'origin/master' into aayushijain23/add-n…

50341f8

…ew-msg-type-delete

Merge remote-tracking branch 'origin/master' into aayushijain23/add-n…

d39bb23

…ew-msg-type-delete

hschoenburg approved these changes Aug 2, 2019

View reviewed changes

Merge remote-tracking branch 'origin/master' into aayushijain23/add-n…

a702f54

…ew-msg-type-delete Resolved merge conflicts in tutorial/cli.md

hschoenburg merged commit d9927fe into master Aug 2, 2019

tac0turtle deleted the aayushijain23/add-new-msg-type-delete branch May 25, 2020 13:15

Conversation

aayushijain23 commented Jun 3, 2019

Uh oh!

alexanderbez left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

aayushijain23 Jun 19, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

alexanderbez left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

aayushijain23 Jun 21, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

aayushijain23 Jun 19, 2019 •

edited

Loading

aayushijain23 Jun 21, 2019 •

edited

Loading