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.

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

explicitly propagate events from cached contexts to original context in delaymsg endblocker #393

Merged
merged 5 commits into from
Sep 29, 2023

Conversation

tqin7
Copy link
Contributor

@tqin7 tqin7 commented Sep 27, 2023

Context:
Couldn't find bank transfer events when a bridge is completed.
x/gov explicitly propagates events from cached contexts (see here)

Testing:
Ran localnet

  1. Before this change, can't find the bank transfer events in end_block_events in /block_results response
  2. After this change, can find the bridge bank transfer events in end_block_events in /block_results response
    example:
    {"type":"transfer","attributes":[{"key":"recipient","value":"dydx1zg69v7yszg69v7yszg69v7yszg69v7ysuz0wqr","index":true},{"key":"sender","value":"dydx1zlefkpe3g0vvm9a4h0jf9000lmqutlh9jwjnsv","index":true},{"key":"amount","value":"123dv4tnt","index":true}]}
    

@linear
Copy link

linear bot commented Sep 27, 2023

@teddyding
Copy link
Contributor

I had question about events being correctly emitted in this PR (cc @clemire ). It looks like the test we added here was not sufficient to catch this?

Can we add a test that correctly verifies events in delayed messages emitted? (we should verify it fails without the fix in this PR)

}); err != nil {
k.Logger(ctx).Error("failed to execute delayed message with id %v: %v", id, err)
}
}

// Propagate events emitted in cached contexts to current context.
ctx.EventManager().EmitEvents(events)
Copy link
Contributor

@teddyding teddyding Sep 27, 2023

Choose a reason for hiding this comment

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

We should wrap this event emitting logic in RunCached util, if possible. Does the axelar example util work?

Copy link
Contributor

@clemire clemire Sep 27, 2023

Choose a reason for hiding this comment

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

Can you double-check that such a change doesn't cause events to be emitted twice? I think the writeCache method does emit events, so I'm wondering if the returned events for this transfer did not get added to the cachedCtx event manager, and that's why they were missing. In that case, handling this from RunCached may not be the solution.

Choose a reason for hiding this comment

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

There were a lot of issues due to sdk message handler creating new EventManager. The emitted events could be picked up from the result like in: cosmos/cosmos-sdk#13389
or just propagate events like axelar did and @teddyding is suggesting.I believe Axelar as well had issues with losing events with their initial RunCached implementation: axelarnetwork/axelar-core#1439.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

When I emit events in RunCached like axelar example, they don't show up any more. The reason is that message handlers create a new event manager and thus the events aren't present in ctx.EventManager() in this line (The cosmos issue that @dalmirel linked also mentioned the same issue)

I think for x/delaymsg's use case, we have to emit events that are part of handler response

@tqin7
Copy link
Contributor Author

tqin7 commented Sep 28, 2023

I had question about events being correctly emitted in this PR (cc @clemire ). It looks like the test we added here was not sufficient to catch this?

Can we add a test that correctly verifies events in delayed messages emitted? (we should verify it fails without the fix in this PR)

added a test case in second commit that fails without the fix!

@tqin7
Copy link
Contributor Author

tqin7 commented Sep 28, 2023

As a separate ticket, I think we should also add this line to our RunCached implementation

@tqin7 tqin7 merged commit 46011c0 into main Sep 29, 2023
10 checks passed
@tqin7 tqin7 deleted the tq/core-608 branch September 29, 2023 19:05
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Development

Successfully merging this pull request may close these issues.

4 participants