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

Some cleanup (followup to protobuf dedup) #509

Merged
merged 5 commits into from
Jan 19, 2021
Merged

Some cleanup (followup to protobuf dedup) #509

merged 5 commits into from
Jan 19, 2021

Conversation

adizere
Copy link
Member

@adizere adizere commented Jan 8, 2021

Closes: cosmos/ibc-rs#107
Closes: cosmos/ibc-rs#96

Fixes for these two problems:

Deferred issues that will be handled later:

  • tendermint ClientState has new fields proof_specs that is currently ignored. -> will be handled together in Follow-up to migration work #469 (because it depends on re-enabling client tests)

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-io commented Jan 8, 2021

Codecov Report

Merging #509 (0a220d0) into master (b1b37f5) will increase coverage by 27.4%.
The diff coverage is 69.4%.

Impacted file tree graph

@@            Coverage Diff            @@
##           master    informalsystems/hermes#509      +/-   ##
=========================================
+ Coverage    13.6%   41.1%   +27.4%     
=========================================
  Files          69     133      +64     
  Lines        3752    8344    +4592     
  Branches     1374       0    -1374     
=========================================
+ Hits          513    3434    +2921     
- Misses       2618    4910    +2292     
+ Partials      621       0     -621     
Impacted Files Coverage Δ
...application/ics20_fungible_token_transfer/error.rs 0.0% <0.0%> (ø)
...pplication/ics20_fungible_token_transfer/events.rs 0.0% <ø> (ø)
...ion/ics20_fungible_token_transfer/msgs/transfer.rs 0.0% <0.0%> (ø)
modules/src/events.rs 0.0% <0.0%> (ø)
modules/src/ics02_client/error.rs 100.0% <ø> (ø)
modules/src/ics02_client/events.rs 0.0% <0.0%> (ø)
modules/src/ics02_client/raw.rs 0.0% <0.0%> (ø)
modules/src/ics03_connection/error.rs 100.0% <ø> (+66.6%) ⬆️
modules/src/ics03_connection/events.rs 0.0% <0.0%> (ø)
modules/src/ics04_channel/error.rs 100.0% <ø> (+75.0%) ⬆️
... and 228 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 133524a...4115c0c. Read the comment docs.

@adizere adizere marked this pull request as ready for review January 18, 2021 10:58
@adizere
Copy link
Member Author

adizere commented Jan 18, 2021

@cezarad and @vitorenesduarte if you have a chance to look over this, any thoughts would be appreciated!

Comment on lines +265 to +272
// Verify that the error kind matches
if let Some(expected_kind) = test.error_kind {
assert_eq!(
&expected_kind,
e.kind(),
"conn_open_ack: expected error kind mismatches thrown error kind"
)
}
Copy link
Member

Choose a reason for hiding this comment

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

👍

@adizere adizere merged commit bc6b445 into master Jan 19, 2021
@adizere adizere deleted the adi/306_dedupfix branch January 19, 2021 09:04
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 Adi!

hu55a1n1 pushed a commit to hu55a1n1/hermes that referenced this pull request Sep 13, 2022
* MsgConnectionOpenAck test cleanup

* Quick fix for #513.

* Better error msgs & frozen client check.

* UPdated changelog
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.

4 participants