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

ClientState implementation with DomainTypes #247

Merged
merged 4 commits into from
Sep 22, 2020

Conversation

greg-szabo
Copy link
Member

@greg-szabo greg-szabo commented Sep 22, 2020

Part of #249. Not the full implementation yet, but this piece of code is ready to be used.

First stab at implementing DomainTypes in IBC.

  • Note the simplified TryFrom implementations in modules/src/ics02_client/client_def.rs
  • Note that the new requirement of implementing the From trait for encoding to protobuf highlights missing pieces of the domain implementation. (It's easy to see and to implement domain logic and it's not hidden inside the encoding trenches.)
  • Note the implementation of the new Chain::query2 function that returns Vec<u8>. The original query should be replaced with this. (Showing it side-by-side for now.) Some chains might not want to use DomainTypes and they can still implement query this way because of the general Vec<u8> return. Unfortunately, the disadvantage is that the decoding needs to be done at a higher level (see the AnyClientState query) but with DomainTypes, it's literally just calling one decode_vec() function.

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

@ancazamfir
Copy link
Collaborator

Looks great!
I am working on a relayer CLI to create a client and the From for AnyClientState, AnyConsensusState and MsgCreateAnyClient to protobuf are all missing. You have implemented the first two which is great.

Here is the flow:

  • we query the latest header (using the light client) and we get a SignedHeader
  • from the SignedHeader we create a tendermint instance of AnyConsensusState (domain type)
  • from the configuration (and a currently missing chain query for unbonding period) we create a tendermint instance of AnyClientState (domain type)
  • then we create the MsgCreateAnyClient (domain type)
  • this should implement the Msg trait, including the get_sign_bytes(), get_signer() and get_type() that are required by a chain to send a message
  • CosmosSDKChain should implement send(msgs: Vec<Msg>)
  • the Msg's get_sign_bytes() impl of the concrete message converts to protobuf and then to Vec. I see that Andy is using prost::Message::encode(&msg, &mut buf) where msg is the protobuf message.

So I think it would be nice to have here also the impl From<MsgCreateAnyClient> for MsgCreateClient. And the implementation of get_sign_bytes(). But we can also do these in a different PR.

Copy link
Member

@adizere adizere left a comment

Choose a reason for hiding this comment

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

Looks neat, I like the DomainTypes idea!

modules/src/ics02_client/client_def.rs Show resolved Hide resolved
modules/src/mock_client/state.rs Show resolved Hide resolved
relayer/Cargo.toml Show resolved Hide resolved
@greg-szabo
Copy link
Member Author

So, my original focus for this PR was to agree on the method and then get rid of the TryFromRaw trait. As discussed today, the implemented structs are so urgent for Anca that we're going to finalize this PR without getting all the other work done. (These structs are done, so this is a complete PR.)

I'm happy to discuss the Msg trait and the structs involved in a new issue. I believe we should be careful creating new traits (especially if they relate to an already existing trait) but I don't yet have the full context to understand everything.

I'll finish up the query code and ask for reviews.

@greg-szabo greg-szabo marked this pull request as ready for review September 22, 2020 16:06
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.

🚀 Thanks Greg!

@greg-szabo greg-szabo merged commit 3e60e67 into master Sep 22, 2020
@greg-szabo greg-szabo deleted the greg/IdentifiedClientState branch September 22, 2020 17:17
hu55a1n1 pushed a commit to hu55a1n1/hermes that referenced this pull request Sep 13, 2022
* ClientState implementation with DomainTypes

* abci_query introduced
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.

3 participants