From b00da0402f384a0d0b9fd8216b555bbb1b0c4cc7 Mon Sep 17 00:00:00 2001 From: Farhad Shabani Date: Tue, 5 Sep 2023 10:25:06 -0700 Subject: [PATCH] fix: increase_*_counters return Result --- ...-use-result-for-safe-counters-increment.md | 3 ++ crates/ibc/src/core/context.rs | 6 ++-- crates/ibc/src/core/ics02_client/error.rs | 2 ++ .../ics02_client/handler/create_client.rs | 12 ++++++- crates/ibc/src/core/ics03_connection/error.rs | 2 ++ .../handler/conn_open_init.rs | 5 ++- .../ics03_connection/handler/conn_open_try.rs | 5 ++- crates/ibc/src/core/ics04_channel/error.rs | 6 ++-- .../ics04_channel/handler/chan_open_init.rs | 5 ++- .../ics04_channel/handler/chan_open_try.rs | 5 ++- crates/ibc/src/mock/context.rs | 33 +++++++++++++++---- 11 files changed, 68 insertions(+), 16 deletions(-) create mode 100644 .changelog/unreleased/breaking-change/857-use-result-for-safe-counters-increment.md diff --git a/.changelog/unreleased/breaking-change/857-use-result-for-safe-counters-increment.md b/.changelog/unreleased/breaking-change/857-use-result-for-safe-counters-increment.md new file mode 100644 index 000000000..f0f87285e --- /dev/null +++ b/.changelog/unreleased/breaking-change/857-use-result-for-safe-counters-increment.md @@ -0,0 +1,3 @@ +- Allow hosts to handle overflow cases in `increase_*_counter` methods by + returning `Result<(),ContextError>` type. + ([#857](https://github.com/cosmos/ibc-rs/issues/857)) diff --git a/crates/ibc/src/core/context.rs b/crates/ibc/src/core/context.rs index 9d9ae5e77..02b1387e8 100644 --- a/crates/ibc/src/core/context.rs +++ b/crates/ibc/src/core/context.rs @@ -251,7 +251,7 @@ pub trait ExecutionContext: ValidationContext { /// Called upon client creation. /// Increases the counter which keeps track of how many clients have been created. /// Should never fail. - fn increase_client_counter(&mut self); + fn increase_client_counter(&mut self) -> Result<(), ContextError>; /// Called upon successful client update. /// Implementations are expected to use this to record the specified time as the time at which @@ -290,7 +290,7 @@ pub trait ExecutionContext: ValidationContext { /// Called upon connection identifier creation (Init or Try process). /// Increases the counter which keeps track of how many connections have been created. /// Should never fail. - fn increase_connection_counter(&mut self); + fn increase_connection_counter(&mut self) -> Result<(), ContextError>; /// Stores the given packet commitment at the given store path fn store_packet_commitment( @@ -353,7 +353,7 @@ pub trait ExecutionContext: ValidationContext { /// Called upon channel identifier creation (Init or Try message processing). /// Increases the counter which keeps track of how many channels have been created. /// Should never fail. - fn increase_channel_counter(&mut self); + fn increase_channel_counter(&mut self) -> Result<(), ContextError>; /// Emit the given IBC event fn emit_ibc_event(&mut self, event: IbcEvent); diff --git a/crates/ibc/src/core/ics02_client/error.rs b/crates/ibc/src/core/ics02_client/error.rs index c055863cf..9ae4336e2 100644 --- a/crates/ibc/src/core/ics02_client/error.rs +++ b/crates/ibc/src/core/ics02_client/error.rs @@ -103,6 +103,8 @@ pub enum ClientError { MisbehaviourHandlingFailure { reason: String }, /// client specific error: `{description}` ClientSpecific { description: String }, + /// client counter overflow error + CounterOverflow, /// other error: `{description}` Other { description: String }, } diff --git a/crates/ibc/src/core/ics02_client/handler/create_client.rs b/crates/ibc/src/core/ics02_client/handler/create_client.rs index ff770a22b..59082c005 100644 --- a/crates/ibc/src/core/ics02_client/handler/create_client.rs +++ b/crates/ibc/src/core/ics02_client/handler/create_client.rs @@ -81,7 +81,7 @@ where ctx.store_update_time(client_id.clone(), latest_height, ctx.host_timestamp()?)?; ctx.store_update_height(client_id.clone(), latest_height, ctx.host_height()?)?; - ctx.increase_client_counter(); + ctx.increase_client_counter()?; let event = IbcEvent::CreateClient(CreateClient::new( client_id.clone(), @@ -144,8 +144,12 @@ mod tests { assert!(res.is_ok(), "execution happy path"); + assert_eq!(ctx.client_counter().unwrap(), 1); + let expected_client_state = ctx.decode_client_state(msg.client_state).unwrap(); + assert_eq!(expected_client_state.client_type(), client_type); + assert_eq!(ctx.client_state(&client_id).unwrap(), expected_client_state); } @@ -173,13 +177,19 @@ mod tests { ); let res = validate(&ctx, msg.clone()); + assert!(res.is_ok(), "tendermint client validation happy path"); let res = execute(&mut ctx, msg.clone()); + assert!(res.is_ok(), "tendermint client execution happy path"); + assert_eq!(ctx.client_counter().unwrap(), 1); + let expected_client_state = ctx.decode_client_state(msg.client_state).unwrap(); + assert_eq!(expected_client_state.client_type(), client_type); + assert_eq!(ctx.client_state(&client_id).unwrap(), expected_client_state); } } diff --git a/crates/ibc/src/core/ics03_connection/error.rs b/crates/ibc/src/core/ics03_connection/error.rs index 3364a274c..40d677ade 100644 --- a/crates/ibc/src/core/ics03_connection/error.rs +++ b/crates/ibc/src/core/ics03_connection/error.rs @@ -85,6 +85,8 @@ pub enum ConnectionError { }, /// timestamp overflowed error: `{0}` TimestampOverflow(TimestampOverflowError), + /// connection counter overflow error + CounterOverflow, /// other error: `{description}` Other { description: String }, } diff --git a/crates/ibc/src/core/ics03_connection/handler/conn_open_init.rs b/crates/ibc/src/core/ics03_connection/handler/conn_open_init.rs index 6ee7daedc..c583482fa 100644 --- a/crates/ibc/src/core/ics03_connection/handler/conn_open_init.rs +++ b/crates/ibc/src/core/ics03_connection/handler/conn_open_init.rs @@ -77,7 +77,7 @@ where ctx_a.emit_ibc_event(event); } - ctx_a.increase_connection_counter(); + ctx_a.increase_connection_counter()?; ctx_a.store_connection_to_client( &ClientConnectionPath::new(&msg.client_id_on_a), conn_id_on_a.clone(), @@ -161,6 +161,9 @@ mod tests { } Expect::Success => { assert!(res.is_ok(), "{err_msg}"); + + assert_eq!(fxt.ctx.connection_counter().unwrap(), 1); + assert_eq!(fxt.ctx.events.len(), 2); assert!(matches!( diff --git a/crates/ibc/src/core/ics03_connection/handler/conn_open_try.rs b/crates/ibc/src/core/ics03_connection/handler/conn_open_try.rs index 2cdd5da8c..123a58be0 100644 --- a/crates/ibc/src/core/ics03_connection/handler/conn_open_try.rs +++ b/crates/ibc/src/core/ics03_connection/handler/conn_open_try.rs @@ -160,7 +160,7 @@ where ctx_b.emit_ibc_event(event); ctx_b.log_message("success: conn_open_try verification passed".to_string()); - ctx_b.increase_connection_counter(); + ctx_b.increase_connection_counter()?; ctx_b.store_connection_to_client( &ClientConnectionPath::new(&msg.client_id_on_b), vars.conn_id_on_b.clone(), @@ -294,6 +294,9 @@ mod tests { } Expect::Success => { assert!(res.is_ok(), "{err_msg}"); + + assert_eq!(fxt.ctx.connection_counter().unwrap(), 1); + assert_eq!(fxt.ctx.events.len(), 2); assert!(matches!( diff --git a/crates/ibc/src/core/ics04_channel/error.rs b/crates/ibc/src/core/ics04_channel/error.rs index 669a54e2f..aaa99f546 100644 --- a/crates/ibc/src/core/ics04_channel/error.rs +++ b/crates/ibc/src/core/ics04_channel/error.rs @@ -68,14 +68,16 @@ pub enum ChannelError { ProcessedHeightNotFound { client_id: ClientId, height: Height }, /// application module error: `{description}` AppModule { description: String }, - /// other error: `{description}` - Other { description: String }, /// Undefined counterparty connection for `{connection_id}` UndefinedConnectionCounterparty { connection_id: ConnectionId }, /// invalid proof: empty proof InvalidProof, /// identifier error: `{0}` InvalidIdentifier(IdentifierError), + /// channel counter overflow error + CounterOverflow, + /// other error: `{description}` + Other { description: String }, } #[derive(Debug, Display)] diff --git a/crates/ibc/src/core/ics04_channel/handler/chan_open_init.rs b/crates/ibc/src/core/ics04_channel/handler/chan_open_init.rs index 15d5792ac..c6f550712 100644 --- a/crates/ibc/src/core/ics04_channel/handler/chan_open_init.rs +++ b/crates/ibc/src/core/ics04_channel/handler/chan_open_init.rs @@ -67,7 +67,7 @@ where let chan_end_path_on_a = ChannelEndPath::new(&msg.port_id_on_a, &chan_id_on_a); ctx_a.store_channel(&chan_end_path_on_a, chan_end_on_a)?; - ctx_a.increase_channel_counter(); + ctx_a.increase_channel_counter()?; // Initialize send, recv, and ack sequence numbers. let seq_send_path = SeqSendPath::new(&msg.port_id_on_a, &chan_id_on_a); @@ -251,7 +251,10 @@ mod tests { assert!(res.is_ok(), "Execution succeeds; good parameters"); + assert_eq!(ctx.channel_counter().unwrap(), 1); + assert_eq!(ctx.events.len(), 2); + assert!(matches!( ctx.events[0], IbcEvent::Message(MessageEvent::Channel) diff --git a/crates/ibc/src/core/ics04_channel/handler/chan_open_try.rs b/crates/ibc/src/core/ics04_channel/handler/chan_open_try.rs index 4cf78dce7..1effe5d9e 100644 --- a/crates/ibc/src/core/ics04_channel/handler/chan_open_try.rs +++ b/crates/ibc/src/core/ics04_channel/handler/chan_open_try.rs @@ -75,7 +75,7 @@ where let chan_end_path_on_b = ChannelEndPath::new(&msg.port_id_on_b, &chan_id_on_b); ctx_b.store_channel(&chan_end_path_on_b, chan_end_on_b)?; - ctx_b.increase_channel_counter(); + ctx_b.increase_channel_counter()?; // Initialize send, recv, and ack sequence numbers. let seq_send_path = SeqSendPath::new(&msg.port_id_on_b, &chan_id_on_b); @@ -337,7 +337,10 @@ mod tests { assert!(res.is_ok(), "Execution success: happy path"); + assert_eq!(ctx.channel_counter().unwrap(), 1); + assert_eq!(ctx.events.len(), 2); + assert!(matches!( ctx.events[0], IbcEvent::Message(MessageEvent::Channel) diff --git a/crates/ibc/src/mock/context.rs b/crates/ibc/src/mock/context.rs index 1830b211d..83ebcab32 100644 --- a/crates/ibc/src/mock/context.rs +++ b/crates/ibc/src/mock/context.rs @@ -1090,8 +1090,15 @@ impl ExecutionContext for MockContext { self } - fn increase_client_counter(&mut self) { - self.ibc_store.lock().client_ids_counter += 1 + fn increase_client_counter(&mut self) -> Result<(), ContextError> { + let mut ibc_store = self.ibc_store.lock(); + + ibc_store.client_ids_counter = ibc_store + .client_ids_counter + .checked_add(1) + .ok_or(ClientError::CounterOverflow)?; + + Ok(()) } fn store_update_time( @@ -1148,8 +1155,15 @@ impl ExecutionContext for MockContext { Ok(()) } - fn increase_connection_counter(&mut self) { - self.ibc_store.lock().connection_ids_counter += 1; + fn increase_connection_counter(&mut self) -> Result<(), ContextError> { + let mut ibc_store = self.ibc_store.lock(); + + ibc_store.connection_ids_counter = ibc_store + .connection_ids_counter + .checked_add(1) + .ok_or(ClientError::CounterOverflow)?; + + Ok(()) } fn store_packet_commitment( @@ -1299,8 +1313,15 @@ impl ExecutionContext for MockContext { Ok(()) } - fn increase_channel_counter(&mut self) { - self.ibc_store.lock().channel_ids_counter += 1; + fn increase_channel_counter(&mut self) -> Result<(), ContextError> { + let mut ibc_store = self.ibc_store.lock(); + + ibc_store.channel_ids_counter = ibc_store + .channel_ids_counter + .checked_add(1) + .ok_or(ClientError::CounterOverflow)?; + + Ok(()) } fn emit_ibc_event(&mut self, event: IbcEvent) {