From b83b7c3f929771c9a9989623eb60264c54198263 Mon Sep 17 00:00:00 2001 From: Charly Date: Wed, 2 Nov 2022 18:01:55 +0100 Subject: [PATCH 1/4] rm event emission after context caching --- modules/apps/29-fee/keeper/escrow.go | 12 ------------ modules/core/keeper/msg_server.go | 28 ++++++++++++++++------------ 2 files changed, 16 insertions(+), 24 deletions(-) diff --git a/modules/apps/29-fee/keeper/escrow.go b/modules/apps/29-fee/keeper/escrow.go index 97d29a5c71f..8914279a71d 100644 --- a/modules/apps/29-fee/keeper/escrow.go +++ b/modules/apps/29-fee/keeper/escrow.go @@ -74,9 +74,6 @@ func (k Keeper) DistributePacketFeesOnAcknowledgement(ctx sdk.Context, forwardRe k.distributePacketFeeOnAcknowledgement(cacheCtx, refundAddr, forwardAddr, reverseRelayer, packetFee) } - // NOTE: The context returned by CacheContext() refers to a new EventManager, so it needs to explicitly set events to the original context. - ctx.EventManager().EmitEvents(cacheCtx.EventManager().Events()) - // write the cache writeFn() @@ -130,9 +127,6 @@ func (k Keeper) DistributePacketFeesOnTimeout(ctx sdk.Context, timeoutRelayer sd k.distributePacketFeeOnTimeout(cacheCtx, refundAddr, timeoutRelayer, packetFee) } - // NOTE: The context returned by CacheContext() refers to a new EventManager, so it needs to explicitly set events to the original context. - ctx.EventManager().EmitEvents(cacheCtx.EventManager().Events()) - // write the cache writeFn() @@ -177,9 +171,6 @@ func (k Keeper) distributeFee(ctx sdk.Context, receiver, refundAccAddress sdk.Ac // write the cache writeFn() - - // NOTE: The context returned by CacheContext() refers to a new EventManager, so it needs to explicitly set events to the original context. - ctx.EventManager().EmitEvents(cacheCtx.EventManager().Events()) } // RefundFeesOnChannelClosure will refund all fees associated with the given port and channel identifiers. @@ -228,9 +219,6 @@ func (k Keeper) RefundFeesOnChannelClosure(ctx sdk.Context, portID, channelID st } } - // NOTE: The context returned by CacheContext() refers to a new EventManager, so it needs to explicitly set events to the original context. - ctx.EventManager().EmitEvents(cacheCtx.EventManager().Events()) - // write the cache writeFn() diff --git a/modules/core/keeper/msg_server.go b/modules/core/keeper/msg_server.go index 844621e8ec7..016f5435f84 100644 --- a/modules/core/keeper/msg_server.go +++ b/modules/core/keeper/msg_server.go @@ -395,15 +395,16 @@ func (k Keeper) RecvPacket(goCtx context.Context, msg *channeltypes.MsgRecvPacke cacheCtx, writeFn := ctx.CacheContext() err = k.ChannelKeeper.RecvPacket(cacheCtx, cap, msg.Packet, msg.ProofCommitment, msg.ProofHeight) - // NOTE: The context returned by CacheContext() refers to a new EventManager, so it needs to explicitly set events to the original context. - ctx.EventManager().EmitEvents(cacheCtx.EventManager().Events()) - switch err { case nil: writeFn() case channeltypes.ErrNoOpMsg: + // NOTE: The context returned by CacheContext() refers to a new EventManager, so it needs to explicitly set events to the original context. + ctx.EventManager().EmitEvents(cacheCtx.EventManager().Events()) return &channeltypes.MsgRecvPacketResponse{Result: channeltypes.NOOP}, nil default: + // NOTE: The context returned by CacheContext() refers to a new EventManager, so it needs to explicitly set events to the original context. + ctx.EventManager().EmitEvents(cacheCtx.EventManager().Events()) return nil, sdkerrors.Wrap(err, "receive packet verification failed") } @@ -412,12 +413,13 @@ func (k Keeper) RecvPacket(goCtx context.Context, msg *channeltypes.MsgRecvPacke // Cache context so that we may discard state changes from callback if the acknowledgement is unsuccessful. cacheCtx, writeFn = ctx.CacheContext() ack := cbs.OnRecvPacket(cacheCtx, msg.Packet, relayer) - // NOTE: The context returned by CacheContext() refers to a new EventManager, so it needs to explicitly set events to the original context. - // Events from callback are emitted regardless of acknowledgement success - ctx.EventManager().EmitEvents(cacheCtx.EventManager().Events()) if ack == nil || ack.Success() { // write application state changes for asynchronous and successful acknowledgements writeFn() + } else { + // NOTE: The context returned by CacheContext() refers to a new EventManager, so it needs to explicitly set events to the original context. + // Events from callback are emitted regardless of acknowledgement success + ctx.EventManager().EmitEvents(cacheCtx.EventManager().Events()) } // Set packet acknowledgement only if the acknowledgement is not nil. @@ -473,15 +475,16 @@ func (k Keeper) Timeout(goCtx context.Context, msg *channeltypes.MsgTimeout) (*c cacheCtx, writeFn := ctx.CacheContext() err = k.ChannelKeeper.TimeoutPacket(cacheCtx, msg.Packet, msg.ProofUnreceived, msg.ProofHeight, msg.NextSequenceRecv) - // NOTE: The context returned by CacheContext() refers to a new EventManager, so it needs to explicitly set events to the original context. - ctx.EventManager().EmitEvents(cacheCtx.EventManager().Events()) - switch err { case nil: writeFn() case channeltypes.ErrNoOpMsg: + // NOTE: The context returned by CacheContext() refers to a new EventManager, so it needs to explicitly set events to the original context. + ctx.EventManager().EmitEvents(cacheCtx.EventManager().Events()) return &channeltypes.MsgTimeoutResponse{Result: channeltypes.NOOP}, nil default: + // NOTE: The context returned by CacheContext() refers to a new EventManager, so it needs to explicitly set events to the original context. + ctx.EventManager().EmitEvents(cacheCtx.EventManager().Events()) return nil, sdkerrors.Wrap(err, "timeout packet verification failed") } @@ -541,15 +544,16 @@ func (k Keeper) TimeoutOnClose(goCtx context.Context, msg *channeltypes.MsgTimeo cacheCtx, writeFn := ctx.CacheContext() err = k.ChannelKeeper.TimeoutOnClose(cacheCtx, cap, msg.Packet, msg.ProofUnreceived, msg.ProofClose, msg.ProofHeight, msg.NextSequenceRecv) - // NOTE: The context returned by CacheContext() refers to a new EventManager, so it needs to explicitly set events to the original context. - ctx.EventManager().EmitEvents(cacheCtx.EventManager().Events()) - switch err { case nil: writeFn() case channeltypes.ErrNoOpMsg: + // NOTE: The context returned by CacheContext() refers to a new EventManager, so it needs to explicitly set events to the original context. + ctx.EventManager().EmitEvents(cacheCtx.EventManager().Events()) return &channeltypes.MsgTimeoutOnCloseResponse{Result: channeltypes.NOOP}, nil default: + // NOTE: The context returned by CacheContext() refers to a new EventManager, so it needs to explicitly set events to the original context. + ctx.EventManager().EmitEvents(cacheCtx.EventManager().Events()) return nil, sdkerrors.Wrap(err, "timeout on close packet verification failed") } From d3a14873767182b9e1ac162582c956eb56ee68ef Mon Sep 17 00:00:00 2001 From: Charly Date: Wed, 9 Nov 2022 17:09:46 +0100 Subject: [PATCH 2/4] update event emissions --- modules/core/keeper/msg_server.go | 19 +++++++++---------- 1 file changed, 9 insertions(+), 10 deletions(-) diff --git a/modules/core/keeper/msg_server.go b/modules/core/keeper/msg_server.go index 016f5435f84..9b02e37e606 100644 --- a/modules/core/keeper/msg_server.go +++ b/modules/core/keeper/msg_server.go @@ -400,11 +400,11 @@ func (k Keeper) RecvPacket(goCtx context.Context, msg *channeltypes.MsgRecvPacke writeFn() case channeltypes.ErrNoOpMsg: // NOTE: The context returned by CacheContext() refers to a new EventManager, so it needs to explicitly set events to the original context. - ctx.EventManager().EmitEvents(cacheCtx.EventManager().Events()) + // no-ops do not need event emission as they will be ignored. return &channeltypes.MsgRecvPacketResponse{Result: channeltypes.NOOP}, nil default: // NOTE: The context returned by CacheContext() refers to a new EventManager, so it needs to explicitly set events to the original context. - ctx.EventManager().EmitEvents(cacheCtx.EventManager().Events()) + // no-ops do not need event emission as they will be ignored. return nil, sdkerrors.Wrap(err, "receive packet verification failed") } @@ -418,7 +418,7 @@ func (k Keeper) RecvPacket(goCtx context.Context, msg *channeltypes.MsgRecvPacke writeFn() } else { // NOTE: The context returned by CacheContext() refers to a new EventManager, so it needs to explicitly set events to the original context. - // Events from callback are emitted regardless of acknowledgement success + // Events should still be emitted from failed acks and asynchronous acks ctx.EventManager().EmitEvents(cacheCtx.EventManager().Events()) } @@ -480,11 +480,11 @@ func (k Keeper) Timeout(goCtx context.Context, msg *channeltypes.MsgTimeout) (*c writeFn() case channeltypes.ErrNoOpMsg: // NOTE: The context returned by CacheContext() refers to a new EventManager, so it needs to explicitly set events to the original context. - ctx.EventManager().EmitEvents(cacheCtx.EventManager().Events()) + // no-ops do not need event emission as they will be ignored. return &channeltypes.MsgTimeoutResponse{Result: channeltypes.NOOP}, nil default: // NOTE: The context returned by CacheContext() refers to a new EventManager, so it needs to explicitly set events to the original context. - ctx.EventManager().EmitEvents(cacheCtx.EventManager().Events()) + // no-ops do not need event emission as they will be ignored. return nil, sdkerrors.Wrap(err, "timeout packet verification failed") } @@ -548,12 +548,12 @@ func (k Keeper) TimeoutOnClose(goCtx context.Context, msg *channeltypes.MsgTimeo case nil: writeFn() case channeltypes.ErrNoOpMsg: + // no-ops do not need event emission as they will be ignored. // NOTE: The context returned by CacheContext() refers to a new EventManager, so it needs to explicitly set events to the original context. - ctx.EventManager().EmitEvents(cacheCtx.EventManager().Events()) return &channeltypes.MsgTimeoutOnCloseResponse{Result: channeltypes.NOOP}, nil default: // NOTE: The context returned by CacheContext() refers to a new EventManager, so it needs to explicitly set events to the original context. - ctx.EventManager().EmitEvents(cacheCtx.EventManager().Events()) + // no-ops do not need event emission as they will be ignored. return nil, sdkerrors.Wrap(err, "timeout on close packet verification failed") } @@ -616,15 +616,14 @@ func (k Keeper) Acknowledgement(goCtx context.Context, msg *channeltypes.MsgAckn cacheCtx, writeFn := ctx.CacheContext() err = k.ChannelKeeper.AcknowledgePacket(cacheCtx, cap, msg.Packet, msg.Acknowledgement, msg.ProofAcked, msg.ProofHeight) - // NOTE: The context returned by CacheContext() refers to a new EventManager, so it needs to explicitly set events to the original context. - ctx.EventManager().EmitEvents(cacheCtx.EventManager().Events()) - switch err { case nil: writeFn() case channeltypes.ErrNoOpMsg: + // no-ops do not need event emission as they will be ignored. return &channeltypes.MsgAcknowledgementResponse{Result: channeltypes.NOOP}, nil default: + // no-ops do not need event emission as they will be ignored. return nil, sdkerrors.Wrap(err, "acknowledge packet verification failed") } From 632e0942f60f512466d06e921c847764fc03d851 Mon Sep 17 00:00:00 2001 From: Charly Date: Wed, 9 Nov 2022 17:30:25 +0100 Subject: [PATCH 3/4] rm unnecessary event emission --- modules/apps/27-interchain-accounts/host/keeper/relay.go | 1 - 1 file changed, 1 deletion(-) diff --git a/modules/apps/27-interchain-accounts/host/keeper/relay.go b/modules/apps/27-interchain-accounts/host/keeper/relay.go index b964ce4ca6f..884688384a8 100644 --- a/modules/apps/27-interchain-accounts/host/keeper/relay.go +++ b/modules/apps/27-interchain-accounts/host/keeper/relay.go @@ -74,7 +74,6 @@ func (k Keeper) executeTx(ctx sdk.Context, sourcePort, destPort, destChannel str } // NOTE: The context returned by CacheContext() creates a new EventManager, so events must be correctly propagated back to the current context - ctx.EventManager().EmitEvents(cacheCtx.EventManager().Events()) writeCache() txResponse, err := proto.Marshal(txMsgData) From 591946a0d243a880a780b12e677dcf4b6b0803b5 Mon Sep 17 00:00:00 2001 From: Charly Date: Thu, 10 Nov 2022 12:43:19 +0100 Subject: [PATCH 4/4] fix pr comments --- .../host/keeper/relay.go | 1 - modules/core/keeper/msg_server.go | 18 ++++-------------- 2 files changed, 4 insertions(+), 15 deletions(-) diff --git a/modules/apps/27-interchain-accounts/host/keeper/relay.go b/modules/apps/27-interchain-accounts/host/keeper/relay.go index 884688384a8..4b8c5ff70b4 100644 --- a/modules/apps/27-interchain-accounts/host/keeper/relay.go +++ b/modules/apps/27-interchain-accounts/host/keeper/relay.go @@ -73,7 +73,6 @@ func (k Keeper) executeTx(ctx sdk.Context, sourcePort, destPort, destChannel str txMsgData.MsgResponses[i] = any } - // NOTE: The context returned by CacheContext() creates a new EventManager, so events must be correctly propagated back to the current context writeCache() txResponse, err := proto.Marshal(txMsgData) diff --git a/modules/core/keeper/msg_server.go b/modules/core/keeper/msg_server.go index 9b02e37e606..7c15728a01f 100644 --- a/modules/core/keeper/msg_server.go +++ b/modules/core/keeper/msg_server.go @@ -399,12 +399,9 @@ func (k Keeper) RecvPacket(goCtx context.Context, msg *channeltypes.MsgRecvPacke case nil: writeFn() case channeltypes.ErrNoOpMsg: - // NOTE: The context returned by CacheContext() refers to a new EventManager, so it needs to explicitly set events to the original context. - // no-ops do not need event emission as they will be ignored. + // no-ops do not need event emission as they will be ignored return &channeltypes.MsgRecvPacketResponse{Result: channeltypes.NOOP}, nil default: - // NOTE: The context returned by CacheContext() refers to a new EventManager, so it needs to explicitly set events to the original context. - // no-ops do not need event emission as they will be ignored. return nil, sdkerrors.Wrap(err, "receive packet verification failed") } @@ -479,12 +476,9 @@ func (k Keeper) Timeout(goCtx context.Context, msg *channeltypes.MsgTimeout) (*c case nil: writeFn() case channeltypes.ErrNoOpMsg: - // NOTE: The context returned by CacheContext() refers to a new EventManager, so it needs to explicitly set events to the original context. - // no-ops do not need event emission as they will be ignored. + // no-ops do not need event emission as they will be ignored return &channeltypes.MsgTimeoutResponse{Result: channeltypes.NOOP}, nil default: - // NOTE: The context returned by CacheContext() refers to a new EventManager, so it needs to explicitly set events to the original context. - // no-ops do not need event emission as they will be ignored. return nil, sdkerrors.Wrap(err, "timeout packet verification failed") } @@ -548,12 +542,9 @@ func (k Keeper) TimeoutOnClose(goCtx context.Context, msg *channeltypes.MsgTimeo case nil: writeFn() case channeltypes.ErrNoOpMsg: - // no-ops do not need event emission as they will be ignored. - // NOTE: The context returned by CacheContext() refers to a new EventManager, so it needs to explicitly set events to the original context. + // no-ops do not need event emission as they will be ignored return &channeltypes.MsgTimeoutOnCloseResponse{Result: channeltypes.NOOP}, nil default: - // NOTE: The context returned by CacheContext() refers to a new EventManager, so it needs to explicitly set events to the original context. - // no-ops do not need event emission as they will be ignored. return nil, sdkerrors.Wrap(err, "timeout on close packet verification failed") } @@ -620,10 +611,9 @@ func (k Keeper) Acknowledgement(goCtx context.Context, msg *channeltypes.MsgAckn case nil: writeFn() case channeltypes.ErrNoOpMsg: - // no-ops do not need event emission as they will be ignored. + // no-ops do not need event emission as they will be ignored return &channeltypes.MsgAcknowledgementResponse{Result: channeltypes.NOOP}, nil default: - // no-ops do not need event emission as they will be ignored. return nil, sdkerrors.Wrap(err, "acknowledge packet verification failed") }