Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

fix: emit events for ErrFailedAcknowledgement #7891

Open
wants to merge 5 commits into
base: gjermund/use-branch-service-in-recv-packet
Choose a base branch
from
Open

fix: emit events for ErrFailedAcknowledgement #7891

Changes from 1 commit
Commits
Show all changes
5 commits
Select commit Hold shift + click to select a range
9b1426a
hack event
technicallyty Jan 27, 2025
d3146d3
add comment on event stuff
technicallyty Jan 30, 2025
6c23a79
check ack nil
technicallyty Jan 30, 2025
327b41a
check the error in event manager
technicallyty Jan 30, 2025
d33a395
import alias to prevent shadowing
technicallyty Jan 30, 2025
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
23 changes: 16 additions & 7 deletions modules/core/keeper/msg_server.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -4,6 +4,7 @@
"context"
"errors"

"cosmossdk.io/core/event"
errorsmod "cosmossdk.io/errors"

sdk "github.com/cosmos/cosmos-sdk/types"
Expand Down Expand Up @@ -394,23 +395,31 @@
}

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 {
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Maybe we could rename these executionCtx and cacheCtx

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

they use both cacheCtx and subCtx throughout the code, but leave ctx as just ctx. i feel like we're ok here

// Perform application logic callback
ack = cbs.OnRecvPacket(ctx, channelVersion, msg.Packet, relayer)
ack = cbs.OnRecvPacket(subCtx, channelVersion, msg.Packet, relayer)
if !ack.Success() {
// we must return an error here so that false positive events are not emitted
events = sdk.UnwrapSDKContext(subCtx).EventManager().Events()
return channeltypes.ErrFailedAcknowledgement
}

// write application state changes for asynchronous and successful acknowledgements
return nil
}); err != nil {
if errors.Is(err, channeltypes.ErrFailedAcknowledgement) {
// k.EventService.EventManager(ctx).EmitKV()
// Modify events in cached context to reflect unsuccessful acknowledgement

// 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 {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It's not immediately clear to me why we need to loop through these again here?

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

its due to some mismatch between the type sdk.Event and event.Event. I will just change convertToErrorEvents to do this conversion instead

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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
}

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm OK with that. Maybe put a comment to explain so we keep the context in the code.

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

add comment on event stuff

var attrs []event.Attribute
for _, attr := range e.Attributes {
attrs = append(attrs, event.Attribute{
Key: attr.Key,
Value: attr.Value,
})
}
k.EventService.EventManager(ctx).EmitKV(e.Type, attrs...)

Check failure on line 421 in modules/core/keeper/msg_server.go

View workflow job for this annotation

GitHub Actions / lint 

Error return value of `(cosmossdk.io/core/event.Manager).EmitKV` is not checked (errcheck)

Check failure on line 421 in modules/core/keeper/msg_server.go

View workflow job for this annotation

GitHub Actions / lint 

Error return value of `(cosmossdk.io/core/event.Manager).EmitKV` is not checked (errcheck)
}
}
}

Expand Down
Loading