From 9da9567c059405aeec3604a1aa4ba82b63bcc741 Mon Sep 17 00:00:00 2001 From: Riccardo Montagnin Date: Wed, 3 Nov 2021 04:56:33 +0100 Subject: [PATCH] fix: application link verification flow breaks with unordered IBC channels (#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 --- ### 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) --- ...8aa1680a304e4c14b74d95994a5b6d88fef5d.yaml | 6 + x/profiles/keeper/grpc_query.go | 10 +- x/profiles/keeper/keeper_app_links.go | 12 +- x/profiles/keeper/keeper_app_links_test.go | 14 +- x/profiles/keeper/relay_app_links.go | 30 +++- x/profiles/keeper/relay_app_links_test.go | 129 ++++++++++++++++-- x/profiles/types/models_app_links.go | 7 + 7 files changed, 179 insertions(+), 29 deletions(-) create mode 100644 .changeset/entries/7b2791e321645b1b6ce07eb919f8aa1680a304e4c14b74d95994a5b6d88fef5d.yaml diff --git a/.changeset/entries/7b2791e321645b1b6ce07eb919f8aa1680a304e4c14b74d95994a5b6d88fef5d.yaml b/.changeset/entries/7b2791e321645b1b6ce07eb919f8aa1680a304e4c14b74d95994a5b6d88fef5d.yaml new file mode 100644 index 0000000000..ee87080fad --- /dev/null +++ b/.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 diff --git a/x/profiles/keeper/grpc_query.go b/x/profiles/keeper/grpc_query.go index 35dbaccbad..6a4d6753b4 100644 --- a/x/profiles/keeper/grpc_query.go +++ b/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 } diff --git a/x/profiles/keeper/keeper_app_links.go b/x/profiles/keeper/keeper_app_links.go index 30485bef3d..3fd44674d2 100644 --- a/x/profiles/keeper/keeper_app_links.go +++ b/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, diff --git a/x/profiles/keeper/keeper_app_links_test.go b/x/profiles/keeper/keeper_app_links_test.go index b2c90004c9..c956998a4d 100644 --- a/x/profiles/keeper/keeper_app_links_test.go +++ b/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) + } } }) } diff --git a/x/profiles/keeper/relay_app_links.go b/x/profiles/keeper/relay_app_links.go index 8b1985a006..2267fa7666 100644 --- a/x/profiles/keeper/relay_app_links.go +++ b/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) } diff --git a/x/profiles/keeper/relay_app_links_test.go b/x/profiles/keeper/relay_app_links_test.go index 2041e1f507..df446ecf10 100644 --- a/x/profiles/keeper/relay_app_links_test.go +++ b/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( diff --git a/x/profiles/types/models_app_links.go b/x/profiles/types/models_app_links.go index 3a50ef1295..b9f0994d61 100644 --- a/x/profiles/types/models_app_links.go +++ b/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)