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

Implement ADR 009: ChainEndpoint and ChainHandle methods standardization #2200

Merged
merged 21 commits into from
May 19, 2022

Conversation

plafer
Copy link
Contributor

@plafer plafer commented May 9, 2022

Closes: #2192

Description

I chose not to implement tendermint_proto::Protobuf on the new domain types, as we never need to encode/decode the request itself. We either

  1. (gRPC) convert it to the "raw type" and pass that to tonic, or
  2. (tendermint rpc) call abci_query using fields inside the request

PR author checklist:

  • Added changelog entry, using unclog.
  • Added tests: integration (for Hermes) or unit/mock tests (for modules).
  • Linked to GitHub issue.
  • Updated code comments and documentation (e.g., docs/).

Reviewer checklist:

  • Reviewed Files changed in the GitHub PR explorer.
  • Manually tested (in case integration/unit/mock tests are absent).

@plafer plafer changed the title Plafer/2192 adr009 impl Implement ADR 009 May 9, 2022
@plafer plafer assigned plafer and romac and unassigned plafer May 11, 2022
@plafer plafer marked this pull request as ready for review May 11, 2022 18:27
@plafer
Copy link
Contributor Author

plafer commented May 11, 2022

There are only 2 "proto types" left in query reponses: MerkleProof and PacketState. Should we create a domain type for those too? If so, I'm not sure what they should look like.

@romac
Copy link
Member

romac commented May 16, 2022

There are only 2 "proto types" left in query reponses: MerkleProof and PacketState. Should we create a domain type for those too? If so, I'm not sure what they should look like.

We already have one for MerkleProof which we should use: https://github.com/informalsystems/ibc-rs/blob/39606d28908134a6be940f65830e464a6e44d567/modules/src/core/ics23_commitment/merkle.rs#L39

Let's therefore introduce one for PacketState as well.

@plafer
Copy link
Contributor Author

plafer commented May 17, 2022

I just noticed that the ChainEndpoint::proven_*() methods each have an analogous query_*() method which performs the same query except that they don't return a proof.

I suggest we remove the proven_*() methods from ChainEndpoint and ChainHandle, and change the provable query_*() functions as follows:

// previous
fn query_channel(&self, request: QueryChannelRequest) -> Result<ChannelEnd, Error>;
fn proven_channel(&self, request: QueryChannelRequest) -> Result<(ChannelEnd, MerkleProof), Error>;

// new
fn query_channel(&self, request: QueryChannelRequest, include_proof: PerformQuery) -> Result<(ChannelEnd, Option<MerkleProof>), Error>;

enum PerformQuery {
    WithProof,
    WithoutProof,
};

I introduced PerformQuery to avoid boolean blindness.

Example calls:

chain.query_channel(request, PerformQuery::WithProof);
chain.query_channel(request, PerformQuery::WithoutProof);

An alternative name for PerformQuery I thought of:

enum IncludeProof {
    Yes,
    No,
};
chain.query_channel(request, IncludeProof::Yes);
chain.query_channel(request, IncludeProof::No);

I personally prefer merging the two functions into one, as we expose fewer methods while preserving the same functionality.

@ancazamfir
Copy link
Collaborator

I suggest we remove the proven_*() methods from ChainEndpoint and ChainHandle, and change the provable query_*() functions as follows:

I think it makes sense. A note though that the simple query_ APIs are generally (*) gRPC, the proven_ perform abci_query (RPC to tendermint who does app abci_query). When we first implemented the proven_ APIs there was no support for proofs in gRPC. But now there is. Maybe we should then:

  • (*) change all query_.. for application state to use gRPC (we also have query_.. methods that retrieve blockchain info, i.e. they will stay RPCs to tendermint)
  • change all proven_... to use the gRPC

wrt to query height, if you look at the gRPC request proto defs they do not contain the height at which the query should be done. This is "hidden" in the gRPC metadata. For and example on how this is done with gRPC you can take a look at https://github.com/informalsystems/ibc-rs/blob/c5c89dc82b1466101066e69d1228cda439da1d4b/relayer/src/chain/cosmos.rs#L931-L937

Also I don't know if there is a way to make a gRPC request and specify that there shouldn't be any proof included. If that's the case, we should look at performance differences between abci queries without proof and gRCP (with proof always??), especially if the APIs used by the packet relaying are affected.

@@ -51,6 +51,22 @@ impl From<RawMerkleProof> for MerkleProof {
}
}

impl From<MerkleProof> for RawMerkleProof {
Copy link
Contributor

Choose a reason for hiding this comment

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

Question: What's a RawMerkleProof and when is it appropriate to use it vs. a "regular" MerkleProof?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

The RawMerkleProof is the type that is defined in ibc-go's protobuf files. MerkleProof here is our own domain type for it, and hence we should use it everywhere in our codebase except when it's time to interact with a gRPC endpoint (where we convert our MerkleProof back into a RawMerkleProof to be sent on the wire)

@romac romac changed the title Implement ADR 009 Implement ADR 009: ChainEndpoint and ChainHandle methods standardization May 19, 2022
@romac romac merged commit 6abaa31 into master May 19, 2022
@romac romac deleted the plafer/2192-adr009-impl branch May 19, 2022 13:37
hu55a1n1 pushed a commit to hu55a1n1/hermes that referenced this pull request Sep 13, 2022
…ization (informalsystems#2200)

* QueryChannelClientStateRequest domain type

* QueryChannelClientStateRequest domain type

* QueryChannelRequest domain type

* QueryChannelClientStateRequest: remove unused Protobuf

* clippy

* PageRequest domain type

* QueryClientStatesRequest domain type

* QueryClientStateRequest domain type

* QueryConsensusStatesRequest domain type

* bunch of domain types

* bunch of domain types

* rest of domain types

* changelog

* Update relayer-cli/src/commands/misbehaviour.rs

Co-authored-by: Romain Ruetschi <[email protected]>

* remove extraneous `request` vars

* Use MerkleProof domain type

* Remove `PacketState` return from queries

* Use existing Sequence domain type

Co-authored-by: Romain Ruetschi <[email protected]>
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.

Implement ADR 009: ChainEndpoint and ChainHandle methods standardization
4 participants