Skip to content

Commit

Permalink
Some cleanup (followup to protobuf dedup) (#509)
Browse files Browse the repository at this point in the history 
* MsgConnectionOpenAck test cleanup

* Quick fix for #513.

* Better error msgs & frozen client check.

* UPdated changelog
  • Loading branch information
@adizere
adizere authored Jan 19, 2021
1 parent 133524a commit bc6b445
Show file tree
Hide file tree
Showing 8 changed files with 145 additions and 107 deletions.
10 changes: 9 additions & 1 deletion CHANGELOG.md
View file
Original file line number Diff line number Diff line change
Expand Up @@ -21,16 +21,24 @@
- Fix for mismatching `bitcoin` dep ([#525])

- [modules]
- Clean the validate_basic method ([#94])
- Clean the validate_basic method ([#94])
- MsgConnectionOpenAck testing improvements ([#306])

- Update to `tendermint-rs` v0.17.1 ([#517])

### BUG FIXES:

- [modules]
- Fix for storing ClientType upon 'create-client' ([#513])

[#94]: https://github.com/informalsystems/ibc-rs/issues/94
[#306]: https://github.com/informalsystems/ibc-rs/issues/306
[#470]: https://github.com/informalsystems/ibc-rs/issues/470
[#500]: https://github.com/informalsystems/ibc-rs/issues/500
[#505]: https://github.com/informalsystems/ibc-rs/issues/505
[#507]: https://github.com/informalsystems/ibc-rs/issues/507
[#511]: https://github.com/informalsystems/ibc-rs/pull/511
[#513]: https://github.com/informalsystems/ibc-rs/issues/513
[#514]: https://github.com/informalsystems/ibc-rs/issues/514
[#517]: https://github.com/informalsystems/ibc-rs/issues/517
[#525]: https://github.com/informalsystems/ibc-rs/issues/525
Expand Down
16 changes: 10 additions & 6 deletions modules/src/ics02_client/context.rs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -19,19 +19,14 @@ pub trait ClientReader {

/// Defines the write-only part of ICS2 (client functions) context.
pub trait ClientKeeper {
fn store_client_type(
&mut self,
client_id: ClientId,
client_type: ClientType,
) -> Result<(), Error>;

fn store_client_result(
&mut self,
handler_res: ClientResult,
) -> Result<Vec<ClientEvent>, Error> {
match handler_res {
Create(res) => {
let client_id = self.next_client_id();
self.store_client_type(client_id.clone(), res.client_type)?;
self.store_client_state(client_id.clone(), res.client_state.clone())?;
self.store_consensus_state(
client_id.clone(),
Expand All @@ -54,12 +49,21 @@ pub trait ClientKeeper {

fn next_client_id(&mut self) -> ClientId;

/// Called upon successful client creation
fn store_client_type(
&mut self,
client_id: ClientId,
client_type: ClientType,
) -> Result<(), Error>;

/// Called upon successful client creation and update
fn store_client_state(
&mut self,
client_id: ClientId,
client_state: AnyClientState,
) -> Result<(), Error>;

/// Called upon successful client creation and update
fn store_consensus_state(
&mut self,
client_id: ClientId,
Expand Down
5 changes: 5 additions & 0 deletions modules/src/ics03_connection/connection.rs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -109,6 +109,11 @@ impl ConnectionEnd {
self.state = new_state;
}

/// Setter for the `counterparty` field.
pub fn set_counterparty(&mut self, new_cparty: Counterparty) {
self.counterparty = new_cparty;
}

/// Setter for the `version` field.
/// TODO: A ConnectionEnd should only store one version.
pub fn set_version(&mut self, new_version: Version) {
Expand Down
14 changes: 7 additions & 7 deletions modules/src/ics03_connection/error.rs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -6,7 +6,7 @@ pub type Error = anomaly::Error<Kind>;
use crate::ics24_host::identifier::{ClientId, ConnectionId};
use crate::Height;

#[derive(Clone, Debug, Error)]
#[derive(Clone, Debug, Error, Eq, PartialEq)]
pub enum Kind {
#[error("connection state unknown")]
InvalidState(i32),
Expand Down Expand Up @@ -74,23 +74,23 @@ pub enum Kind {
#[error("client proof must be present")]
NullClientProof,

#[error("the client is frozen")]
FrozenClient,
#[error("the client running locally is frozen")]
FrozenClient(ClientId),

#[error("the connection proof verification failed")]
ConnectionVerificationFailure,

#[error("the expected consensus state could not be retrieved")]
MissingClientConsensusState,
#[error("the consensus state at height {0} for client id {1} could not be retrieved")]
MissingClientConsensusState(Height, ClientId),

#[error("the local consensus state could not be retrieved")]
MissingLocalConsensusState,

#[error("the consensus proof verification failed (height: {0})")]
ConsensusStateVerificationFailure(Height),

#[error("the client state proof verification failed")]
ClientStateVerificationFailure,
#[error("the client state proof verification failed for client id: {0}")]
ClientStateVerificationFailure(ClientId),
}

impl Kind {
Expand Down
155 changes: 84 additions & 71 deletions modules/src/ics03_connection/handler/conn_open_ack.rs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -96,7 +96,7 @@ mod tests {

use crate::handler::EventType;
use crate::ics03_connection::connection::{ConnectionEnd, Counterparty, State};
use crate::ics03_connection::context::ConnectionReader;
use crate::ics03_connection::error::Kind;
use crate::ics03_connection::handler::{dispatch, ConnectionResult};
use crate::ics03_connection::msgs::conn_open_ack::test_util::get_dummy_msg_conn_open_ack;
use crate::ics03_connection::msgs::conn_open_ack::MsgConnectionOpenAck;
Expand All @@ -114,115 +114,119 @@ mod tests {
ctx: MockContext,
msg: ConnectionMsg,
want_pass: bool,
error_kind: Option<Kind>,
}

let client_id = ClientId::from_str("mock_clientid").unwrap();
let msg_ack = MsgConnectionOpenAck::try_from(get_dummy_msg_conn_open_ack()).unwrap();
let counterparty = Counterparty::new(
client_id.clone(),
msg_ack.counterparty_connection_id().cloned(),
CommitmentPrefix::from(vec![]),
);
let conn_id = msg_ack.connection_id.clone();

let incorrect_context = MockContext::default();
// Client parameters -- identifier and correct height (matching the proof height)
let client_id = ClientId::from_str("mock_clientid").unwrap();
let proof_height = msg_ack.proofs.height();

// A connection end (with incorrect state `Open`) that will be part of the context.
let incorrect_conn_end_state = ConnectionEnd::new(
State::Open,
client_id.clone(),
counterparty,
incorrect_context.get_compatible_versions(),
0_u64,
// Parametrize the host chain to have a height at least as recent as the
// the height of the proofs in the Ack msg.
let default_context = MockContext::new(
ChainId::new("mockgaia".to_string(), 1),
HostType::Mock,
5,
Height::new(1, msg_ack.proofs().height().increment().revision_height),
);

// A connection end (with correct state but incorrect versions); exercises unsuccessful
// processing path.
let mut incorrect_conn_end_vers = incorrect_conn_end_state.clone();
incorrect_conn_end_vers.set_state(State::Init);

// A connection end (with correct versions and correct state, but incorrect prefix for the
// counterparty) that will be part of the context to exercise unsuccessful path.
let mut incorrect_conn_end_prefix = incorrect_conn_end_state.clone();
incorrect_conn_end_prefix.set_state(State::Init);
incorrect_conn_end_prefix.set_version(msg_ack.version().clone());

// Build a connection end that will exercise the successful path.
let correct_counterparty = Counterparty::new(
client_id.clone(),
msg_ack.counterparty_connection_id().cloned(),
CommitmentPrefix::from(b"ibc".to_vec()),
);
let correct_conn_end = ConnectionEnd::new(
// A connection end that will exercise the successful path.
let default_conn_end = ConnectionEnd::new(
State::Init,
client_id.clone(),
correct_counterparty,
Counterparty::new(
client_id.clone(),
msg_ack.counterparty_connection_id().cloned(),
CommitmentPrefix::from(b"ibc".to_vec()),
),
vec![msg_ack.version().clone()],
0_u64,
);

// Parametrize the (correct) host chain to have a height at least as recent as the
// the height of the proofs in the Ack msg.
let correct_context = MockContext::new(
ChainId::new("mockgaia".to_string(), 1),
HostType::Mock,
5,
Height::new(1, msg_ack.proofs().height().increment().revision_height),
);
// A connection end with incorrect state `Open`; will be part of the context.
let mut conn_end_open = default_conn_end.clone();
conn_end_open.set_state(State::Open); // incorrect field

// A connection end with correct state, but incorrect prefix for the
// counterparty; will be part of the context to exercise unsuccessful path.
let mut conn_end_prefix = conn_end_open.clone();
conn_end_prefix.set_state(State::Init);
conn_end_prefix.set_counterparty(Counterparty::new(
client_id.clone(),
msg_ack.counterparty_connection_id().cloned(),
CommitmentPrefix::from(vec![]), // incorrect field
));

// A connection end with correct state & prefix, but incorrect counterparty; exercises
// unsuccessful processing path.
let mut conn_end_cparty = conn_end_open.clone();
conn_end_cparty.set_state(State::Init);
conn_end_cparty.set_counterparty(Counterparty::new(
client_id.clone(),
None, // incorrect field
CommitmentPrefix::from(b"ibc".to_vec()),
));

let tests: Vec<Test> = vec![
Test {
name: "Processing fails due to missing connection in context".to_string(),
ctx: correct_context.clone(),
name: "Successful processing of an Ack message".to_string(),
ctx: default_context
.clone()
.with_client(&client_id, proof_height)
.with_connection(conn_id.clone(), default_conn_end.clone()),
msg: ConnectionMsg::ConnectionOpenAck(Box::new(msg_ack.clone())),
want_pass: false,
want_pass: true,
error_kind: None,
},
Test {
name: "Processing fails due to connections mismatch (incorrect state)".to_string(),
ctx: correct_context
.clone()
.with_client(&client_id, Height::new(0, 10))
.with_connection(msg_ack.connection_id().clone(), incorrect_conn_end_state),
name: "Processing fails because the connection does not exist in the context".to_string(),
ctx: MockContext::default(), // Empty context
msg: ConnectionMsg::ConnectionOpenAck(Box::new(msg_ack.clone())),
want_pass: false,
error_kind: Some(Kind::UninitializedConnection(conn_id.clone())),
},
Test {
name: "Processing fails due to connections mismatch (incorrect versions)"
.to_string(),
ctx: correct_context
name: "Processing fails due to connections mismatch (incorrect 'open' state)".to_string(),
ctx: default_context
.clone()
.with_client(&client_id, Height::new(0, 10))
.with_connection(msg_ack.connection_id().clone(), incorrect_conn_end_vers),
.with_client(&client_id, proof_height)
.with_connection(conn_id.clone(), conn_end_open),
msg: ConnectionMsg::ConnectionOpenAck(Box::new(msg_ack.clone())),
want_pass: false,
error_kind: Some(Kind::ConnectionMismatch(conn_id.clone()))
},
Test {
name: "Processing fails: ConsensusStateVerificationFailure due to empty counterparty prefix".to_string(),
ctx: correct_context
ctx: default_context
.clone()
.with_client(&client_id, Height::new(0, 10))
.with_connection(msg_ack.connection_id().clone(), incorrect_conn_end_prefix),
.with_client(&client_id, proof_height)
.with_connection(conn_id.clone(), conn_end_prefix),
msg: ConnectionMsg::ConnectionOpenAck(Box::new(msg_ack.clone())),
want_pass: false,
error_kind: Some(Kind::ConsensusStateVerificationFailure(proof_height))
},
Test {
name: "Processing fails due to MissingLocalConsensusState".to_string(),
ctx: incorrect_context
.with_client(&client_id, Height::new(0, 10))
.with_connection(msg_ack.connection_id().clone(), correct_conn_end.clone()),
name: "Processing fails due to mismatching counterparty conn id".to_string(),
ctx: default_context
.with_client(&client_id, proof_height)
.with_connection(conn_id.clone(), conn_end_cparty),
msg: ConnectionMsg::ConnectionOpenAck(Box::new(msg_ack.clone())),
want_pass: false,
error_kind: Some(Kind::ConnectionMismatch(conn_id.clone()))
},
Test {
name: "Successful processing of Ack message".to_string(),
ctx: correct_context
.with_client(&client_id, Height::new(0, 10))
.with_connection(msg_ack.connection_id().clone(), correct_conn_end),
msg: ConnectionMsg::ConnectionOpenAck(Box::new(msg_ack.clone())),
want_pass: true,
name: "Processing fails due to MissingLocalConsensusState".to_string(),
ctx: MockContext::default()
.with_client(&client_id, proof_height)
.with_connection(conn_id, default_conn_end),
msg: ConnectionMsg::ConnectionOpenAck(Box::new(msg_ack)),
want_pass: false,
error_kind: Some(Kind::MissingLocalConsensusState)
},
]
.into_iter()
.collect();
];

for test in tests {
let res = dispatch(&test.ctx, test.msg.clone());
Expand All @@ -248,6 +252,7 @@ mod tests {
}
}
Err(e) => {
println!("Error for {:?} was {:#?}", test.name, e.kind());
assert_eq!(
test.want_pass,
false,
Expand All @@ -257,6 +262,14 @@ mod tests {
test.ctx.clone(),
e,
);
// Verify that the error kind matches
if let Some(expected_kind) = test.error_kind {
assert_eq!(
&expected_kind,
e.kind(),
"conn_open_ack: expected error kind mismatches thrown error kind"
)
}
}
}
}
Expand Down
Loading

0 comments on commit bc6b445

Please sign in to comment.