From 735e2dba763c9c940bdb261ec9074e5365d37d2a Mon Sep 17 00:00:00 2001 From: Damian Nolan Date: Wed, 13 Oct 2021 16:34:55 +0200 Subject: [PATCH 1/3] cleaning up AuthenticateTx and executeTx to reduce unnecessary complexity --- .../27-interchain-accounts/keeper/relay.go | 48 +++++++------------ .../27-interchain-accounts/types/packet.go | 1 - 2 files changed, 17 insertions(+), 32 deletions(-) diff --git a/modules/apps/27-interchain-accounts/keeper/relay.go b/modules/apps/27-interchain-accounts/keeper/relay.go index 446f1521837..8d5f5b40727 100644 --- a/modules/apps/27-interchain-accounts/keeper/relay.go +++ b/modules/apps/27-interchain-accounts/keeper/relay.go @@ -99,26 +99,19 @@ func (k Keeper) DeserializeCosmosTx(_ sdk.Context, data []byte) ([]sdk.Msg, erro return msgs, nil } -func (k Keeper) AuthenticateTx(ctx sdk.Context, msgs []sdk.Msg, portId string) error { - seen := map[string]bool{} - var signers []sdk.AccAddress - for _, msg := range msgs { - for _, addr := range msg.GetSigners() { - if !seen[addr.String()] { - signers = append(signers, addr) - seen[addr.String()] = true - } - } - } - - interchainAccountAddr, found := k.GetInterchainAccountAddress(ctx, portId) +// AuthenticateTx ensures the provided msgs contain the correct interchain account signer address retrieved +// from state using the provided controller port identifier +func (k Keeper) AuthenticateTx(ctx sdk.Context, msgs []sdk.Msg, portID string) error { + interchainAccountAddr, found := k.GetInterchainAccountAddress(ctx, portID) if !found { return sdkerrors.ErrUnauthorized } - for _, signer := range signers { - if interchainAccountAddr != signer.String() { - return sdkerrors.ErrUnauthorized + for _, msg := range msgs { + for _, signer := range msg.GetSigners() { + if interchainAccountAddr != signer.String() { + return sdkerrors.ErrUnauthorized + } } } @@ -126,33 +119,26 @@ func (k Keeper) AuthenticateTx(ctx sdk.Context, msgs []sdk.Msg, portId string) e } func (k Keeper) executeTx(ctx sdk.Context, sourcePort, destPort, destChannel string, msgs []sdk.Msg) error { - err := k.AuthenticateTx(ctx, msgs, sourcePort) - if err != nil { + if err := k.AuthenticateTx(ctx, msgs, sourcePort); err != nil { return err } for _, msg := range msgs { - err := msg.ValidateBasic() - if err != nil { + if err := msg.ValidateBasic(); err != nil { return err } } - cacheContext, writeFn := ctx.CacheContext() + // CacheContext returns a new context with the multi-store branched into a cached storage object + // writeCache is called only if all msgs succeed, performing state transitions atomically + cacheCtx, writeCache := ctx.CacheContext() for _, msg := range msgs { - _, msgErr := k.executeMsg(cacheContext, msg) - if msgErr != nil { - err = msgErr - break + if _, err := k.executeMsg(cacheCtx, msg); err != nil { + return err } } - if err != nil { - return err - } - - // Write the state transitions if all handlers succeed. - writeFn() + writeCache() return nil } diff --git a/modules/apps/27-interchain-accounts/types/packet.go b/modules/apps/27-interchain-accounts/types/packet.go index 8342f911bbf..3c5223f4390 100644 --- a/modules/apps/27-interchain-accounts/types/packet.go +++ b/modules/apps/27-interchain-accounts/types/packet.go @@ -22,7 +22,6 @@ func (iapd InterchainAccountPacketData) ValidateBasic() error { if len(iapd.Memo) > MaxMemoCharLength { return sdkerrors.Wrapf(ErrInvalidOutgoingData, "packet data memo cannot be greater than %d characters", MaxMemoCharLength) } - // TODO: add type validation when data type enum supports unspecified type return nil } From 8e5664e39b8b19c78743746003583a762d53e1f7 Mon Sep 17 00:00:00 2001 From: Damian Nolan Date: Wed, 3 Nov 2021 11:31:15 +0100 Subject: [PATCH 2/3] adding error wrapping to AuthenticateTx --- modules/apps/27-interchain-accounts/keeper/relay.go | 7 +++---- 1 file changed, 3 insertions(+), 4 deletions(-) diff --git a/modules/apps/27-interchain-accounts/keeper/relay.go b/modules/apps/27-interchain-accounts/keeper/relay.go index 4b8102fa9bb..0f361f8c1c1 100644 --- a/modules/apps/27-interchain-accounts/keeper/relay.go +++ b/modules/apps/27-interchain-accounts/keeper/relay.go @@ -80,13 +80,13 @@ func (k Keeper) createOutgoingPacket( func (k Keeper) AuthenticateTx(ctx sdk.Context, msgs []sdk.Msg, portID string) error { interchainAccountAddr, found := k.GetInterchainAccountAddress(ctx, portID) if !found { - return sdkerrors.ErrUnauthorized + return sdkerrors.Wrapf(types.ErrInterchainAccountNotFound, "failed to retrieve interchain account on port %s", portID) } for _, msg := range msgs { for _, signer := range msg.GetSigners() { if interchainAccountAddr != signer.String() { - return sdkerrors.ErrUnauthorized + return sdkerrors.Wrap(sdkerrors.ErrUnauthorized, "failed to authenticate interchain account transaction") } } } @@ -143,8 +143,7 @@ func (k Keeper) OnRecvPacket(ctx sdk.Context, packet channeltypes.Packet) error return err } - err = k.executeTx(ctx, packet.SourcePort, packet.DestinationPort, packet.DestinationChannel, msgs) - if err != nil { + if err = k.executeTx(ctx, packet.SourcePort, packet.DestinationPort, packet.DestinationChannel, msgs); err != nil { return err } From 71d050e321af303708391072bd4483051928f9ce Mon Sep 17 00:00:00 2001 From: Damian Nolan Date: Wed, 3 Nov 2021 16:53:49 +0100 Subject: [PATCH 3/3] updating err msg to include expected signer --- modules/apps/27-interchain-accounts/keeper/relay.go | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/modules/apps/27-interchain-accounts/keeper/relay.go b/modules/apps/27-interchain-accounts/keeper/relay.go index 0f361f8c1c1..b8aa237a1f0 100644 --- a/modules/apps/27-interchain-accounts/keeper/relay.go +++ b/modules/apps/27-interchain-accounts/keeper/relay.go @@ -86,7 +86,7 @@ func (k Keeper) AuthenticateTx(ctx sdk.Context, msgs []sdk.Msg, portID string) e for _, msg := range msgs { for _, signer := range msg.GetSigners() { if interchainAccountAddr != signer.String() { - return sdkerrors.Wrap(sdkerrors.ErrUnauthorized, "failed to authenticate interchain account transaction") + return sdkerrors.Wrapf(sdkerrors.ErrUnauthorized, "unexpected signer address: expected %s, got %s", interchainAccountAddr, signer.String()) } } }