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

Consolidate ChainEndpoint/Handle proven queries #2226

Merged
merged 26 commits into from
Jun 6, 2022

Conversation

plafer
Copy link
Contributor

@plafer plafer commented May 19, 2022

Closes: #2223

Remaining:

  • Remove proven_client_consensus()

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 marked this pull request as ready for review May 31, 2022 20:38
Copy link
Member

@romac romac left a comment

Choose a reason for hiding this comment

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

Preemptively approved, thanks a lot for going through this @plafer!

It's a bit unfortunate that we now cannot statically rely on the proof being present in the returned type when IncludeProof is Yes but we can perhaps find a way to recover this statically in a follow-up PR, perhaps with something like:

trait WithProof<Proof> {
    type Proof;

    fn include_proof(&self) -> bool;

    fn build_proof<E>(&self, build: impl FnOnce() -> Result<Proof, E>) -> Result<Self::Proof, E>;
}

struct IncludeProof;
struct NoProof;

impl<Proof> WithProof<Proof> for IncludeProof {
    type Proof = Proof;

    fn include_proof(&self) -> bool { true }

    fn build_proof<E>(&self, build: impl FnOnce() -> Result<Proof, E>) -> Result<Self::Proof, E> {
        build()
    }
}


impl<Proof> WithProof<Proof> for NoProof {
      type Proof = ();

      fn include_proof(&self) -> bool { false }

      fn build_proof<E>(&self, _build: impl FnOnce() -> Result<Proof, E>) -> Result<Self::Proof, E> {
          Ok(())
      }
}

impl ChainEndpoint for CosmosSdkChain {
   fn query_channel<P: WithProof<MerkleProof>>(
         &self,
         request: QueryChannelRequest,
         with_proof: P,
     ) -> Result<(ChannelEnd, P::Proof), Error> {
       // ...

         let res = self.query(
             ChannelEndsPath(request.port_id, request.channel_id),
             request.height,
             with_proof.include_proof(),
         )?;

         let channel_end = ChannelEnd::decode_vec(&res.value).map_err(Error::decode)?;

         let proof = with_proof.build_proof(|| res.proof.ok_or_else(Error::empty_response_proof))?;

        Ok(channel_end, proof)
     }
}

Not clear how that would interact with the chain runtime, but we might be able to solve this in one go once we move the queries out of the runtime and expose them directly from the ChainHandle.

@plafer @soareschen What do you think?

Rust playground

relayer/src/chain/mock.rs Outdated Show resolved Hide resolved
relayer/src/chain/mock.rs Outdated Show resolved Hide resolved
@plafer plafer merged commit b5b165b into master Jun 6, 2022
@plafer plafer deleted the plafer/2223-consolidate-chain-query-proven branch June 6, 2022 17:08
hu55a1n1 pushed a commit to hu55a1n1/hermes that referenced this pull request Sep 13, 2022
* Remove TODO

* query_client_state consolidation

* query_connection consolidation

* query_channel consolidation

* query_packet_commitment function

* query_packet_acknowledgement

* query_packet_receipt

* query_next_sequence_receive

* use request instead of build_packet_proofs

where appropriate

* build_packet_proofs now only returns proofs

* build_packet_proofs no longer uses proven_packet

* proven_packet removed

* query_consensus_state

* remove proven_client_consensus

* cleanup

* docstrings

* changelog

* fix compilation errors

* mock consensus_state query

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.

Consolidate ChainEndpoint::proven_*() functions with their analogous query_*() version
2 participants