fix: application link verification flow breaks with unordered IBC cha…

…nnels (#670)

## Description
This PR fixes the bugs described inside #669 by applying some small changes: 
1. do not update application links if they are already in either `AppLinkStateVerificationError`, `AppLinkStateVerificationSuccess` or `AppLinkStateVerificationTimedOut` 
2. do not error when we try to update a link that does not exist 

Closes: #669

<!-- Add a description of the changes that this PR introduces and the files that
are the most critical to review. -->

---

### 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...

- [x] included the correct [type prefix](https://github.com/commitizen/conventional-commit-types/blob/v3.0.0/index.json) in the PR title
- [ ] added `!` to the type prefix if API or client breaking change
- [x] targeted the correct branch (see [PR Targeting](https://github.com/desmos-labs/desmos/blob/master/CONTRIBUTING.md#pr-targeting))
- [ ] provided a link to the relevant issue or specification
- [ ] followed the guidelines for [building modules](https://docs.cosmos.network/v0.44/building-modules/intro.html)
- [x] included the necessary unit and integration [tests](https://github.com/desmos-labs/desmos/blob/master/CONTRIBUTING.md#testing)
- [x] added a changelog entry to `CHANGELOG.md`
- [ ] included comments for [documenting Go code](https://blog.golang.org/godoc)
- [ ] updated the relevant documentation or specification
- [x] reviewed "Files changed" and left comments if necessary
- [ ] confirmed all CI checks have passed

### 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](https://github.com/commitizen/conventional-commit-types/blob/v3.0.0/index.json) 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)

.changeset/entries/7b2791e321645b1b6ce07eb919f8aa1680a304e4c14b74d95994a5b6d88fef5d.yaml

@@ -0,0 +1,6 @@
+type: fix
+module: x/profiles
+pull_request: 670
+description: Fixed application link verification flow breaking on unordered IBC channels
+backward_compatible: false
+date: 2021-11-02T10:01:35.042284589Z

x/profiles/keeper/grpc_query.go

@@ -231,14 +231,14 @@ func (k Keeper) UserApplicationLink(ctx context.Context, request *types.QueryUse
 func (k Keeper) ApplicationLinkByClientID(ctx context.Context, request *types.QueryApplicationLinkByClientIDRequest) (*types.QueryApplicationLinkByClientIDResponse, error) {
 	sdkCtx := sdk.UnwrapSDKContext(ctx)
 
-	link, err := k.GetApplicationLinkByClientID(sdkCtx, request.ClientId)
+	link, found, err := k.GetApplicationLinkByClientID(sdkCtx, request.ClientId)
 	if err != nil {
-		if sdkerrors.ErrNotFound.Is(err) {
-			return nil, status.Errorf(codes.NotFound, "link for client id %s not found", request.ClientId)
-		}
-
 		return nil, status.Error(codes.Internal, err.Error())
 	}
 
+	if !found {
+		return nil, status.Errorf(codes.NotFound, "link for client id %s not found", request.ClientId)
+	}
+
 	return &types.QueryApplicationLinkByClientIDResponse{Link: link}, nil
 }

x/profiles/keeper/keeper_app_links.go

@@ -60,15 +60,15 @@ func (k Keeper) GetApplicationLink(ctx sdk.Context, user, application, username
 	return link, true, nil
 }
 
-// GetApplicationLinkByClientID returns the application link and user given a specific client id
-func (k Keeper) GetApplicationLinkByClientID(ctx sdk.Context, clientID string) (types.ApplicationLink, error) {
+// GetApplicationLinkByClientID returns the application link and user given a specific client id.
+// If the link is not found, returns false instead.
+func (k Keeper) GetApplicationLinkByClientID(ctx sdk.Context, clientID string) (types.ApplicationLink, bool, error) {
 	store := ctx.KVStore(k.storeKey)
 
 	// Get the client request using the client id
 	clientIDKey := types.ApplicationLinkClientIDKey(clientID)
 	if !store.Has(clientIDKey) {
-		return types.ApplicationLink{},
-			sdkerrors.Wrapf(sdkerrors.ErrNotFound, "link for client id %s not found", clientID)
+		return types.ApplicationLink{}, false, nil
 	}
 
 	// Get the link key
@@ -78,10 +78,10 @@ func (k Keeper) GetApplicationLinkByClientID(ctx sdk.Context, clientID string) (
 	var link types.ApplicationLink
 	err := k.cdc.Unmarshal(store.Get(applicationLinkKey), &link)
 	if err != nil {
-		return types.ApplicationLink{}, sdkerrors.Wrap(err, "error while reading application link")
+		return types.ApplicationLink{}, true, sdkerrors.Wrap(err, "error while reading application link")
 	}
 
-	return link, nil
+	return link, true, nil
 }
 
 // DeleteApplicationLink removes the application link associated to the given user,

x/profiles/keeper/keeper_app_links_test.go

@@ -236,13 +236,15 @@ func (suite *KeeperTestSuite) Test_GetApplicationLinkByClientID() {
 		name      string
 		store     func(ctx sdk.Context)
 		clientID  string
+		expFound  bool
 		shouldErr bool
 		expLink   types.ApplicationLink
 	}{
 		{
-			name:      "invalid client id returns error",
+			name:      "invalid client id returns false",
 			clientID:  "client_id",
-			shouldErr: true,
+			expFound:  false,
+			shouldErr: false,
 		},
 		{
 			name: "valid client id returns proper data",
@@ -267,6 +269,7 @@ func (suite *KeeperTestSuite) Test_GetApplicationLinkByClientID() {
 				err := suite.k.SaveApplicationLink(ctx, link)
 				suite.Require().NoError(err)
 			},
+			expFound:  true,
 			shouldErr: false,
 			clientID:  "client_id",
 			expLink: types.NewApplicationLink(
@@ -293,12 +296,15 @@ func (suite *KeeperTestSuite) Test_GetApplicationLinkByClientID() {
 				tc.store(ctx)
 			}
 
-			link, err := suite.k.GetApplicationLinkByClientID(ctx, tc.clientID)
+			link, found, err := suite.k.GetApplicationLinkByClientID(ctx, tc.clientID)
 			if tc.shouldErr {
 				suite.Require().Error(err)
 			} else {
 				suite.Require().NoError(err)
-				suite.Require().Equal(tc.expLink, link)
+				suite.Require().Equal(tc.expFound, found)
+				if tc.expFound {
+					suite.Require().Equal(tc.expLink, link)
+				}
 			}
 		})
 	}

x/profiles/keeper/relay_app_links.go

@@ -154,11 +154,21 @@ func (k Keeper) OnRecvApplicationLinkPacketData(
 	data oracletypes.OracleResponsePacketData,
 ) error {
 	// Get the request by the client ID
-	link, err := k.GetApplicationLinkByClientID(ctx, data.ClientID)
+	link, found, err := k.GetApplicationLinkByClientID(ctx, data.ClientID)
 	if err != nil {
 		return err
 	}
 
+	// If the link is not found, do nothing (it might have been deleted by the user in the meanwhile)
+	if !found {
+		return nil
+	}
+
+	// If the link has already been verified, do nothing
+	if link.IsVerificationCompleted() {
+		return nil
+	}
+
 	switch data.ResolveStatus {
 	case oracletypes.RESOLVE_STATUS_EXPIRED:
 		link.State = types.AppLinkStateVerificationError
@@ -218,11 +228,16 @@ func (k Keeper) OnOracleRequestAcknowledgementPacket(
 	ack channeltypes.Acknowledgement,
 ) error {
 	// Get the request by the client ID
-	link, err := k.GetApplicationLinkByClientID(ctx, data.ClientID)
+	link, found, err := k.GetApplicationLinkByClientID(ctx, data.ClientID)
 	if err != nil {
 		return err
 	}
 
+	// If the link is not found, do nothing (it might have been deleted by the user in the meanwhile)
+	if !found {
+		return nil
+	}
+
 	switch res := ack.Response.(type) {
 	case *channeltypes.Acknowledgement_Error:
 		// The acknowledgment failed on the receiving chain.
@@ -255,12 +270,17 @@ func (k Keeper) OnOracleRequestTimeoutPacket(
 	data oracletypes.OracleRequestPacketData,
 ) error {
 	// Get the request by the client ID
-	connection, err := k.GetApplicationLinkByClientID(ctx, data.ClientID)
+	link, found, err := k.GetApplicationLinkByClientID(ctx, data.ClientID)
 	if err != nil {
 		return err
 	}
 
-	connection.State = types.AppLinkStateVerificationTimedOut
+	// If the link is not found, do nothing (it might have been deleted by the user in the meanwhile)
+	if !found {
+		return nil
+	}
+
+	link.State = types.AppLinkStateVerificationTimedOut
 
-	return k.SaveApplicationLink(ctx, connection)
+	return k.SaveApplicationLink(ctx, link)
 }

x/profiles/keeper/relay_app_links_test.go

@@ -205,14 +205,14 @@ func (suite *KeeperTestSuite) TestKeeper_OnRecvApplicationLinkPacketData() {
 		expLink   types.ApplicationLink
 	}{
 		{
-			name: "non existing connection returns error",
+			name: "non existing link returns no error",
 			data: createResponsePacketData(
 				"client_id",
 				0,
 				oracletypes.RESOLVE_STATUS_SUCCESS,
 				"",
 			),
-			shouldErr: true,
+			shouldErr: false,
 		},
 		{
 			name: "resolve status expired updates connection properly",
@@ -482,6 +482,117 @@ func (suite *KeeperTestSuite) TestKeeper_OnRecvApplicationLinkPacketData() {
 				time.Date(2020, 1, 1, 00, 00, 00, 000, time.UTC),
 			),
 		},
+		{
+			name: "timed out link does not get updated",
+			store: func(ctx sdk.Context) {
+				suite.ak.SetAccount(ctx, profile.Profile)
+
+				link := types.NewApplicationLink(
+					profile.GetAddress().String(),
+					types.NewData("twitter", username),
+					types.AppLinkStateVerificationTimedOut,
+					types.NewOracleRequest(
+						1,
+						1,
+						types.NewOracleRequestCallData("twitter", "tweet-123456789"),
+						"client_id",
+					),
+					nil,
+					time.Date(2020, 1, 1, 00, 00, 00, 000, time.UTC),
+				)
+				err := suite.k.SaveApplicationLink(ctx, link)
+				suite.Require().NoError(err)
+			},
+			data:      createResponsePacketData("client_id", 1, oracletypes.RESOLVE_STATUS_SUCCESS, resultBase64),
+			shouldErr: false,
+			expLink: types.NewApplicationLink(
+				profile.GetAddress().String(),
+				types.NewData("twitter", username),
+				types.AppLinkStateVerificationTimedOut,
+				types.NewOracleRequest(
+					1,
+					1,
+					types.NewOracleRequestCallData("twitter", "tweet-123456789"),
+					"client_id",
+				),
+				nil,
+				time.Date(2020, 1, 1, 00, 00, 00, 000, time.UTC),
+			),
+		},
+		{
+			name: "errored link does not get updated",
+			store: func(ctx sdk.Context) {
+				suite.ak.SetAccount(ctx, profile.Profile)
+
+				link := types.NewApplicationLink(
+					profile.GetAddress().String(),
+					types.NewData("twitter", username),
+					types.AppLinkStateVerificationError,
+					types.NewOracleRequest(
+						1,
+						1,
+						types.NewOracleRequestCallData("twitter", "tweet-123456789"),
+						"client_id",
+					),
+					types.NewErrorResult(types.ErrInvalidSignature),
+					time.Date(2020, 1, 1, 00, 00, 00, 000, time.UTC),
+				)
+				err := suite.k.SaveApplicationLink(ctx, link)
+				suite.Require().NoError(err)
+			},
+			data:      createResponsePacketData("client_id", 1, oracletypes.RESOLVE_STATUS_SUCCESS, resultBase64),
+			shouldErr: false,
+			expLink: types.NewApplicationLink(
+				profile.GetAddress().String(),
+				types.NewData("twitter", username),
+				types.AppLinkStateVerificationError,
+				types.NewOracleRequest(
+					1,
+					1,
+					types.NewOracleRequestCallData("twitter", "tweet-123456789"),
+					"client_id",
+				),
+				types.NewErrorResult(types.ErrInvalidSignature),
+				time.Date(2020, 1, 1, 00, 00, 00, 000, time.UTC),
+			),
+		},
+		{
+			name: "already verified link does not get updated",
+			store: func(ctx sdk.Context) {
+				suite.ak.SetAccount(ctx, profile.Profile)
+
+				link := types.NewApplicationLink(
+					profile.GetAddress().String(),
+					types.NewData("twitter", username),
+					types.AppLinkStateVerificationSuccess,
+					types.NewOracleRequest(
+						1,
+						1,
+						types.NewOracleRequestCallData("twitter", "tweet-123456789"),
+						"client_id",
+					),
+					types.NewSuccessResult("value", "signature"),
+					time.Date(2020, 1, 1, 00, 00, 00, 000, time.UTC),
+				)
+				err := suite.k.SaveApplicationLink(ctx, link)
+				suite.Require().NoError(err)
+			},
+			data:      createResponsePacketData("client_id", 1, oracletypes.RESOLVE_STATUS_SUCCESS, resultBase64),
+			shouldErr: false,
+			expLink: types.NewApplicationLink(
+				profile.GetAddress().String(),
+				types.NewData("twitter", username),
+				types.AppLinkStateVerificationSuccess,
+				types.NewOracleRequest(
+					1,
+					1,
+					types.NewOracleRequestCallData("twitter", "tweet-123456789"),
+					"client_id",
+				),
+				types.NewSuccessResult("value", "signature"),
+				time.Date(2020, 1, 1, 00, 00, 00, 000, time.UTC),
+			),
+		},
 	}
 
 	for _, tc := range testCases {
@@ -499,7 +610,7 @@ func (suite *KeeperTestSuite) TestKeeper_OnRecvApplicationLinkPacketData() {
 			} else {
 				suite.Require().NoError(err)
 
-				stored, err := suite.k.GetApplicationLinkByClientID(ctx, tc.expLink.OracleRequest.ClientID)
+				stored, _, err := suite.k.GetApplicationLinkByClientID(ctx, tc.expLink.OracleRequest.ClientID)
 				suite.Require().NoError(err)
 				suite.Require().Truef(tc.expLink.Equal(stored), "%s\n%s", tc.expLink, stored)
 			}
@@ -519,10 +630,10 @@ func (suite *KeeperTestSuite) TestKeeper_OnOracleRequestAcknowledgementPacket()
 		expLink   types.ApplicationLink
 	}{
 		{
-			name:      "invalid client id returns error",
+			name:      "non existing link returns no error",
 			data:      createRequestPacketData("client_id"),
 			ack:       channeltypes.NewErrorAcknowledgement("error"),
-			shouldErr: true,
+			shouldErr: false,
 		},
 		{
 			name: "acknowledgment error updates link properly",
@@ -644,7 +755,7 @@ func (suite *KeeperTestSuite) TestKeeper_OnOracleRequestAcknowledgementPacket()
 			} else {
 				suite.Require().NoError(err)
 
-				link, err := suite.k.GetApplicationLinkByClientID(ctx, tc.data.ClientID)
+				link, _, err := suite.k.GetApplicationLinkByClientID(ctx, tc.data.ClientID)
 				suite.Require().NoError(err)
 				suite.Require().Equal(tc.expLink, link)
 			}
@@ -661,9 +772,9 @@ func (suite *KeeperTestSuite) TestKeeper_OnOracleRequestTimeoutPacket() {
 		check     func(ctx sdk.Context)
 	}{
 		{
-			name:      "invalid client id request returns error",
+			name:      "not found link returns no error",
 			data:      createRequestPacketData("client_id"),
-			shouldErr: true,
+			shouldErr: false,
 		},
 		{
 			name: "valid client id updates the link properly",
@@ -689,7 +800,7 @@ func (suite *KeeperTestSuite) TestKeeper_OnOracleRequestTimeoutPacket() {
 			data:      createRequestPacketData("client_id"),
 			shouldErr: false,
 			check: func(ctx sdk.Context) {
-				link, err := suite.k.GetApplicationLinkByClientID(ctx, "client_id")
+				link, _, err := suite.k.GetApplicationLinkByClientID(ctx, "client_id")
 				suite.Require().NoError(err)
 
 				expected := types.NewApplicationLink(

x/profiles/types/models_app_links.go

@@ -60,6 +60,13 @@ func (l *ApplicationLink) IsVerificationOngoing() bool {
 	return l.State == ApplicationLinkStateInitialized || l.State == AppLinkStateVerificationStarted
 }
 
+// IsVerificationCompleted tells whether the verification for the link has completed or not
+func (l *ApplicationLink) IsVerificationCompleted() bool {
+	return l.State == AppLinkStateVerificationSuccess ||
+		l.State == AppLinkStateVerificationError ||
+		l.State == AppLinkStateVerificationTimedOut
+}
+
 // MustMarshalApplicationLink serializes the given application link using the provided BinaryCodec
 func MustMarshalApplicationLink(cdc codec.BinaryCodec, link ApplicationLink) []byte {
 	return cdc.MustMarshal(&link)