Generic query, channel query and reduce code #152

greg-szabo · 2020-07-16T02:09:47Z

Closes: cosmos/ibc-rs#132 - Use TryFrom to map protobuf data structures to our domain types
Closes: cosmos/ibc-rs#131 - Reduce number of query related traits and types
Closes: cosmos/ibc-proto-rs#22 - Using serde for deserializing prost proto structs

Description

Implements a generic query for abci_query for the relayer, removes the now obsolete code.
Implements a basic channel query to show off the query function and the reduced codebase - validation still needs implementing!
Changes the connection and channel queries to use the generic query.
Disables the client query because it needs to be reimplemented using this generic query.

Instead of the built-in TryFrom trait a new TryFromRaw trait was implemented with a try_from(raw) method. This has the same purpose as the built-in TryForm would have but it also allows us to store the RawType (in an associated type) and implement transparent deserialization from the wire, through the compiled proto struct into our domain struct. Much like serde would have done in cosmos/ibc-proto-rs#22.

channel query by id was implemented as an example to see how much code needs to be written. It's not a complete implementation, the validate function needs to be properly implemented. (Just as it has to be expanded on the connection query too.)

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

…y and channel query to use that and disabled client query for now. Removed unused code.

…wn PR This reverts commit 86fb408.

ancazamfir · 2020-07-16T11:19:46Z

relayer/cli/src/commands/query/channel.rs

+        let res = block_on(query::<TendermintChain, ProtoChannel, ChannelEnd>(
            &chain,
-            opts.height,
-            opts.port_id.clone(),
-            opts.channel_id.clone(),
-            opts.proof,
+            Request {
+                path: Some(TendermintPath::from_str(&"store/ibc/key").unwrap()),
+                data: ChannelEndsPath::new(opts.port_id, opts.channel_id)
+                    .to_string()
+                    .into_bytes(),
+                height: Some(Height::from(opts.height)),
+                prove: opts.proof,
+            },


Can we discuss about this as I am not entirely sure about the change here and in similar places. IMO it would still be nice to have a function like query_channel that takes the chain, height, proof and channel key. This function could be called from the CLI or as part of relaying action. The peculiarities of how the query is done depending on the chain type should be in the chain implementation.

Good find. :) This part of the code is admittedly not ready yet, it's an example implementation so we can see the whole code running and to be able to highlight the chain-specific instructions. (This was all over the place generating redundant code.)
As a minimum, the Request struct would need a constructor (::new). Then, the store path should be stored somewhere so we don't hardwire it in each query. Then we should have some kind of helper function so we don't do this horrible .to_string().into_bytes() stuff, which is there because the Request struct is just not optimal. The Request struct should come from the tendermint RPC client, which it can't today because it's not fully public there. (Hence it's a copy, which is also a horrible idea.)

But apart from too much wiring shown here, it has exactly the properties you're looking for with one addition: you can customize it to any query. A query_channel function's real value is that you don't have to remember the path (ChannelEndsPath::new(opts.port_id, opts.channel_id)) but everything else is the same even in another query_connection function. This will be specific to every query, so if we write a query_channel, we'll have to write it for all queries.

I guess it would be nice to see how this works with a more complex query, like with the client. Client already implements helper functions, maybe implementing it using these tools would surface the optimal ratio for helper functions vs generic functions.

To understand why I left this so ugly: I was focusing on the lower part of the query function, which was calling build_response and it had to be implemented for each query separately. Now it's generic enough that it's only implemented once and you don't have to do it for each query.

More notes that I was going to mention on the catchup call: Why this specific implementation?

I wanted to separate the generics of an abci_query from the actual important information, the data. An abci_query is always the same, only its input is different each time. The same with the response: we really only care about the data in it. I think the current code is a good enough example of the response part.

The request part is up for discussion. But I would like to keep it slightly generic so we don't produce code that has to be copy-pasted. You can see that the actual input is generic (the request struct) and all details are encapsulated in it. (As it would be on a "next layer" of a layered protocol.) But we can make it simpler.

Here's an alternative approach where the formatting for the request is kept within the QueryXYZOptions struct by using Into<>. greg/query-channel-and-reduce-code...greg/different-query-input

The point is that the request will always require standardized parameters (within a chain), so having different structs for different queries only make sense if we can standardize their content for the RPC call.

adizere · 2020-07-17T08:12:37Z

modules/src/ics03_connection/connection.rs

-        if pc.id == "" {
+    fn try_from(value: ProtoConnectionEnd) -> Result<Self, Self::Error> {
+        // Todo: Is validation complete here? (Code was moved from `from_proto_connection_end`.)
+        if value.id == "" {


Following the recent changes in the code, this if does not make sense anymore.

I think we can now delete this if statement and remove the todo. The caller of try_from is relayer/relay/src/query.rs:query() and this should figure out that the response value is null and consequently return an error that the response is empty (without even reaching the call to try_from). Relevant code:

https://github.com/informalsystems/ibc-rs/blob/85a94394d7cba7beac6b2b305b3c160ef6ed3ecb/relayer/relay/src/query.rs#L74-L77

I made a commit to capture concretely my suggestion from the comment above. Full commit details here.

Relevant code:

https://github.com/informalsystems/ibc-rs/blob/980d0391e9b2c33f67834a4074c6627046aeb3bd/relayer/relay/src/query.rs#L64-L67

@greg-szabo if this makes sense with what you have in mind, then we can delete the if above. Also, according to clippy, we should use abci_response.value.is_empty() instead of my shady choice of abci_response.value.len() == 0 so this needs fixing as well.

Nice, thanks! I submitted a followup that fixes the Clippy warning.

I'm not sure how much we want to manually check different results of the response before we return, but this case seems straightforward.

There's a TODO in the type validation which I think should go into it's own issue. There's a lot to improve on that piece of code.

Awesome!
That works, we can leave the TODOs for the moment (also in Counterparty), and fix validation, perhaps add also some unit tests in a separate issue.

adizere · 2020-07-17T08:46:45Z

modules/src/ics03_connection/connection.rs

@@ -7,6 +7,7 @@ use serde_derive::{Deserialize, Serialize};
 // Import proto declarations.
 use ibc_proto::connection::ConnectionEnd as ProtoConnectionEnd;


I wonder if we should consider renaming the way we import proto data structures to be as RawConnectionEnd; instead of as ProtoConnectionEnd;. The same would go for RawCounterparty and others. The reason to do this renaming is purely aesthetic and in the interest of consistency with the tendermint-rs codebase (I am referring to the RawCommitSig struct, though I am not sure that the Raw types are analogous). WDYT?

Good idea to keep the same naming schema. Proto was used here for the underlying implementation which you never know. Raw makes more sense. Raw, as in coming straight from the wire.

Submitted a commit for it.

adizere · 2020-07-17T08:58:37Z

relayer/cli/src/commands/query/connection.rs

-            Ok(cs) => status_info!("connection query result: ", "{:?}", cs.connection),
-            Err(e) => status_info!("connection query error", "{}", e),
-        }
+        println!("{:?}", res);


The previous code with the match was slightly better, because it did not expose the whole stacktrace in case of errors, and printed the specific cs.connection object details.

Suggested change

println!("{:?}", res);

match res {

Ok(cs) => status_info!("connection query result: ", "{:?}", cs.connection),

Err(e) => status_info!("connection query error", "{}", e),

}

I thought that thing is just for debugging. The CLI should return a better-formed result. But sure, the result has to be unpacked. I'll submit a commit for it. (and for the same in channel.)

(Sidenote: Please note that the type of res is different than the earlier cs. res is already the data, there's no need to call .connection.)

…malsystems/ibc-rs into greg/query-channel-and-reduce-code

* Moved query parameters into the command parameters with Into.

* Implemented better Raw type handling * Some minor error-handling was fixed

greg-szabo · 2020-07-18T05:35:46Z

Please note that this PR is now ready for review. Summary of work done was updated in the PR description.

adizere · 2020-07-20T09:53:15Z

relayer/cli/src/commands/query/channel.rs

        match res {
-            Ok(cs) => status_info!("channel query result: ", "{:?}", cs.channel),
-            Err(e) => status_info!("channel query error: ", "{:?}", e),
+            Ok(cs) => status_info!("connection query result: ", "{:?}", cs),


Typo? Let's keep "channel" instead of "connection" in these two lines.

The curse of copy-paste

* Added channel query and TryFromRaw trait * Implemented generic query for abci queries, converted connection query and channel query to use that and disabled client query for now. Removed unused code. * Fix for handling empty response value. * Moved query parameters into the command parameters with Into. (informalsystems#155) Co-authored-by: Adi Seredinschi <adi@informal.systems>

greg-szabo added 3 commits July 15, 2020 01:53

ci README fix and added simple_config

86fb408

Added channel query and TryFrom trait to connection query

0882154

Implemented generic query for abci queries, converted connection quer…

1e32302

…y and channel query to use that and disabled client query for now. Removed unused code.

greg-szabo requested review from ancazamfir and romac as code owners July 16, 2020 02:09

greg-szabo added 2 commits July 15, 2020 22:10

Revert "ci README fix and added simple_config" - separated into its o…

2a88624

…wn PR This reverts commit 86fb408.

fmt fix

53d8f1a

ancazamfir reviewed Jul 16, 2020

View reviewed changes

typo fix

85a9439

greg-szabo marked this pull request as draft July 16, 2020 12:34

adizere reviewed Jul 17, 2020

View reviewed changes

Fix for handling empty response value.

980d039

adizere reviewed Jul 17, 2020

View reviewed changes

adizere mentioned this pull request Jul 17, 2020

query channel given channel id and port id cosmos/ibc-rs#130

Closed

greg-szabo added 4 commits July 17, 2020 10:58

Different output for channel and connection query.

53a4d50

Merge branch 'greg/query-channel-and-reduce-code' of github.com:infor…

c71709e

…malsystems/ibc-rs into greg/query-channel-and-reduce-code

Refactored Proto types to Raw types

882902c

clipyy fix

b584b88

greg-szabo mentioned this pull request Jul 17, 2020

Moved query parameters into the command parameters with Into. #155

Merged

6 tasks

greg-szabo added 2 commits July 17, 2020 16:28

Moved query parameters into the command parameters with Into. (#155)

7e8dc89

* Moved query parameters into the command parameters with Into.

Implemented better Raw type handling (#156)

35ee937

* Implemented better Raw type handling * Some minor error-handling was fixed

greg-szabo marked this pull request as ready for review July 18, 2020 05:35

This was referenced Jul 18, 2020

query connections given client id #151

Closed

Use TryFrom to map protobuf data structures to our domain types cosmos/ibc-rs#132

Closed

greg-szabo added 4 commits July 18, 2020 10:25

Renamed trait to TryFromRaw to make it easier to understand its purpose

843191f

comment typo fix

1676a52

Comments fix

621717c

TryFromRaw introduces a function

8ed3f61

adizere reviewed Jul 20, 2020

View reviewed changes

adizere approved these changes Jul 20, 2020

View reviewed changes

Typo fix

988960d

greg-szabo merged commit 91bb40e into master Jul 20, 2020

greg-szabo deleted the greg/query-channel-and-reduce-code branch July 20, 2020 14:14

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Generic query, channel query and reduce code #152

Generic query, channel query and reduce code #152

greg-szabo commented Jul 16, 2020 •

edited

Loading

ancazamfir Jul 16, 2020

greg-szabo Jul 16, 2020

greg-szabo Jul 16, 2020

greg-szabo Jul 16, 2020 •

edited

Loading

greg-szabo Jul 17, 2020

adizere Jul 17, 2020 •

edited

Loading

adizere Jul 17, 2020 •

edited

Loading

greg-szabo Jul 17, 2020

adizere Jul 20, 2020

adizere Jul 17, 2020

greg-szabo Jul 17, 2020

greg-szabo Jul 17, 2020

adizere Jul 17, 2020

greg-szabo Jul 17, 2020

greg-szabo commented Jul 18, 2020

adizere Jul 20, 2020

greg-szabo Jul 20, 2020

		@@ -7,6 +7,7 @@ use serde_derive::{Deserialize, Serialize};
		// Import proto declarations.
		use ibc_proto::connection::ConnectionEnd as ProtoConnectionEnd;

Generic query, channel query and reduce code #152

Generic query, channel query and reduce code #152

Conversation

greg-szabo commented Jul 16, 2020 • edited Loading

Description

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

greg-szabo Jul 16, 2020 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

adizere Jul 17, 2020 • edited Loading

Choose a reason for hiding this comment

adizere Jul 17, 2020 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

greg-szabo commented Jul 18, 2020

Choose a reason for hiding this comment

Choose a reason for hiding this comment

greg-szabo commented Jul 16, 2020 •

edited

Loading

greg-szabo Jul 16, 2020 •

edited

Loading

adizere Jul 17, 2020 •

edited

Loading

adizere Jul 17, 2020 •

edited

Loading