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

Explicit assumptions in packet_from_tx_search_response #617

Merged
merged 4 commits into from
Feb 8, 2021

Conversation

vitorenesduarte
Copy link
Contributor

@vitorenesduarte vitorenesduarte commented Feb 6, 2021

Closes: #XXX

Description

This PR makes the following two assumptions from packet_from_tx_search_response explicit:

  • the number of returned transactions is at most 1
  • if a transaction is returned, then there's exactly 1 transaction event matching the packet query

To make the second assumption explicit, it's required that we iterate all transactions events, which may come with a performance cost. To alleviate this, we're now only trying to parse channel events with ChannelEvents::try_from_tx(&e) instead of the more general from_tx_response_event(e).

This is a follow-up to #613.


For contributor use:

  • Updated the Unreleased section of CHANGELOG.md with the issue.
  • If applicable: Unit tests written, added test to CI.
  • Linked to Github issue with discussion and accepted design OR link to spec that describes this work.
  • Updated relevant documentation (docs/) and code comments.
  • Re-reviewed Files changed in the Github PR explorer.

@codecov-io
Copy link

Codecov Report

Merging #617 (3e95014) into master (b1b37f5) will increase coverage by 30.8%.
The diff coverage is n/a.

Impacted file tree graph

@@            Coverage Diff            @@
##           master    #617      +/-   ##
=========================================
+ Coverage    13.6%   44.5%   +30.8%     
=========================================
  Files          69     144      +75     
  Lines        3752    9550    +5798     
  Branches     1374       0    -1374     
=========================================
+ Hits          513    4255    +3742     
- Misses       2618    5295    +2677     
+ Partials      621       0     -621     
Impacted Files Coverage Δ
modules/src/address.rs 100.0% <ø> (ø)
...application/ics20_fungible_token_transfer/error.rs 0.0% <ø> (ø)
...pplication/ics20_fungible_token_transfer/events.rs 0.0% <ø> (ø)
...ion/ics20_fungible_token_transfer/msgs/transfer.rs 0.0% <ø> (ø)
modules/src/events.rs 0.0% <ø> (ø)
modules/src/handler.rs 100.0% <ø> (ø)
modules/src/ics02_client/client_def.rs 48.3% <ø> (ø)
modules/src/ics02_client/client_type.rs 79.1% <ø> (+31.5%) ⬆️
modules/src/ics02_client/context.rs 100.0% <ø> (ø)
modules/src/ics02_client/error.rs 100.0% <ø> (ø)
... and 246 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update d3d8a69...3e95014. Read the comment docs.

Comment on lines 867 to 871
assert_eq!(
e.type_str,
request.event_id.as_str(),
"packet_from_tx_search_response: unexpected event type"
);
Copy link
Collaborator

Choose a reason for hiding this comment

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

This doesn't seem right. It is true that there is one single transaction but it may include multiple events. I think what was there before was correct:

Suggested change
assert_eq!(
e.type_str,
request.event_id.as_str(),
"packet_from_tx_search_response: unexpected event type"
);
if e.type_str != request.event_id.as_str() {
continue;
}

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Thanks! This was the reason why the tests were failing. Fixed in b0a9aa0.

Copy link
Collaborator

@ancazamfir ancazamfir left a comment

Choose a reason for hiding this comment

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

Looks good, thanks Vitor!

@ancazamfir ancazamfir merged commit 277426e into master Feb 8, 2021
@ancazamfir ancazamfir deleted the vitor/explicit-assumption branch February 8, 2021 10:51
hu55a1n1 pushed a commit to hu55a1n1/hermes that referenced this pull request Sep 13, 2022
…ms#617)

* Explicit assumption in packet_from_tx_search_response
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