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

Generic query, channel query and reduce code #152

Merged
merged 18 commits into from
Jul 20, 2020
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
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
3 changes: 3 additions & 0 deletions modules/src/error.rs
Original file line number Diff line number Diff line change
Expand Up @@ -16,6 +16,9 @@ pub enum Kind {
/// Response parsing error
#[error("Could not parse/unmarshall response")]
ResponseParsing,

#[error("Empty response value")]
EmptyResponseValue,
}

impl Kind {
Expand Down
93 changes: 2 additions & 91 deletions modules/src/ics02_client/query.rs
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(),
))
}
}
90 changes: 52 additions & 38 deletions modules/src/ics03_connection/connection.rs
Original file line number Diff line number Diff line change
Expand Up @@ -2,11 +2,13 @@ use super::exported::*;
use crate::ics03_connection::error::{Error, Kind};
use crate::ics23_commitment::CommitmentPrefix;
use crate::ics24_host::identifier::{ClientId, ConnectionId};
use crate::try_from_raw::TryFromRaw;
use serde_derive::{Deserialize, Serialize};

// Import proto declarations.
use ibc_proto::connection::ConnectionEnd as ProtoConnectionEnd;
use ibc_proto::connection::Counterparty as ProtoCounterparty;
use ibc_proto::connection::ConnectionEnd as RawConnectionEnd;
use ibc_proto::connection::Counterparty as RawCounterparty;
use std::convert::TryFrom;

#[derive(Clone, Debug, PartialEq, Serialize, Deserialize)]
pub struct ConnectionEnd {
Expand All @@ -16,43 +18,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))?,
})
}

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 == "" {
impl TryFromRaw for ConnectionEnd {
type RawType = RawConnectionEnd;
type Error = anomaly::Error<Kind>;
fn try_from(value: RawConnectionEnd) -> 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

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

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 +52,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 +104,22 @@ pub struct Counterparty {
prefix: CommitmentPrefix,
}

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

fn try_from(value: RawCounterparty) -> 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 +136,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
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;
Loading