From 955f56eebda8f9954c552a48e77f638642ddc90f Mon Sep 17 00:00:00 2001 From: Bax995 Date: Thu, 7 Oct 2021 18:25:00 +0200 Subject: [PATCH 1/7] Wrote ADR 009 --- .../adr-009-avoid-accidental-dtag-override.md | 190 ++++++++++++++++++ 1 file changed, 190 insertions(+) create mode 100644 docs/architecture/adr-009-avoid-accidental-dtag-override.md diff --git a/docs/architecture/adr-009-avoid-accidental-dtag-override.md b/docs/architecture/adr-009-avoid-accidental-dtag-override.md new file mode 100644 index 0000000000..65bca9177c --- /dev/null +++ b/docs/architecture/adr-009-avoid-accidental-dtag-override.md @@ -0,0 +1,190 @@ +# ADR 009: Avoid accidental DTag override + +## Changelog + +- October 7th, 2021: Proposed + +## Status + +PROPOSED + +## Abstract + +We SHOULD edit the behaviour of the current `MsgSaveProfile` making the `DTag` an optional flag +in order to prevent users from accidentally overrides their own DTags. In this way, users will edit +their DTags only when they specify them with the flag. + + +## Context + +Currently, the `MsgSaveProfile` CLI command always requires to specify a DTag when using it. This means that +users that do not want to edit their DTag need to specify it anyway. This could lead to the situation where a user +accidentally make a typo while inserting the DTag triggering its edit. If this happens, and the user doesn't notice it +immediately, another user can register a profile with his DTag that is now free. + +## Decision + +To avoid the situation described above, we need to perform some changes on how the `MsgSaveProfile` handles the DTag. +First, we need to make the `dtag` required field an optional flag by editing the command as follows: +```go +func GetCmdSaveProfile() *cobra.Command { + cmd := &cobra.Command{ + Use: "save", + Args: cobra.NoArgs, + Short: "Save your profile", + Long: fmt.Sprintf(` +Save a new profile or edit the existing one specifying a DTag, a nickname, biography, profile picture and cover picture. +Every data given through the flags is optional. +If you are editing an existing profile you should fill only the fields that you want to edit. +The empty ones will be filled with a special [do-not-modify] flag that tells the system to not edit them. + +%s tx profiles save + --%s LeoDiCap \ + --%s "Leonardo Di Caprio" \ + --%s "Hollywood actor. Proud environmentalist" \ + --%s "https://profilePic.jpg" \ + --%s "https://profileCover.jpg" +`, version.AppName, FlagDTag, FlagNickname, FlagBio, FlagProfilePic, FlagCoverPic), + RunE: func(cmd *cobra.Command, args []string) error { + clientCtx, err := client.GetClientTxContext(cmd) + if err != nil { + return err + } + + dTag, _ := cmd.Flags().GetString(FlagDTag) + nickname, _ := cmd.Flags().GetString(FlagNickname) + bio, _ := cmd.Flags().GetString(FlagBio) + profilePic, _ := cmd.Flags().GetString(FlagProfilePic) + coverPic, _ := cmd.Flags().GetString(FlagCoverPic) + + msg := types.NewMsgSaveProfile(dTag, nickname, bio, profilePic, coverPic, clientCtx.FromAddress.String()) + if err = msg.ValidateBasic(); err != nil { + return fmt.Errorf("message validation failed: %w", err) + } + + return tx.GenerateOrBroadcastTxCLI(clientCtx, cmd.Flags(), msg) + }, + } + + cmd.Flags().String(FlagDTag, types.DoNotModify, "DTag to be used") + cmd.Flags().String(FlagNickname, types.DoNotModify, "Nickname to be used") + cmd.Flags().String(FlagBio, types.DoNotModify, "Biography to be used") + cmd.Flags().String(FlagProfilePic, types.DoNotModify, "Profile picture") + cmd.Flags().String(FlagCoverPic, types.DoNotModify, "Cover picture") + + flags.AddTxFlagsToCmd(cmd) + + return cmd +} +``` +Second, we need to remove the check on DTag inside `ValidateBasic()` method of `MsgSaveProfile` that will result in: +```go +func (msg MsgSaveProfile) ValidateBasic() error { + _, err := sdk.AccAddressFromBech32(msg.Creator) + if err != nil { + return sdkerrors.Wrap(sdkerrors.ErrInvalidAddress, fmt.Sprintf("invalid creator: %s", msg.Creator)) + } + return nil +} +``` + +Third, we will edit the behaviour of `msgServer` `SaveProfile` method: +```go +func (k msgServer) SaveProfile(goCtx context.Context, msg *types.MsgSaveProfile) (*types.MsgSaveProfileResponse, error) { + ctx := sdk.UnwrapSDKContext(goCtx) + + profile, found, err := k.GetProfile(ctx, msg.Creator) + if err != nil { + return nil, err + } + + // Create a new profile if not found + if !found { + + if msg.DTag == types.DoNotModify { + return nil, sdkerrors.Wrapf(sdkerrors.ErrInvalidRequest, "DTag need to be specified if user doesn't have a profile") + } + + addr, err := sdk.AccAddressFromBech32(msg.Creator) + if err != nil { + return nil, err + } + + profile, err = types.NewProfileFromAccount(msg.DTag, k.ak.GetAccount(ctx, addr), ctx.BlockTime()) + if err != nil { + return nil, err + } + } + + var updated *types.Profile + if msg.DTag == types.DoNotModify { + // Update the existing profile with the values provided from the user + updated, err = profile.Update(types.NewProfileUpdate( + profile.DTag, + msg.Nickname, + msg.Bio, + types.NewPictures(msg.ProfilePicture, msg.CoverPicture), + )) + } else { + // Update the existing profile with the values provided from the user + updated, err = profile.Update(types.NewProfileUpdate( + msg.DTag, + msg.Nickname, + msg.Bio, + types.NewPictures(msg.ProfilePicture, msg.CoverPicture), + )) + } + + if err != nil { + return nil, sdkerrors.Wrap(sdkerrors.ErrInvalidRequest, err.Error()) + } + + // Validate the profile + err = k.ValidateProfile(ctx, updated) + if err != nil { + return nil, sdkerrors.Wrap(sdkerrors.ErrInvalidRequest, err.Error()) + } + + // Save the profile + err = k.StoreProfile(ctx, updated) + if err != nil { + return nil, sdkerrors.Wrap(sdkerrors.ErrInvalidRequest, err.Error()) + } + + ctx.EventManager().EmitEvent(sdk.NewEvent( + types.EventTypeProfileSaved, + sdk.NewAttribute(types.AttributeProfileDTag, updated.DTag), + sdk.NewAttribute(types.AttributeProfileCreator, updated.GetAddress().String()), + sdk.NewAttribute(types.AttributeProfileCreationTime, updated.CreationDate.Format(time.RFC3339Nano)), + )) + + return &types.MsgSaveProfileResponse{}, nil +} +``` + + +## Consequences + +### Backwards Compatibility + +As far as we know, these changes should not produce any backwards compatibility issue. + +### Positive + +* Protect users by accidental DTag edits + +### Negative + +- None known + +### Neutral + +- None known + +## Further Discussions + +## Test Cases [optional] + +## References + +- Issue [#622](https://github.com/desmos-labs/desmos/issues/622) \ No newline at end of file From 2bbe05a68a5188fab2509d097126e7026f93d175 Mon Sep 17 00:00:00 2001 From: Bax995 Date: Fri, 8 Oct 2021 10:27:04 +0200 Subject: [PATCH 2/7] first ADR review --- .../adr-009-avoid-accidental-dtag-override.md | 140 +++++++++++++----- 1 file changed, 107 insertions(+), 33 deletions(-) diff --git a/docs/architecture/adr-009-avoid-accidental-dtag-override.md b/docs/architecture/adr-009-avoid-accidental-dtag-override.md index 65bca9177c..3a8318a39b 100644 --- a/docs/architecture/adr-009-avoid-accidental-dtag-override.md +++ b/docs/architecture/adr-009-avoid-accidental-dtag-override.md @@ -3,6 +3,7 @@ ## Changelog - October 7th, 2021: Proposed +- October 8th, 2021: First review ## Status @@ -10,22 +11,23 @@ PROPOSED ## Abstract -We SHOULD edit the behaviour of the current `MsgSaveProfile` making the `DTag` an optional flag -in order to prevent users from accidentally overrides their own DTags. In this way, users will edit -their DTags only when they specify them with the flag. +We SHOULD edit the behavior of the current `MsgSaveProfile` making the `DTag` an optional flag +in order to prevent users from accidentally overriding their own DTags. In this way, users will edit +their DTag only when they specify them with the flag. ## Context -Currently, the `MsgSaveProfile` CLI command always requires to specify a DTag when using it. This means that +Currently, the `desmos profile tx save` CLI command always requires to specify a DTag when using it. This means that users that do not want to edit their DTag need to specify it anyway. This could lead to the situation where a user -accidentally make a typo while inserting the DTag triggering its edit. If this happens, and the user doesn't notice it -immediately, another user can register a profile with his DTag that is now free. +accidentally makes a typo while inserting the DTag triggering its edit. If this happens, and the user doesn't notice it +immediately, another user can register a profile with the now free DTag, stealing it from the original user. ## Decision -To avoid the situation described above, we need to perform some changes on how the `MsgSaveProfile` handles the DTag. -First, we need to make the `dtag` required field an optional flag by editing the command as follows: +To avoid the situation described above, we need to perform some changes on the logic that handles a `MsgSaveProfile`. +First, we need to edit the `desmos tx profiles save` CLI command so that the now required `dtag` field becomes +an optional flag: ```go func GetCmdSaveProfile() *cobra.Command { cmd := &cobra.Command{ @@ -39,7 +41,7 @@ If you are editing an existing profile you should fill only the fields that you The empty ones will be filled with a special [do-not-modify] flag that tells the system to not edit them. %s tx profiles save - --%s LeoDiCap \ + --%s "LeoDiCap"" \ --%s "Leonardo Di Caprio" \ --%s "Hollywood actor. Proud environmentalist" \ --%s "https://profilePic.jpg" \ @@ -77,7 +79,8 @@ The empty ones will be filled with a special [do-not-modify] flag that tells the return cmd } ``` -Second, we need to remove the check on DTag inside `ValidateBasic()` method of `MsgSaveProfile` that will result in: +Second, we need to remove the check on DTag inside `ValidateBasic()` that currently make it impossible to specify an +empty DTag inside a `MsgSaveProfile`: ```go func (msg MsgSaveProfile) ValidateBasic() error { _, err := sdk.AccAddressFromBech32(msg.Creator) @@ -88,7 +91,7 @@ func (msg MsgSaveProfile) ValidateBasic() error { } ``` -Third, we will edit the behaviour of `msgServer` `SaveProfile` method: +Third, we need to edit the behaviour of the `msgServer#SaveProfile` method: ```go func (k msgServer) SaveProfile(goCtx context.Context, msg *types.MsgSaveProfile) (*types.MsgSaveProfileResponse, error) { ctx := sdk.UnwrapSDKContext(goCtx) @@ -100,7 +103,6 @@ func (k msgServer) SaveProfile(goCtx context.Context, msg *types.MsgSaveProfile) // Create a new profile if not found if !found { - if msg.DTag == types.DoNotModify { return nil, sdkerrors.Wrapf(sdkerrors.ErrInvalidRequest, "DTag need to be specified if user doesn't have a profile") } @@ -115,25 +117,14 @@ func (k msgServer) SaveProfile(goCtx context.Context, msg *types.MsgSaveProfile) return nil, err } } - - var updated *types.Profile - if msg.DTag == types.DoNotModify { - // Update the existing profile with the values provided from the user - updated, err = profile.Update(types.NewProfileUpdate( - profile.DTag, - msg.Nickname, - msg.Bio, - types.NewPictures(msg.ProfilePicture, msg.CoverPicture), - )) - } else { - // Update the existing profile with the values provided from the user - updated, err = profile.Update(types.NewProfileUpdate( - msg.DTag, - msg.Nickname, - msg.Bio, - types.NewPictures(msg.ProfilePicture, msg.CoverPicture), - )) - } + + // Update the existing profile with the values provided from the user + updated, err = profile.Update(types.NewProfileUpdate( + msg.DTag, + msg.Nickname, + msg.Bio, + types.NewPictures(msg.ProfilePicture, msg.CoverPicture), + )) if err != nil { return nil, sdkerrors.Wrap(sdkerrors.ErrInvalidRequest, err.Error()) @@ -161,17 +152,100 @@ func (k msgServer) SaveProfile(goCtx context.Context, msg *types.MsgSaveProfile) return &types.MsgSaveProfileResponse{}, nil } ``` +And last as last step we need to edit the behavior of `Update` method. There are two possible ways to handle this: + +1) The method handles the empty DTag situation as it does when it finds the `DoNotModify` identifier. This +is a simple way to go, but it can cause some confusion in the user that tries to set an empty DTag and got no error back. +```go +func (p *Profile) Update(update *ProfileUpdate) (*Profile, error) { + if update.DTag == DoNotModify || strings.TrimSpace(update.DTag) == "" { + update.DTag = p.DTag + } + + if update.Nickname == DoNotModify { + update.Nickname = p.Nickname + } + + if update.Bio == DoNotModify { + update.Bio = p.Bio + } + + if update.Pictures.Profile == DoNotModify { + update.Pictures.Profile = p.Pictures.Profile + } + + if update.Pictures.Cover == DoNotModify { + update.Pictures.Cover = p.Pictures.Cover + } + newProfile, err := NewProfile( + update.DTag, + update.Nickname, + update.Bio, + update.Pictures, + p.CreationDate, + p.GetAccount(), + ) + if err != nil { + return nil, err + } + + return newProfile, nil +} +``` + +2) In the second, we return an error when an empty DTag is given: +```go +func (p *Profile) Update(update *ProfileUpdate) (*Profile, error) { + if update.DTag == DoNotModify { + update.DTag = p.DTag + } + + if strings.TrimSpace(update.DTag) == "" { + return nil, fmt.Errorf("Profile's DTag cannot be set empty or blank") + } + + if update.Nickname == DoNotModify { + update.Nickname = p.Nickname + } + + if update.Bio == DoNotModify { + update.Bio = p.Bio + } + + if update.Pictures.Profile == DoNotModify { + update.Pictures.Profile = p.Pictures.Profile + } + + if update.Pictures.Cover == DoNotModify { + update.Pictures.Cover = p.Pictures.Cover + } + + newProfile, err := NewProfile( + update.DTag, + update.Nickname, + update.Bio, + update.Pictures, + p.CreationDate, + p.GetAccount(), + ) + if err != nil { + return nil, err + } + + return newProfile, nil +} +``` ## Consequences ### Backwards Compatibility -As far as we know, these changes should not produce any backwards compatibility issue. +There are no backwards compatibility issues related to these changes. ### Positive -* Protect users by accidental DTag edits +* Protect users from accidentally editing their DTag. ### Negative From 59a58d76537e63f0e3f3ca638e0570482adf8e84 Mon Sep 17 00:00:00 2001 From: Bax995 Date: Fri, 8 Oct 2021 11:32:21 +0200 Subject: [PATCH 3/7] fixed some stuff according to review --- .../adr-009-avoid-accidental-dtag-override.md | 50 ++----------------- 1 file changed, 3 insertions(+), 47 deletions(-) diff --git a/docs/architecture/adr-009-avoid-accidental-dtag-override.md b/docs/architecture/adr-009-avoid-accidental-dtag-override.md index 3a8318a39b..69a0181f93 100644 --- a/docs/architecture/adr-009-avoid-accidental-dtag-override.md +++ b/docs/architecture/adr-009-avoid-accidental-dtag-override.md @@ -41,7 +41,7 @@ If you are editing an existing profile you should fill only the fields that you The empty ones will be filled with a special [do-not-modify] flag that tells the system to not edit them. %s tx profiles save - --%s "LeoDiCap"" \ + --%s "LeoDiCap" \ --%s "Leonardo Di Caprio" \ --%s "Hollywood actor. Proud environmentalist" \ --%s "https://profilePic.jpg" \ @@ -79,7 +79,7 @@ The empty ones will be filled with a special [do-not-modify] flag that tells the return cmd } ``` -Second, we need to remove the check on DTag inside `ValidateBasic()` that currently make it impossible to specify an +Second, we need to remove the check on DTag inside `ValidateBasic()` that currently makes it impossible to specify an empty DTag inside a `MsgSaveProfile`: ```go func (msg MsgSaveProfile) ValidateBasic() error { @@ -125,7 +125,6 @@ func (k msgServer) SaveProfile(goCtx context.Context, msg *types.MsgSaveProfile) msg.Bio, types.NewPictures(msg.ProfilePicture, msg.CoverPicture), )) - if err != nil { return nil, sdkerrors.Wrap(sdkerrors.ErrInvalidRequest, err.Error()) } @@ -152,58 +151,15 @@ func (k msgServer) SaveProfile(goCtx context.Context, msg *types.MsgSaveProfile) return &types.MsgSaveProfileResponse{}, nil } ``` -And last as last step we need to edit the behavior of `Update` method. There are two possible ways to handle this: +Finally, we need to edit the behavior of `Update` method. There are two possible ways to handle this: 1) The method handles the empty DTag situation as it does when it finds the `DoNotModify` identifier. This is a simple way to go, but it can cause some confusion in the user that tries to set an empty DTag and got no error back. ```go -func (p *Profile) Update(update *ProfileUpdate) (*Profile, error) { - if update.DTag == DoNotModify || strings.TrimSpace(update.DTag) == "" { - update.DTag = p.DTag - } - - if update.Nickname == DoNotModify { - update.Nickname = p.Nickname - } - - if update.Bio == DoNotModify { - update.Bio = p.Bio - } - - if update.Pictures.Profile == DoNotModify { - update.Pictures.Profile = p.Pictures.Profile - } - - if update.Pictures.Cover == DoNotModify { - update.Pictures.Cover = p.Pictures.Cover - } - - newProfile, err := NewProfile( - update.DTag, - update.Nickname, - update.Bio, - update.Pictures, - p.CreationDate, - p.GetAccount(), - ) - if err != nil { - return nil, err - } - - return newProfile, nil -} -``` - -2) In the second, we return an error when an empty DTag is given: -```go func (p *Profile) Update(update *ProfileUpdate) (*Profile, error) { if update.DTag == DoNotModify { update.DTag = p.DTag } - - if strings.TrimSpace(update.DTag) == "" { - return nil, fmt.Errorf("Profile's DTag cannot be set empty or blank") - } if update.Nickname == DoNotModify { update.Nickname = p.Nickname From 592016822b23a8dbd0cad5a59659df7944e5bfed Mon Sep 17 00:00:00 2001 From: Bax995 Date: Fri, 8 Oct 2021 15:28:39 +0200 Subject: [PATCH 4/7] added test cases to ADR --- .../adr-009-avoid-accidental-dtag-override.md | 152 ++++++++++++++++++ 1 file changed, 152 insertions(+) diff --git a/docs/architecture/adr-009-avoid-accidental-dtag-override.md b/docs/architecture/adr-009-avoid-accidental-dtag-override.md index 69a0181f93..b41a1f1ba4 100644 --- a/docs/architecture/adr-009-avoid-accidental-dtag-override.md +++ b/docs/architecture/adr-009-avoid-accidental-dtag-override.md @@ -215,6 +215,158 @@ There are no backwards compatibility issues related to these changes. ## Test Cases [optional] +The following tests cases needs to be added: +1) Creating a profile with an empty DTag returns an error; +2) Creating a profile with DTag [do-not-modify] returns an error; +3) Updating a profile with a different DTag changes its value and returns no error; +4) Updating a profile with DTag [do-not-modify] does not update its value and returns no error. + +```go +func TestProfile_Validate(t *testing.T) { + testCases := []struct { + name string + account *types.Profile + shouldErr bool + }{ + { + name: "empty profile DTag returns error", + account: testutil.AssertNoProfileError(types.NewProfile( + "", + "", + "bio", + types.NewPictures( + "https://shorturl.at/adnX3", + "https://shorturl.at/cgpyF", + ), + time.Now(), + testutil.AccountFromAddr("cosmos1y54exmx84cqtasvjnskf9f63djuuj68p7hqf47"), + )), + shouldErr: true, + }, + { + name: "setting the dTag to DoNotModify returns error", + account: testutil.AssertNoProfileError(types.NewProfile( + types.DoNotModify, + "", + "", + types.Pictures{}, + time.Now(), + testutil.AccountFromAddr("cosmos1y54exmx84cqtasvjnskf9f63djuuj68p7hqf47"), + )), + shouldErr: true, + }, + } + + for _, tc := range testCases { + tc := tc + t.Run(tc.name, func(t *testing.T) { + err := tc.account.Validate() + + if tc.shouldErr { + require.Error(t, err) + } else { + require.NoError(t, err) + } + }) + } +} +``` + +```go +func TestProfile_Update(t *testing.T) { + testCases := []struct { + name string + original *types.Profile + update *types.ProfileUpdate + shouldErr bool + expProfile *types.Profile + }{ + { + name: "DoNotModify does not update original values", + original: testutil.AssertNoProfileError(types.NewProfile( + "dtag", + "nickname", + "bio", + types.NewPictures( + "https://example.com", + "https://example.com", + ), + time.Unix(100, 0), + testutil.AccountFromAddr("cosmos1y54exmx84cqtasvjnskf9f63djuuj68p7hqf47"), + )), + update: types.NewProfileUpdate( + types.DoNotModify, + "", + types.DoNotModify, + types.NewPictures( + types.DoNotModify, + "", + ), + ), + shouldErr: false, + expProfile: testutil.AssertNoProfileError(types.NewProfile( + "dtag", + "", + "bio", + types.NewPictures( + "https://example.com", + "", + ), + time.Unix(100, 0), + testutil.AccountFromAddr("cosmos1y54exmx84cqtasvjnskf9f63djuuj68p7hqf47"), + )), + }, + { + name: "New DTag update original one", + original: testutil.AssertNoProfileError(types.NewProfile( + "dtag", + "nickname", + "bio", + types.NewPictures( + "https://example.com", + "https://example.com", + ), + time.Unix(100, 0), + testutil.AccountFromAddr("cosmos1y54exmx84cqtasvjnskf9f63djuuj68p7hqf47"), + )), + update: types.NewProfileUpdate( + "newDtag", + "", + types.DoNotModify, + types.NewPictures( + types.DoNotModify, + "", + ), + ), + shouldErr: false, + expProfile: testutil.AssertNoProfileError(types.NewProfile( + "newDTag", + "", + "bio", + types.NewPictures( + "https://example.com", + "", + ), + time.Unix(100, 0), + testutil.AccountFromAddr("cosmos1y54exmx84cqtasvjnskf9f63djuuj68p7hqf47"), + )), + }, + } + for _, tc := range testCases { + tc := tc + t.Run(tc.name, func(t *testing.T) { + updated, err := tc.original.Update(tc.update) + if tc.shouldErr { + require.Error(t, err) + } else { + require.NoError(t, err) + require.Equal(t, tc.expProfile, updated) + } + }) + } +} +``` + ## References - Issue [#622](https://github.com/desmos-labs/desmos/issues/622) \ No newline at end of file From 78e928ef5350e95535dda47448537d03dbba08a4 Mon Sep 17 00:00:00 2001 From: Bax995 Date: Mon, 11 Oct 2021 10:14:03 +0200 Subject: [PATCH 5/7] Last fixes and ADR finalization --- .../adr-009-avoid-accidental-dtag-override.md | 252 +----------------- 1 file changed, 2 insertions(+), 250 deletions(-) diff --git a/docs/architecture/adr-009-avoid-accidental-dtag-override.md b/docs/architecture/adr-009-avoid-accidental-dtag-override.md index b41a1f1ba4..986e32fcbe 100644 --- a/docs/architecture/adr-009-avoid-accidental-dtag-override.md +++ b/docs/architecture/adr-009-avoid-accidental-dtag-override.md @@ -4,6 +4,7 @@ - October 7th, 2021: Proposed - October 8th, 2021: First review +- October 11th, 2021: Finalized ADR ## Status @@ -15,7 +16,6 @@ We SHOULD edit the behavior of the current `MsgSaveProfile` making the `DTag` an in order to prevent users from accidentally overriding their own DTags. In this way, users will edit their DTag only when they specify them with the flag. - ## Context Currently, the `desmos profile tx save` CLI command always requires to specify a DTag when using it. This means that @@ -41,7 +41,7 @@ If you are editing an existing profile you should fill only the fields that you The empty ones will be filled with a special [do-not-modify] flag that tells the system to not edit them. %s tx profiles save - --%s "LeoDiCap" \ + --%s "LeoDiCaprio" \ --%s "Leonardo Di Caprio" \ --%s "Hollywood actor. Proud environmentalist" \ --%s "https://profilePic.jpg" \ @@ -91,108 +91,6 @@ func (msg MsgSaveProfile) ValidateBasic() error { } ``` -Third, we need to edit the behaviour of the `msgServer#SaveProfile` method: -```go -func (k msgServer) SaveProfile(goCtx context.Context, msg *types.MsgSaveProfile) (*types.MsgSaveProfileResponse, error) { - ctx := sdk.UnwrapSDKContext(goCtx) - - profile, found, err := k.GetProfile(ctx, msg.Creator) - if err != nil { - return nil, err - } - - // Create a new profile if not found - if !found { - if msg.DTag == types.DoNotModify { - return nil, sdkerrors.Wrapf(sdkerrors.ErrInvalidRequest, "DTag need to be specified if user doesn't have a profile") - } - - addr, err := sdk.AccAddressFromBech32(msg.Creator) - if err != nil { - return nil, err - } - - profile, err = types.NewProfileFromAccount(msg.DTag, k.ak.GetAccount(ctx, addr), ctx.BlockTime()) - if err != nil { - return nil, err - } - } - - // Update the existing profile with the values provided from the user - updated, err = profile.Update(types.NewProfileUpdate( - msg.DTag, - msg.Nickname, - msg.Bio, - types.NewPictures(msg.ProfilePicture, msg.CoverPicture), - )) - if err != nil { - return nil, sdkerrors.Wrap(sdkerrors.ErrInvalidRequest, err.Error()) - } - - // Validate the profile - err = k.ValidateProfile(ctx, updated) - if err != nil { - return nil, sdkerrors.Wrap(sdkerrors.ErrInvalidRequest, err.Error()) - } - - // Save the profile - err = k.StoreProfile(ctx, updated) - if err != nil { - return nil, sdkerrors.Wrap(sdkerrors.ErrInvalidRequest, err.Error()) - } - - ctx.EventManager().EmitEvent(sdk.NewEvent( - types.EventTypeProfileSaved, - sdk.NewAttribute(types.AttributeProfileDTag, updated.DTag), - sdk.NewAttribute(types.AttributeProfileCreator, updated.GetAddress().String()), - sdk.NewAttribute(types.AttributeProfileCreationTime, updated.CreationDate.Format(time.RFC3339Nano)), - )) - - return &types.MsgSaveProfileResponse{}, nil -} -``` -Finally, we need to edit the behavior of `Update` method. There are two possible ways to handle this: - -1) The method handles the empty DTag situation as it does when it finds the `DoNotModify` identifier. This -is a simple way to go, but it can cause some confusion in the user that tries to set an empty DTag and got no error back. -```go -func (p *Profile) Update(update *ProfileUpdate) (*Profile, error) { - if update.DTag == DoNotModify { - update.DTag = p.DTag - } - - if update.Nickname == DoNotModify { - update.Nickname = p.Nickname - } - - if update.Bio == DoNotModify { - update.Bio = p.Bio - } - - if update.Pictures.Profile == DoNotModify { - update.Pictures.Profile = p.Pictures.Profile - } - - if update.Pictures.Cover == DoNotModify { - update.Pictures.Cover = p.Pictures.Cover - } - - newProfile, err := NewProfile( - update.DTag, - update.Nickname, - update.Bio, - update.Pictures, - p.CreationDate, - p.GetAccount(), - ) - if err != nil { - return nil, err - } - - return newProfile, nil -} -``` - ## Consequences ### Backwards Compatibility @@ -221,152 +119,6 @@ The following tests cases needs to be added: 3) Updating a profile with a different DTag changes its value and returns no error; 4) Updating a profile with DTag [do-not-modify] does not update its value and returns no error. -```go -func TestProfile_Validate(t *testing.T) { - testCases := []struct { - name string - account *types.Profile - shouldErr bool - }{ - { - name: "empty profile DTag returns error", - account: testutil.AssertNoProfileError(types.NewProfile( - "", - "", - "bio", - types.NewPictures( - "https://shorturl.at/adnX3", - "https://shorturl.at/cgpyF", - ), - time.Now(), - testutil.AccountFromAddr("cosmos1y54exmx84cqtasvjnskf9f63djuuj68p7hqf47"), - )), - shouldErr: true, - }, - { - name: "setting the dTag to DoNotModify returns error", - account: testutil.AssertNoProfileError(types.NewProfile( - types.DoNotModify, - "", - "", - types.Pictures{}, - time.Now(), - testutil.AccountFromAddr("cosmos1y54exmx84cqtasvjnskf9f63djuuj68p7hqf47"), - )), - shouldErr: true, - }, - } - - for _, tc := range testCases { - tc := tc - t.Run(tc.name, func(t *testing.T) { - err := tc.account.Validate() - - if tc.shouldErr { - require.Error(t, err) - } else { - require.NoError(t, err) - } - }) - } -} -``` - -```go -func TestProfile_Update(t *testing.T) { - testCases := []struct { - name string - original *types.Profile - update *types.ProfileUpdate - shouldErr bool - expProfile *types.Profile - }{ - { - name: "DoNotModify does not update original values", - original: testutil.AssertNoProfileError(types.NewProfile( - "dtag", - "nickname", - "bio", - types.NewPictures( - "https://example.com", - "https://example.com", - ), - time.Unix(100, 0), - testutil.AccountFromAddr("cosmos1y54exmx84cqtasvjnskf9f63djuuj68p7hqf47"), - )), - update: types.NewProfileUpdate( - types.DoNotModify, - "", - types.DoNotModify, - types.NewPictures( - types.DoNotModify, - "", - ), - ), - shouldErr: false, - expProfile: testutil.AssertNoProfileError(types.NewProfile( - "dtag", - "", - "bio", - types.NewPictures( - "https://example.com", - "", - ), - time.Unix(100, 0), - testutil.AccountFromAddr("cosmos1y54exmx84cqtasvjnskf9f63djuuj68p7hqf47"), - )), - }, - { - name: "New DTag update original one", - original: testutil.AssertNoProfileError(types.NewProfile( - "dtag", - "nickname", - "bio", - types.NewPictures( - "https://example.com", - "https://example.com", - ), - time.Unix(100, 0), - testutil.AccountFromAddr("cosmos1y54exmx84cqtasvjnskf9f63djuuj68p7hqf47"), - )), - update: types.NewProfileUpdate( - "newDtag", - "", - types.DoNotModify, - types.NewPictures( - types.DoNotModify, - "", - ), - ), - shouldErr: false, - expProfile: testutil.AssertNoProfileError(types.NewProfile( - "newDTag", - "", - "bio", - types.NewPictures( - "https://example.com", - "", - ), - time.Unix(100, 0), - testutil.AccountFromAddr("cosmos1y54exmx84cqtasvjnskf9f63djuuj68p7hqf47"), - )), - }, - } - for _, tc := range testCases { - tc := tc - t.Run(tc.name, func(t *testing.T) { - updated, err := tc.original.Update(tc.update) - if tc.shouldErr { - require.Error(t, err) - } else { - require.NoError(t, err) - require.Equal(t, tc.expProfile, updated) - } - }) - } -} -``` - ## References - Issue [#622](https://github.com/desmos-labs/desmos/issues/622) \ No newline at end of file From 8bbd9bc4ed1f62ca820d80b9ea5019a4020e9fe8 Mon Sep 17 00:00:00 2001 From: Riccardo Montagnin Date: Tue, 12 Oct 2021 21:29:59 +0200 Subject: [PATCH 6/7] Update docs/architecture/adr-009-avoid-accidental-dtag-override.md --- docs/architecture/adr-009-avoid-accidental-dtag-override.md | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/docs/architecture/adr-009-avoid-accidental-dtag-override.md b/docs/architecture/adr-009-avoid-accidental-dtag-override.md index 986e32fcbe..8de18ea46d 100644 --- a/docs/architecture/adr-009-avoid-accidental-dtag-override.md +++ b/docs/architecture/adr-009-avoid-accidental-dtag-override.md @@ -41,7 +41,7 @@ If you are editing an existing profile you should fill only the fields that you The empty ones will be filled with a special [do-not-modify] flag that tells the system to not edit them. %s tx profiles save - --%s "LeoDiCaprio" \ + --%s "LeoDiCaprio" \ --%s "Leonardo Di Caprio" \ --%s "Hollywood actor. Proud environmentalist" \ --%s "https://profilePic.jpg" \ From c857c5b90ec314f40fc1906e3de6dcec03479d74 Mon Sep 17 00:00:00 2001 From: Riccardo Montagnin Date: Tue, 12 Oct 2021 21:30:06 +0200 Subject: [PATCH 7/7] Update docs/architecture/adr-009-avoid-accidental-dtag-override.md --- .../adr-009-avoid-accidental-dtag-override.md | 10 +++++----- 1 file changed, 5 insertions(+), 5 deletions(-) diff --git a/docs/architecture/adr-009-avoid-accidental-dtag-override.md b/docs/architecture/adr-009-avoid-accidental-dtag-override.md index 8de18ea46d..294847be2d 100644 --- a/docs/architecture/adr-009-avoid-accidental-dtag-override.md +++ b/docs/architecture/adr-009-avoid-accidental-dtag-override.md @@ -113,11 +113,11 @@ There are no backwards compatibility issues related to these changes. ## Test Cases [optional] -The following tests cases needs to be added: -1) Creating a profile with an empty DTag returns an error; -2) Creating a profile with DTag [do-not-modify] returns an error; -3) Updating a profile with a different DTag changes its value and returns no error; -4) Updating a profile with DTag [do-not-modify] does not update its value and returns no error. +The following tests cases MUST to be present: +1. Creating a profile with an empty DTag returns an error; +2. Creating a profile with DTag `[do-not-modify]` returns an error; +3. Updating a profile with a different DTag changes its value and returns no error; +4. Updating a profile with DTag `[do-not-modify]` does not update its value and returns no error. ## References