Fix for client query subcommands #235

adizere · 2020-09-16T15:21:06Z

Closes: #231

~~Waiting for #233 to get merged to get this to compile...~~ ready for review!

For contributor use:

Unit tests written
Added test to CI if applicable
Updated CHANGELOG_PENDING.md
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

…t compile yet).

codecov-commenter · 2020-09-17T15:45:38Z

Codecov Report

Merging #235 into master will increase coverage by 23.7%.
The diff coverage is 53.7%.

@@            Coverage Diff            @@
##           master    #235      +/-   ##
=========================================
+ Coverage    13.6%   37.4%   +23.7%     
=========================================
  Files          69      79      +10     
  Lines        3752    5680    +1928     
  Branches     1374    1884     +510     
=========================================
+ Hits          513    2127    +1614     
- Misses       2618    3344     +726     
+ Partials      621     209     -412

Impacted Files	Coverage Δ
modules/src/events.rs	`0.0% <ø> (ø)`
modules/src/ics02_client/events.rs	`0.0% <ø> (ø)`
modules/src/ics02_client/raw.rs	`0.0% <0.0%> (ø)`
modules/src/ics03_connection/error.rs	`9.6% <0.0%> (-23.7%)`	⬇️
modules/src/ics04_channel/error.rs	`23.0% <0.0%> (-2.0%)`	⬇️
modules/src/ics04_channel/packet.rs	`0.0% <0.0%> (ø)`
modules/src/ics07_tendermint/client_def.rs	`0.0% <0.0%> (ø)`
modules/src/ics07_tendermint/error.rs	`30.0% <0.0%> (+30.0%)`	⬆️
modules/src/ics07_tendermint/msgs/update_client.rs	`0.0% <0.0%> (ø)`
modules/src/mock_client/header.rs	`0.0% <0.0%> (ø)`
... and 133 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 9f2f0df...106c459. Read the comment docs.

ancazamfir

Looks great! Added a few comments and questions.

ancazamfir · 2020-09-17T16:22:31Z

modules/src/ics02_client/client_def.rs

+impl TryFromRaw for AnyClientState {
+    type RawType = prost_types::Any;
+    type Error = Error;
+
+    fn try_from(value: Self::RawType) -> Result<Self, Self::Error> {
+        match value.type_url.as_str() {
            "ibc.tendermint.ClientState" => {
-                let raw = RawTendermintClientState::decode(any.value.as_ref())
+                let raw = RawTendermintClientState::decode(value.value.as_ref())


Just curious on why you changed from_any that transforms from a prost type to the try_from for TryFromRaw impl. I liked the old version a bit better.
Also value.value.as_ref() doesn't ready very well :)

It was necessary for AnyClientState to implement the TryFromRaw trait, otherwise the call below would fail:

https://github.com/informalsystems/ibc-rs/blob/106c459cab6cc1a6359759c7bfc2d0d62e004686/relayer-cli/src/commands/query/client.rs#L80-L81

The failure would occur because the function chain.query::<T> requires the trait bound: T: TryFromRaw.

Also value.value.as_ref() doesn't ready very well :)

Agree! Didn't notice that, will fix.

modules/src/ics07_tendermint/consensus_state.rs

modules/src/ics24_host/path.rs

modules/src/ics02_client/client_def.rs

ancazamfir · 2020-09-21T06:57:32Z

Opened issue #245 to track updating the mock consensus state in the proto file.

ancazamfir

Looks great! Thanks Adi!

* Adapted client query subcommands to new query infrastructure (does not compile yet). * Fixes for ClientState -> AnyClientState * Fix for ConsensusState -> AnyConsensusState up to deserialization. * Added impl TryFromRaw for AnyClientState; fixes for Anca's review

adizere added 4 commits September 16, 2020 17:18

Adapted client query subcommands to new query infrastructure (does no…

1efdfde

…t compile yet).

Merge branch 'master' into adi/231_client_query

5a96946

Fixes for ClientState -> AnyClientState

8a82a6e

Fix for ConsensusState -> AnyConsensusState up to deserialization.

106c459

adizere marked this pull request as ready for review September 17, 2020 15:47

adizere requested review from ancazamfir and romac as code owners September 17, 2020 15:47

ancazamfir reviewed Sep 17, 2020

View reviewed changes

adizere added 2 commits September 18, 2020 12:29

Added impl TryFromRaw for AnyClientState; fixes for Anca's review

194b500

Merge branch 'master' into adi/231_client_query

8965bc0

adizere added this to the v0.0.4 milestone Sep 18, 2020

ancazamfir approved these changes Sep 21, 2020

View reviewed changes

adizere merged commit 8b96008 into master Sep 21, 2020

adizere deleted the adi/231_client_query branch September 21, 2020 08:36

adizere removed this from the v0.0.4 milestone Sep 30, 2020

This was referenced Sep 29, 2022

query ClientState given client id cosmos/ibc-proto-rs#21

Closed

query ConsensusState given client id and height cosmos/ibc-rs#129

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fix for client query subcommands #235

Fix for client query subcommands #235

adizere commented Sep 16, 2020 •

edited

Loading

codecov-commenter commented Sep 17, 2020

ancazamfir left a comment

ancazamfir Sep 17, 2020

adizere Sep 18, 2020 •

edited

Loading

ancazamfir commented Sep 21, 2020

ancazamfir left a comment

Fix for client query subcommands #235

Fix for client query subcommands #235

Conversation

adizere commented Sep 16, 2020 • edited Loading

codecov-commenter commented Sep 17, 2020

Codecov Report

ancazamfir left a comment

Choose a reason for hiding this comment

ancazamfir Sep 17, 2020

Choose a reason for hiding this comment

adizere Sep 18, 2020 • edited Loading

Choose a reason for hiding this comment

ancazamfir commented Sep 21, 2020

ancazamfir left a comment

Choose a reason for hiding this comment

adizere commented Sep 16, 2020 •

edited

Loading

adizere Sep 18, 2020 •

edited

Loading