chore: deprecate MsgSubmitMisbehaviour (#1079)

* Decorate MsgSubmitMisbehaviour with `deprecated` attribute * Add #[allow(deprecated)] attributes in ibc-testkit * Add #![allow(deprecated)] attribute to ics02/types/msgs/mod.rs * Remove usages of MsgSubmitMisbehaviour from ibc-testkit/update_client.rs * Remove unused rstest fixture * Fix incorrect doc comment * Add changelog entry * chore: add missing unclog for PR835 --------- Co-authored-by: Farhad Shabani <[email protected]>
cosmos · Feb 7, 2024 · 4428fa9 · 4428fa9
1 parent 21b6ad0
commit 4428fa9
Show file tree

Hide file tree

Showing 6 changed files with 32 additions and 60 deletions.
diff --git a/.changelog/unreleased/improvements/1077-deprecate-msgsubmitmisbehaviour.md b/.changelog/unreleased/improvements/1077-deprecate-msgsubmitmisbehaviour.md
@@ -0,0 +1,3 @@
+- [ibc-core] Deprecate `MsgSubmitMisbehaviour` in favor of `MsgUpdateClient` for
+  submitting misbehaviour reports
+  ([\#1077](https://github.com/cosmos/ibc-rs/issues/1077))
diff --git a/...og/unreleased/improvements/835-msg-update-client-handler-accept-misbehaviour.md b/...og/unreleased/improvements/835-msg-update-client-handler-accept-misbehaviour.md
@@ -0,0 +1,3 @@
+- [ibc-core] Update `MsgUpdateClient` handler to accept misbehaviour reports via
+  its `client_message` field
+  ([\#835](https://github.com/cosmos/ibc-rs/issues/835))
diff --git a/ibc-core/ics02-client/types/src/msgs/misbehaviour.rs b/ibc-core/ics02-client/types/src/msgs/misbehaviour.rs
@@ -12,6 +12,13 @@ use crate::error::ClientError;
 pub const SUBMIT_MISBEHAVIOUR_TYPE_URL: &str = "/ibc.core.client.v1.MsgSubmitMisbehaviour";
 
 /// A type of message that submits client misbehaviour proof.
+///
+/// Deprecated since v0.51.0. Misbehaviour reports should be submitted via the `MsgUpdateClient`
+/// type through its `client_message` field.
+#[deprecated(
+    since = "0.51.0",
+    note = "Misbehaviour reports should be submitted via `MsgUpdateClient` through its `client_message` field"
+)]
 #[cfg_attr(
     feature = "borsh",
     derive(borsh::BorshSerialize, borsh::BorshDeserialize)
@@ -32,7 +39,6 @@ impl Protobuf<RawMsgSubmitMisbehaviour> for MsgSubmitMisbehaviour {}
 impl TryFrom<RawMsgSubmitMisbehaviour> for MsgSubmitMisbehaviour {
     type Error = ClientError;
 
-    #[allow(deprecated)]
     fn try_from(raw: RawMsgSubmitMisbehaviour) -> Result<Self, Self::Error> {
         let raw_misbehaviour = raw
             .misbehaviour
@@ -51,7 +57,6 @@ impl TryFrom<RawMsgSubmitMisbehaviour> for MsgSubmitMisbehaviour {
 
 impl From<MsgSubmitMisbehaviour> for RawMsgSubmitMisbehaviour {
     fn from(ics_msg: MsgSubmitMisbehaviour) -> Self {
-        #[allow(deprecated)]
         RawMsgSubmitMisbehaviour {
             client_id: ics_msg.client_id.to_string(),
             misbehaviour: Some(ics_msg.misbehaviour),

diff --git a/ibc-core/ics02-client/types/src/msgs/mod.rs b/ibc-core/ics02-client/types/src/msgs/mod.rs
@@ -1,4 +1,7 @@
+#![allow(deprecated)]
+
 //! Defines the client message types that are sent to the chain by the relayer.
+
 use ibc_core_host_types::identifiers::ClientId;
 use ibc_primitives::prelude::*;
 use ibc_primitives::Signer;

diff --git a/ibc-core/ics25-handler/types/src/msgs.rs b/ibc-core/ics25-handler/types/src/msgs.rs
@@ -6,6 +6,7 @@ use ibc_core_channel_types::msgs::{
     CHAN_OPEN_INIT_TYPE_URL, CHAN_OPEN_TRY_TYPE_URL, RECV_PACKET_TYPE_URL,
     TIMEOUT_ON_CLOSE_TYPE_URL, TIMEOUT_TYPE_URL,
 };
+#[allow(deprecated)]
 use ibc_core_client_types::msgs::{
     ClientMsg, MsgCreateClient, MsgSubmitMisbehaviour, MsgUpdateClient, MsgUpgradeClient,
     CREATE_CLIENT_TYPE_URL, SUBMIT_MISBEHAVIOUR_TYPE_URL, UPDATE_CLIENT_TYPE_URL,
@@ -35,6 +36,7 @@ pub enum MsgEnvelope {
     Packet(PacketMsg),
 }
 
+#[allow(deprecated)]
 impl TryFrom<Any> for MsgEnvelope {
     type Error = RouterError;
 

diff --git a/ibc-testkit/tests/core/ics02_client/update_client.rs b/ibc-testkit/tests/core/ics02_client/update_client.rs
@@ -9,7 +9,7 @@ use ibc::clients::tendermint::types::{
 };
 use ibc::core::client::context::client_state::{ClientStateCommon, ClientStateValidation};
 use ibc::core::client::context::ClientValidationContext;
-use ibc::core::client::types::msgs::{ClientMsg, MsgSubmitMisbehaviour, MsgUpdateClient};
+use ibc::core::client::types::msgs::{ClientMsg, MsgUpdateClient};
 use ibc::core::client::types::proto::v1::Height as RawHeight;
 use ibc::core::client::types::Height;
 use ibc::core::commitment_types::specs::ProofSpecs;
@@ -57,11 +57,8 @@ fn fixture() -> Fixture {
     Fixture { ctx, router }
 }
 
-/// rstest fixture that returns a `MsgEnvelope` with the `client_message`
-/// field set to a `MockMisbehaviour` report.
-#[fixture]
-fn msg_update_client() -> MsgEnvelope {
-    let client_id = ClientId::default();
+/// Returns a `MsgEnvelope` with the `client_message` field set to a `MockMisbehaviour` report.
+fn msg_update_client(client_id: &ClientId) -> MsgEnvelope {
     let timestamp = Timestamp::now();
     let height = Height::new(0, 46).unwrap();
     let msg = MsgUpdateClient {
@@ -78,27 +75,6 @@ fn msg_update_client() -> MsgEnvelope {
     MsgEnvelope::from(ClientMsg::from(msg))
 }
 
-/// rstest fixture that returns a `MsgEnvelope` with the `misbehaviour`
-/// field set to a `MockMisbehaviour` report.
-#[fixture]
-fn msg_submit_misbehaviour() -> MsgEnvelope {
-    let client_id = ClientId::default();
-    let timestamp = Timestamp::now();
-    let height = Height::new(0, 46).unwrap();
-    let msg = MsgSubmitMisbehaviour {
-        client_id: client_id.clone(),
-        misbehaviour: MockMisbehaviour {
-            client_id: client_id.clone(),
-            header1: MockHeader::new(height).with_timestamp(timestamp),
-            header2: MockHeader::new(height).with_timestamp(timestamp),
-        }
-        .into(),
-        signer: dummy_account_id(),
-    };
-
-    MsgEnvelope::from(ClientMsg::from(msg))
-}
-
 #[rstest]
 fn test_update_client_ok(fixture: Fixture) {
     let Fixture {
@@ -782,15 +758,14 @@ fn ensure_misbehaviour(ctx: &MockContext, client_id: &ClientId, client_type: &Cl
 /// Misbehaviour evidence consists of identical headers - mock misbehaviour handler
 /// considers it a valid proof of misbehaviour.
 #[rstest]
-#[case::msg_submit_misbehaviour(msg_submit_misbehaviour())]
-#[case::msg_update_client(msg_update_client())]
-fn test_misbehaviour_client_ok(fixture: Fixture, #[case] msg_envelope: MsgEnvelope) {
+fn test_misbehaviour_client_ok(fixture: Fixture) {
     let Fixture {
         mut ctx,
         mut router,
     } = fixture;
 
     let client_id = ClientId::default();
+    let msg_envelope = msg_update_client(&client_id);
 
     let res = validate(&ctx, &router, msg_envelope.clone());
     assert!(res.is_ok());
@@ -806,18 +781,8 @@ fn test_submit_misbehaviour_nonexisting_client(fixture: Fixture) {
     let Fixture { router, .. } = fixture;
 
     let client_id = ClientId::from_str("mockclient1").unwrap();
-    let height = Height::new(0, 46).unwrap();
-    let msg = MsgSubmitMisbehaviour {
-        client_id: ClientId::from_str("nonexistingclient").unwrap(),
-        misbehaviour: MockMisbehaviour {
-            client_id: client_id.clone(),
-            header1: MockHeader::new(height),
-            header2: MockHeader::new(height),
-        }
-        .into(),
-        signer: dummy_account_id(),
-    };
-    let msg_envelope = MsgEnvelope::from(ClientMsg::from(msg));
+
+    let msg_envelope = msg_update_client(&ClientId::from_str("nonexistingclient").unwrap());
 
     let ctx = MockContext::default().with_client_config(
         MockClientConfig::builder()
@@ -834,18 +799,8 @@ fn test_client_update_misbehaviour_nonexisting_client(fixture: Fixture) {
     let Fixture { router, .. } = fixture;
 
     let client_id = ClientId::from_str("mockclient1").unwrap();
-    let height = Height::new(0, 46).unwrap();
-    let msg = MsgUpdateClient {
-        client_id: ClientId::from_str("nonexistingclient").unwrap(),
-        client_message: MockMisbehaviour {
-            client_id: client_id.clone(),
-            header1: MockHeader::new(height),
-            header2: MockHeader::new(height),
-        }
-        .into(),
-        signer: dummy_account_id(),
-    };
-    let msg_envelope = MsgEnvelope::from(ClientMsg::from(msg));
+
+    let msg_envelope = msg_update_client(&ClientId::from_str("nonexistingclient").unwrap());
 
     let ctx = MockContext::default().with_client_config(
         MockClientConfig::builder()
@@ -907,9 +862,9 @@ fn test_misbehaviour_synthetic_tendermint_equivocation() {
         tm_block.into()
     };
 
-    let msg = MsgSubmitMisbehaviour {
+    let msg = MsgUpdateClient {
         client_id: client_id.clone(),
-        misbehaviour: TmMisbehaviour::new(client_id.clone(), header1, header2).into(),
+        client_message: TmMisbehaviour::new(client_id.clone(), header1, header2).into(),
         signer: dummy_account_id(),
     };
     let msg_envelope = MsgEnvelope::from(ClientMsg::from(msg));
@@ -969,9 +924,10 @@ fn test_misbehaviour_synthetic_tendermint_bft_time() {
         tm_block
     };
 
-    let msg = MsgSubmitMisbehaviour {
+    let msg = MsgUpdateClient {
         client_id: client_id.clone(),
-        misbehaviour: TmMisbehaviour::new(client_id.clone(), header1.into(), header2.into()).into(),
+        client_message: TmMisbehaviour::new(client_id.clone(), header1.into(), header2.into())
+            .into(),
         signer: dummy_account_id(),
     };