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.

Sign up for GitHub

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

Jump to bottom

Generic query, channel query and reduce code #152

Merged
merged 18 commits into from
Jul 20, 2020
Merged

Generic query, channel query and reduce code #152

Show file tree
Hide file tree
Changes from 5 commits
Commits
Show all changes
18 commits
Select commit Hold shift + click to select a range
86fb408
ci README fix and added simple_config
greg-szabo Jul 15, 2020
0882154
Added channel query and TryFrom trait to connection query
greg-szabo Jul 15, 2020
1e32302
Implemented generic query for abci queries, converted connection quer…
greg-szabo Jul 16, 2020
2a88624
Revert "ci README fix and added simple_config" - separated into its o…
greg-szabo Jul 16, 2020
53d8f1a
fmt fix
greg-szabo Jul 16, 2020
85a9439
typo fix
greg-szabo Jul 16, 2020
980d039
Fix for handling empty response value.
adizere Jul 17, 2020
53a4d50
Different output for channel and connection query.
greg-szabo Jul 17, 2020
c71709e
Merge branch 'greg/query-channel-and-reduce-code' of github.com:infor…
greg-szabo Jul 17, 2020
882902c
Refactored Proto types to Raw types
greg-szabo Jul 17, 2020
b584b88
clipyy fix
greg-szabo Jul 17, 2020
7e8dc89
Moved query parameters into the command parameters with Into. (#155)
greg-szabo Jul 17, 2020
35ee937
Implemented better Raw type handling (#156)
greg-szabo Jul 18, 2020
843191f
Renamed trait to TryFromRaw to make it easier to understand its purpose
greg-szabo Jul 18, 2020
1676a52
comment typo fix
greg-szabo Jul 18, 2020
621717c
Comments fix
greg-szabo Jul 20, 2020
8ed3f61
TryFromRaw introduces a function
greg-szabo Jul 20, 2020
988960d
Typo fix
greg-szabo Jul 20, 2020
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
93 changes: 2 additions & 91 deletions modules/src/ics02_client/query.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,15 +1,10 @@
use std::marker::PhantomData;
use tendermint_rpc::endpoint::abci_query::AbciQuery;

use tendermint::abci;

use crate::ics23_commitment::{CommitmentPath, CommitmentProof};

use crate::error;
use crate::ics02_client::state::{ClientState, ConsensusState};
//use crate::ics02_client::state::{ClientState, ConsensusState};
use crate::ics24_host::identifier::ClientId;
use crate::path::{ClientStatePath, ConsensusStatePath, Path};
use crate::query::{IbcQuery, IbcResponse};
use crate::path::{ClientStatePath, ConsensusStatePath};
use crate::Height;

pub struct QueryClientFullState<CLS> {
Expand All @@ -32,29 +27,6 @@ impl<CLS> QueryClientFullState<CLS> {
}
}

impl<CLS> IbcQuery for QueryClientFullState<CLS>
where
CLS: ClientState,
{
type Response = ClientFullStateResponse<CLS>;

fn path(&self) -> abci::Path {
"/store/ibc/key".parse().unwrap()
}

fn height(&self) -> Height {
self.chain_height
}

fn prove(&self) -> bool {
self.prove
}

fn data(&self) -> Vec<u8> {
self.client_state_path.to_key().into()
}
}

pub struct ClientFullStateResponse<CLS> {
pub client_state: CLS,
pub proof: Option<CommitmentProof>,
Expand All @@ -80,27 +52,6 @@ impl<CLS> ClientFullStateResponse<CLS> {
}
}

impl<CLS> IbcResponse<QueryClientFullState<CLS>> for ClientFullStateResponse<CLS>
where
CLS: ClientState,
{
fn from_abci_response(
query: QueryClientFullState<CLS>,
response: AbciQuery,
) -> Result<Self, error::Error> {
Ok(ClientFullStateResponse::new(
query.client_id,
amino_unmarshal_binary_length_prefixed(&response.value)?,
response.proof,
response.height.into(),
))
}
}

fn amino_unmarshal_binary_length_prefixed<T>(_bytes: &[u8]) -> Result<T, error::Error> {
todo!()
}

pub struct QueryClientConsensusState<CS> {
pub chain_height: Height,
pub client_id: ClientId,
Expand Down Expand Up @@ -128,29 +79,6 @@ impl<CS> QueryClientConsensusState<CS> {
}
}

impl<CS> IbcQuery for QueryClientConsensusState<CS>
where
CS: ConsensusState,
{
type Response = ConsensusStateResponse<CS>;

fn path(&self) -> abci::Path {
"/store/ibc/key".parse().unwrap()
}

fn height(&self) -> Height {
self.chain_height
}

fn prove(&self) -> bool {
self.prove
}

fn data(&self) -> Vec<u8> {
self.consensus_state_path.to_key().into()
}
}

pub struct ConsensusStateResponse<CS> {
pub consensus_state: CS,
pub proof: Option<CommitmentProof>,
Expand All @@ -176,20 +104,3 @@ impl<CS> ConsensusStateResponse<CS> {
}
}
}

impl<CS> IbcResponse<QueryClientConsensusState<CS>> for ConsensusStateResponse<CS>
where
CS: ConsensusState,
{
fn from_abci_response(
query: QueryClientConsensusState<CS>,
response: AbciQuery,
) -> Result<Self, error::Error> {
Ok(ConsensusStateResponse::new(
query.client_id,
amino_unmarshal_binary_length_prefixed(&response.value)?,
response.proof,
response.height.into(),
))
}
}
83 changes: 48 additions & 35 deletions modules/src/ics03_connection/connection.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -7,6 +7,7 @@ use serde_derive::{Deserialize, Serialize};
// Import proto declarations.
use ibc_proto::connection::ConnectionEnd as ProtoConnectionEnd;
Copy link
Member

Choose a reason for hiding this comment

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

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?

Copy link
Member Author

Choose a reason for hiding this comment

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

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.

Copy link
Member Author

Choose a reason for hiding this comment

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

Submitted a commit for it.

use ibc_proto::connection::Counterparty as ProtoCounterparty;
use std::convert::TryFrom;

#[derive(Clone, Debug, PartialEq, Serialize, Deserialize)]
pub struct ConnectionEnd {
Expand All @@ -16,43 +17,31 @@ pub struct ConnectionEnd {
versions: Vec<String>,
}

impl ConnectionEnd {
pub fn new(
client_id: ClientId,
counterparty: Counterparty,
versions: Vec<String>,
) -> Result<Self, Error> {
Ok(Self {
state: State::Uninitialized,
client_id,
counterparty,
versions: validate_versions(versions).map_err(|e| Kind::InvalidVersion.context(e))?,
})
}
impl TryFrom<ProtoConnectionEnd> for ConnectionEnd {
type Error = anomaly::Error<Kind>;

pub fn set_state(&mut self, new_state: State) {
self.state = new_state;
}

pub fn from_proto_connection_end(pc: ProtoConnectionEnd) -> Result<Self, Error> {
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 == "" {
Copy link
Member

@adizere adizere Jul 17, 2020 •
edited
Loading

Choose a reason for hiding this comment

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

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

Copy link
Member

@adizere adizere Jul 17, 2020 •
edited
Loading

Choose a reason for hiding this comment

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

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.

Copy link
Member Author

Choose a reason for hiding this comment

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

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.

Copy link
Member

Choose a reason for hiding this comment

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

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.

return Err(Kind::ConnectionNotFound.into());
}

// The Counterparty field is an Option, may be missing.
match pc.counterparty {
match value.counterparty {
Some(cp) => {
let mut conn = ConnectionEnd::new(
pc.client_id
value
.client_id
.parse()
.map_err(|e| Kind::IdentifierError.context(e))?,
Counterparty::from_proto_counterparty(cp)?,
validate_versions(pc.versions).map_err(|e| Kind::InvalidVersion.context(e))?,
Counterparty::try_from(cp)?,
validate_versions(value.versions)
.map_err(|e| Kind::InvalidVersion.context(e))?,
)
.unwrap();

// Set the state.
conn.set_state(State::from_i32(pc.state));
conn.set_state(State::from_i32(value.state));
Ok(conn)
}

Expand All @@ -62,6 +51,25 @@ impl ConnectionEnd {
}
}

impl ConnectionEnd {
pub fn new(
client_id: ClientId,
counterparty: Counterparty,
versions: Vec<String>,
) -> Result<Self, Error> {
Ok(Self {
state: State::Uninitialized,
client_id,
counterparty,
versions: validate_versions(versions).map_err(|e| Kind::InvalidVersion.context(e))?,
})
}

pub fn set_state(&mut self, new_state: State) {
self.state = new_state;
}
}

impl Connection for ConnectionEnd {
type ValidationError = Error;

Expand Down Expand Up @@ -95,6 +103,22 @@ pub struct Counterparty {
prefix: CommitmentPrefix,
}

impl TryFrom<ProtoCounterparty> for Counterparty {
type Error = anomaly::Error<Kind>;

fn try_from(value: ProtoCounterparty) -> Result<Self, Self::Error> {
// Todo: Is validation complete here? (code was moved from `from_proto_counterparty`)
match value.prefix {
Some(prefix) => Counterparty::new(
value.client_id,
value.connection_id,
CommitmentPrefix::new(prefix.key_prefix),
),
None => Err(Kind::MissingCounterpartyPrefix.into()),
}
}
}

impl Counterparty {
pub fn new(
client_id: String,
Expand All @@ -111,17 +135,6 @@ impl Counterparty {
prefix,
})
}

pub fn from_proto_counterparty(pc: ProtoCounterparty) -> Result<Self, Error> {
match pc.prefix {
Some(prefix) => Counterparty::new(
pc.client_id,
pc.connection_id,
CommitmentPrefix::new(prefix.key_prefix),
),
None => Err(Kind::MissingCounterpartyPrefix.into()),
}
}
}

impl ConnectionCounterparty for Counterparty {
Expand Down
1 change: 0 additions & 1 deletion modules/src/ics03_connection/mod.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -5,4 +5,3 @@ pub mod error;
pub mod events;
pub mod exported;
pub mod msgs;
pub mod query;
121 changes: 0 additions & 121 deletions modules/src/ics03_connection/query.rs
View file
Open in desktop

This file was deleted.

Loading