fix: emit events for ErrFailedAcknowledgement #7891
Conversation
| @@ -394,23 +395,31 @@ func (k *Keeper) RecvPacket(ctx context.Context, msg *channeltypes.MsgRecvPacket | |||
| } | |||
|
|
|||
| var ack exported.Acknowledgement | |||
| if err := k.BranchService.Execute(ctx, func(ctx context.Context) error { | |||
| var events []sdk.Event | |||
| if err := k.BranchService.Execute(ctx, func(subCtx context.Context) error { | |||
There was a problem hiding this comment.
Maybe we could rename these executionCtx and cacheCtx
There was a problem hiding this comment.
they use both cacheCtx and subCtx throughout the code, but leave ctx as just ctx. i feel like we're ok here
technicallyty
requested review from
AdityaSripal,
gjermundgaraba and
srdtrk
as code owners
| // We lost apis to propagate and err prefix events from a branched multistore ctx | ||
| // ctx.EventManager().EmitEvents(convertToErrorEvents(cacheCtx.EventManager().Events())) | ||
| errEvents := convertToErrorEvents(events) | ||
| for _, e := range errEvents { |
There was a problem hiding this comment.
It's not immediately clear to me why we need to loop through these again here?
There was a problem hiding this comment.
its due to some mismatch between the type sdk.Event and event.Event. I will just change convertToErrorEvents to do this conversion instead
There was a problem hiding this comment.
the event manager in the SDK will convert from event.Event to sdk.Event inside this EmitKV method. so its a bunch of back and forth really
// EmitKV emits a key value pair event.
func (e Events) EmitKV(eventType string, attrs ...event.Attribute) error {
attributes := make([]sdk.Attribute, 0, len(attrs))
for _, attr := range attrs {
attributes = append(attributes, sdk.NewAttribute(attr.Key, attr.Value))
}
e.EventManagerI.EmitEvents(sdk.Events{sdk.NewEvent(eventType, attributes...)})
return nil
}
There was a problem hiding this comment.
acutally it is a bit of a pain to update the tests for this, i think it might be best to just keep this conversion isolated here.. lmk what you think
There was a problem hiding this comment.
I'm OK with that. Maybe put a comment to explain so we keep the context in the code.
There was a problem hiding this comment.
technicallyty
changed the title
|
@gjermundgaraba i added a check ack nil as it was causing tests to panic without it. let me know if this is the correct behavior. |
|
Description
adds a hack that enables emitting error prefixed events when a Failed Acknowledgement occurs in the core module keeper.
closes: n/a
Before we can merge this PR, please make sure that all the following items have been
checked off. If any of the checklist items are not applicable, please leave them but
write a little note why.
docs/).godoccomments.Files changedin the GitHub PR explorer.SonarCloud Reportin the comment section below once CI passes.