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

Jump to bottom

Fix panics in packet queries #554

Merged
merged 5 commits into from
Jan 27, 2021
Merged

Fix panics in packet queries #554

merged 5 commits into from
Jan 27, 2021

Conversation

adizere
Copy link
Member

@adizere adizere commented Jan 26, 2021 •
edited
Loading

Closes: #521

Description

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.

@adizere adizere changed the title Fix for #521. Will fix the unwraps for QueryUnreceivedAcknowledgement… Fix panics in packet queries Jan 26, 2021
@adizere
Added the 'required' annotation to packet queries.
fd9a5f1 
When a free option (such as a chain_id or port_id) in a command
is tagged with the 'required' attribute, this will safeguard against
invoking the command with `default()` values for the corresponding
option.
@codecov-io
Copy link

codecov-io commented Jan 26, 2021 •
edited
Loading

Codecov Report

Merging #554 (ac5a5f9) into master (b1b37f5) will increase coverage by 33.2%.
The diff coverage is 67.4%.

Impacted file tree graph

@@            Coverage Diff            @@
##           master    #554      +/-   ##
=========================================
+ Coverage    13.6%   46.8%   +33.2%     
=========================================
  Files          69     137      +68     
  Lines        3752    9066    +5314     
  Branches     1374       0    -1374     
=========================================
+ Hits          513    4251    +3738     
- Misses       2618    4815    +2197     
+ 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/raw.rs 0.0% <0.0%> (ø)
modules/src/ics03_connection/error.rs 100.0% <ø> (+66.6%) ⬆️
modules/src/ics03_connection/msgs/conn_open_try.rs 86.7% <ø> (ø)
modules/src/ics03_connection/version.rs 97.6% <ø> (ø)
modules/src/ics04_channel/channel.rs 77.2% <ø> (+62.1%) ⬆️
... and 254 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 9d0a7b8...ac5a5f9. Read the comment docs.

@adizere adizere marked this pull request as ready for review January 26, 2021 13:55
ancazamfir
ancazamfir reviewed Jan 27, 2021
View reviewed changes
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! I have a few minor comments on the error strings and ouput, trying to give just the right amount of information, no more/less than needed. Doesn't apply to this PR specifically, but in general I still find the output of our commands hard to read, sometimes it has information that is not helpful, other times not enough and I have a significant contribution here. We should sit down and discuss this for V1.

relayer-cli/src/commands/query/packet.rs Outdated
.ok_or_else(|| "missing destination chain configuration".to_string())?;
.ok_or_else(|| {
format!(
"missing configuration for the given chain ({}) ",
Copy link
Collaborator

Choose a reason for hiding this comment

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

Suggested change
"missing configuration for the given chain ({}) ",
"missing configuration for chain ({}) ",

Copy link
Member Author

Choose a reason for hiding this comment

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

In parallel with your comments here, I had similar thoughts with respect to these error messages and fixed (hopefully all of them).

The approach I used is

https://github.com/informalsystems/ibc-rs/blob/ffefc34c148bd9e56874ffa9250242a438012716/relayer-cli/src/commands/query/channel.rs#L56

See #559

Copy link
Member Author

Choose a reason for hiding this comment

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

Also, for this file specifically relayer-cli/src/commands/query/packet.rs, I agree with your suggestions and applied them bac733d.

relayer-cli/src/commands/query/packet.rs Outdated Show resolved Hide resolved
relayer-cli/src/commands/query/packet.rs Outdated Show resolved Hide resolved
relayer-cli/src/commands/query/packet.rs Outdated Show resolved Hide resolved
relayer-cli/src/commands/query/packet.rs Outdated Show resolved Hide resolved
relayer-cli/src/commands/query/packet.rs Outdated Show resolved Hide resolved
relayer-cli/src/commands/query/packet.rs Outdated Show resolved Hide resolved
relayer-cli/src/commands/query/packet.rs Outdated Show resolved Hide resolved
relayer-cli/src/commands/query/packet.rs Outdated Show resolved Hide resolved
relayer-cli/src/commands/query/packet.rs Outdated Show resolved Hide resolved
adizere and others added 2 commits January 27, 2021 10:33
@adizere @ancazamfir
Apply suggestions from code review
bac733d 
Co-authored-by: Anca Zamfir <ancazamfir@users.noreply.github.com>
@adizere
FMT fix
ac5a5f9
@adizere adizere merged commit 00570dd into master Jan 27, 2021
@adizere adizere deleted the adi/521_query_panic branch January 27, 2021 09:56
hu55a1n1 pushed a commit to hu55a1n1/hermes that referenced this pull request Sep 13, 2022
@adizere @ancazamfir
Fix panics in packet queries (informalsystems#554)
0fb1c23 
* Fix for informalsystems#521. Will fix the unwraps for QueryUnreceivedAcknowledgementCmd also

* Added the 'required' annotation to packet queries.

When a free option (such as a chain_id or port_id) in a command
is tagged with the 'required' attribute, this will safeguard against
invoking the command with `default()` values for the corresponding
option.

* Updated the changelog

* Apply suggestions from code review

Co-authored-by: Anca Zamfir <ancazamfir@users.noreply.github.com>

* FMT fix

Co-authored-by: Anca Zamfir <ancazamfir@users.noreply.github.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@ancazamfir ancazamfir ancazamfir approved these changes

@romac romac Awaiting requested review from romac

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

query packet unreceived-packets panics if relayer hasn't performed channel handshake yet
3 participants
@adizere @codecov-io @ancazamfir