WIP Connection queries for Relayer #136

adizere · 2020-07-09T08:38:59Z

Closes: #130

Description

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

…files generated but replacing connection mod breaks the build

modules/src/ics03_connection/query.rs

adizere · 2020-07-10T13:07:10Z

Ready for review!

! Before merging:

Because of the code which prost generates (based on the .proto file), we had to disable unused_qualifications warnings here.
The explicit warning originates from the .rs file generated by prost:

  Compiling relayer-modules v0.0.1 (/Users/adi/Hammers/ibc-rs/modules)
error: unnecessary qualification
  --> /Users/adi/Hammers/ibc-rs/target/debug/build/relayer-modules-4a039a021af40008/out/connection.rs:12:13
   |
12 |     pub id: std::string::String,
   |             ^^^^^^^^^^^^^^^^^^^
   |
note: the lint level is defined here
  --> modules/src/lib.rs:9:5
   |
9  |     unused_qualifications,
   |     ^^^^^^^^^^^^^^^^^^^^^

We have two options:

keep the warnings disabled, or
re-enable the warnings and bypass the prost compilation step, i.e., we put in our codebase not the .proto files but the final .rs files, modified by hand to pass the unused_qualifications warning.

Later edit:

The unnecessary qualification issue should be fixed in commit 107cccb

adizere · 2020-07-10T13:25:45Z

modules/src/ics03_connection/exported.rs

@@ -42,6 +42,15 @@ impl State {
            Self::Open => "OPEN",
        }
    }
+
+    pub fn from_i32(nr: i32) -> Self {


@ancazamfir was not sure this is accurate, please double-check.

Looks good.

…uild #136

greg-szabo

I hate to be "that guy" but cherry-picking the proto files for each use-case will become a hassle very quickly. I tried importing them all to see the interconnectedness and in my experience, it's worth keeping proto definitions together.

See the ibc-proto crate for the results or https://github.com/informalsystems/ibc-proto for the source code.

I suggest we use that crate or at least import its findings for a more complete proto experience.

ancazamfir · 2020-07-13T12:20:53Z

modules/src/ics03_connection/connection.rs

+        match pc.counterparty {
+            Some(cp) => {
+                let mut conn = ConnectionEnd::new(
+                    ClientId::from_str(&pc.client_id).unwrap(),


Could we get rid of the unwrap()s? If the user issues a query command and gets back a few pages of traces it does not help. I had this comment earlier but I guess it got lost. Try to query with non-exsiting connection id.

I'd like to make this into a From trait, if possible.

Also I guess this should be verifying all the fields, not just 2? And should we really have an error per field or maybe start with a more general InvalidConnectionEnd error of some kind and include what's missing as a string? Not sure ...

I'd like to make this into a From trait, if possible.

Either option could work, the question is if we want to keep validation inside the from or outside. The current code uses the FromStr trait, with the signature fn from_str(s: &str) -> Result<Self, Self::Err> so we can do validation inside there and return error if validation fails. With From, the sig is fn from(T) -> Self so we'd have to do validation after calling from. Is it generally speaking, or specifically in this case, better to do validation outside of from?

LE: Maybe this helps?

Also I guess this should be verifying all the fields, not just 2?

Agree this deserves a discussion to find a more principled approach.

ancazamfir · 2020-07-13T12:29:22Z

modules/src/ics23_commitment/mod.rs

@@ -32,6 +32,10 @@ impl CommitmentProof {
 */

 #[derive(Clone, Debug, PartialEq, Serialize, Deserialize)]
-pub struct CommitmentPrefix;
+pub struct CommitmentPrefix(Vec<u8>);


Wondering if we should have a formatter for this. The output of the query shows: prefix: CommitmentPrefix([112, 114, 101, 102, 105, 120]) } (decimal), in the genesis and curl it's cHJlZml4 (base64), ascii is "prefix"

Implemented a fmt::Debug trait to properly format the CommitmentPrefix output in this commit

adizere · 2020-07-13T14:38:04Z

I hate to be "that guy" but cherry-picking the proto files for each use-case will become a hassle very quickly. I tried importing them all to see the interconnectedness and in my experience, it's worth keeping proto definitions together.

See the ibc-proto crate for the results or https://github.com/informalsystems/ibc-proto for the source code.

I suggest we use that crate or at least import its findings for a more complete proto experience.

Module ibc_proto::ibc is private [E0603] :( can we have a quick fix for this @greg-szabo ?

greg-szabo · 2020-07-13T14:54:13Z

Why would you need ibc_proto::ibc? It's redundant.
use ibc_proto::connection or similar works. Check the documentation: https://docs.rs/ibc-proto/0.1.0/ibc_proto/

… connection. Doesn't throw exception anymore. Show error message. #130

… value (string instead of vector of bytes). Assumes value is valid utf-8. #136

@gregszabo

* Initial implementation of proto files required for connection. Proto files generated but replacing connection mod breaks the build * Removed reference to MerklePrefix * Implemented logic to proto_unmarshall the connection using prost informalsystems#130 * Trimmed the proto file, fixed commitments. Ready for review. (informalsystems#130) * Fixed a couple of nits * Fix for unused_qualifications h/t @gregszabo @andynog. * Fixing unit testing since CommitPrefix was changed, failed to cargo build informalsystems#136 * Replaced prost compilation with dependency on ibc-proto crate. * Better error propagation in connection query. * Fixed error handling when specifying height (-h) parameter in a query connection. Doesn't throw exception anymore. Show error message. informalsystems#130 * Added validation for versions in ConnectionEnd unmarshalling * Implemented fmt::Debug trait for CommitPrefix to properly display its value (string instead of vector of bytes). Assumes value is valid utf-8. informalsystems#136 Co-authored-by: Andy Nogueira <me@andynogueira.dev>

andynog added 3 commits July 8, 2020 18:08

Initial implementation of proto files required for connection. Proto …

7e6e76a

…files generated but replacing connection mod breaks the build

Removed reference to MerklePrefix

030c4d0

Implemented logic to proto_unmarshall the connection using prost #130

ae792c0

ancazamfir reviewed Jul 10, 2020

View reviewed changes

modules/src/ics03_connection/query.rs Outdated Show resolved Hide resolved

ancazamfir reviewed Jul 10, 2020

View reviewed changes

modules/src/ics03_connection/query.rs Outdated Show resolved Hide resolved

adizere added 2 commits July 10, 2020 14:53

Trimmed the proto file, fixed commitments. Ready for review. (#130)

d332fd6

Fixed a couple of nits

949ad61

adizere marked this pull request as ready for review July 10, 2020 13:01

adizere requested a review from romac as a code owner July 10, 2020 13:01

adizere commented Jul 10, 2020

View reviewed changes

adizere requested review from ancazamfir, ebuchman and greg-szabo July 10, 2020 13:32

adizere and others added 2 commits July 10, 2020 16:48

Fix for unused_qualifications h/t @gregszabo @andynog.

107cccb

Fixing unit testing since CommitPrefix was changed, failed to cargo b…

3aae417

…uild #136

greg-szabo requested changes Jul 11, 2020

View reviewed changes

ancazamfir reviewed Jul 13, 2020

View reviewed changes

adizere self-assigned this Jul 13, 2020

adizere and others added 5 commits July 13, 2020 17:05

Replaced prost compilation with dependency on ibc-proto crate.

9c2f528

Better error propagation in connection query.

7423376

Fixed error handling when specifying height (-h) parameter in a query…

4db544c

… connection. Doesn't throw exception anymore. Show error message. #130

Added validation for versions in ConnectionEnd unmarshalling

a8ed4d5

Implemented fmt::Debug trait for CommitPrefix to properly display its…

f8c0721

… value (string instead of vector of bytes). Assumes value is valid utf-8. #136

greg-szabo approved these changes Jul 14, 2020

View reviewed changes

greg-szabo merged commit fa4cf48 into master Jul 14, 2020

greg-szabo deleted the andy/proto branch July 14, 2020 23:05

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

WIP Connection queries for Relayer #136

WIP Connection queries for Relayer #136

adizere commented Jul 9, 2020 •

edited

Loading

adizere commented Jul 10, 2020 •

edited

Loading

adizere Jul 10, 2020

ancazamfir Jul 14, 2020

greg-szabo left a comment

ancazamfir Jul 13, 2020

greg-szabo Jul 14, 2020

ebuchman Jul 14, 2020

adizere Jul 14, 2020 •

edited

Loading

ancazamfir Jul 13, 2020

andynog Jul 14, 2020

adizere commented Jul 13, 2020

greg-szabo commented Jul 13, 2020

WIP Connection queries for Relayer #136

WIP Connection queries for Relayer #136

Conversation

adizere commented Jul 9, 2020 • edited Loading

Description

adizere commented Jul 10, 2020 • edited Loading

Later edit:

adizere Jul 10, 2020

Choose a reason for hiding this comment

ancazamfir Jul 14, 2020

Choose a reason for hiding this comment

greg-szabo left a comment

Choose a reason for hiding this comment

ancazamfir Jul 13, 2020

Choose a reason for hiding this comment

greg-szabo Jul 14, 2020

Choose a reason for hiding this comment

ebuchman Jul 14, 2020

Choose a reason for hiding this comment

adizere Jul 14, 2020 • edited Loading

Choose a reason for hiding this comment

ancazamfir Jul 13, 2020

Choose a reason for hiding this comment

andynog Jul 14, 2020

Choose a reason for hiding this comment

adizere commented Jul 13, 2020

greg-szabo commented Jul 13, 2020

adizere commented Jul 9, 2020 •

edited

Loading

adizere commented Jul 10, 2020 •

edited

Loading

adizere Jul 14, 2020 •

edited

Loading