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

Make all handlers emit an IbcEvent with height ctx.host_height() #2043

Merged
merged 25 commits into from
Apr 6, 2022

Conversation

plafer
Copy link
Contributor

@plafer plafer commented Mar 31, 2022

Closes: cosmos/ibc-rs#52

Description

Make all handlers emit an IbcEvent with height ctx.host_height().


PR author checklist:

  • Added changelog entry, using unclog.
  • Added tests: integration (for Hermes) or unit/mock tests (for modules).
  • Linked to GitHub issue.
  • Updated code comments and documentation (e.g., docs/).

Reviewer checklist:

  • Reviewed Files changed in the GitHub PR explorer.
  • Manually tested (in case integration/unit/mock tests are absent).

@plafer plafer changed the title Plafer/2035 handler event height handlers event height fix Apr 1, 2022
@romac romac changed the title handlers event height fix Make all handlers emit an IbcEvent with height ctx.host_height() Apr 4, 2022

for event in handler_output.events.iter() {
assert!(matches!(event, &IbcEvent::CloseInitChannel(_)));
assert_eq!(event.height(), context.host_height());
Copy link
Member

Choose a reason for hiding this comment

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

Cool, thanks for adding mock tests here and for channel close confirm!


if !packet.timeout_height.is_zero() && packet_height <= latest_height {
if !packet.timeout_height.is_zero() && packet.timeout_height <= latest_height {
Copy link
Member

Choose a reason for hiding this comment

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

This change makes sense.

On a related topic, there is a comment above (L60) that is confusing me, specifically:

// check if packet height is newer than the height of the latest client state on the receiving chain

The receiving chain is the local chain (the one hosting & running the current ics04 module). It's not obvious to me what is the connection we meant to make between "latest client state" and "packet height". We should clarify that, or remove the comment if stale.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

It doesn't make sense to me either, so I'll just remove it.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Removed a few comments that were either confusing or didn't say much f8525a6867

@adizere adizere added modules O: code-hygiene Objective: cause to improve code hygiene and removed modules O: code-hygiene Objective: cause to improve code hygiene labels Apr 5, 2022
@romac romac self-assigned this Apr 5, 2022
Copy link
Member

@romac romac left a comment

Choose a reason for hiding this comment

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

Thanks @plafer 🙏

@romac romac merged commit 4ca8298 into master Apr 6, 2022
@romac romac deleted the plafer/2035-handler-event-height branch April 6, 2022 10:37
hu55a1n1 pushed a commit to hu55a1n1/hermes that referenced this pull request Sep 13, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants