Fix LaneId encode/decode for backwards compatibility

bridges/modules/messages/src/lib.rs

@@ -97,7 +97,7 @@ pub const LOG_TARGET: &str = "runtime::bridge-messages";
 #[frame_support::pallet]
 pub mod pallet {
 	use super::*;
-	use bp_messages::{ReceivedMessages, ReceptionResult};
+	use bp_messages::{LaneIdBytes, ReceivedMessages, ReceptionResult};
 	use bp_runtime::RangeInclusiveExt;
 	use frame_support::pallet_prelude::*;
 	use frame_system::pallet_prelude::*;
@@ -387,7 +387,7 @@ pub mod pallet {
 				// emit 'delivered' event
 				let received_range = confirmed_messages.begin..=confirmed_messages.end;
 				Self::deposit_event(Event::MessagesDelivered {
-					lane_id,
+					lane_id: lane_id.into(),
 					messages: confirmed_messages,
 				});
 
@@ -441,7 +441,7 @@ pub mod pallet {
 		/// Message has been accepted and is waiting to be delivered.
 		MessageAccepted {
 			/// Lane, which has accepted the message.
-			lane_id: LaneId,
+			lane_id: LaneIdBytes,
 			/// Nonce of accepted message.
 			nonce: MessageNonce,
 		},
@@ -453,7 +453,7 @@ pub mod pallet {
 		/// Messages in the inclusive range have been delivered to the bridged chain.
 		MessagesDelivered {
 			/// Lane for which the delivery has been confirmed.
-			lane_id: LaneId,
+			lane_id: LaneIdBytes,
 			/// Delivered messages.
 			messages: DeliveredMessages,
 		},
@@ -703,7 +703,10 @@ where
 			message_len,
 		);
 
-		Pallet::<T, I>::deposit_event(Event::MessageAccepted { lane_id: args.lane_id, nonce });
+		Pallet::<T, I>::deposit_event(Event::MessageAccepted {
+			lane_id: args.lane_id.into(),
+			nonce,
+		});
 
 		SendMessageArtifacts { nonce, enqueued_messages }
 	}

bridges/modules/messages/src/tests/pallet_tests.rs

@@ -67,7 +67,10 @@ fn send_regular_message(lane_id: LaneId) {
 		System::<TestRuntime>::events(),
 		vec![EventRecord {
 			phase: Phase::Initialization,
-			event: TestEvent::Messages(Event::MessageAccepted { lane_id, nonce: message_nonce }),
+			event: TestEvent::Messages(Event::MessageAccepted {
+				lane_id: lane_id.into(),
+				nonce: message_nonce
+			}),
 			topics: vec![],
 		}],
 	);
@@ -105,7 +108,7 @@ fn receive_messages_delivery_proof() {
 		vec![EventRecord {
 			phase: Phase::Initialization,
 			event: TestEvent::Messages(Event::MessagesDelivered {
-				lane_id: test_lane_id(),
+				lane_id: test_lane_id().into(),
 				messages: DeliveredMessages::new(1),
 			}),
 			topics: vec![],

bridges/modules/xcm-bridge-hub/src/lib.rs

@@ -143,7 +143,7 @@
 #![warn(missing_docs)]
 #![cfg_attr(not(feature = "std"), no_std)]
 
-use bp_messages::{LaneId, LaneState, MessageNonce};
+use bp_messages::{LaneId, LaneIdBytes, LaneState, MessageNonce};
 use bp_runtime::{AccountIdOf, BalanceOf, RangeInclusiveExt};
 pub use bp_xcm_bridge_hub::{Bridge, BridgeId, BridgeState};
 use bp_xcm_bridge_hub::{BridgeLocations, BridgeLocationsError, LocalXcmChannelManager};
@@ -391,7 +391,7 @@ pub mod pallet {
 				// deposit the `ClosingBridge` event
 				Self::deposit_event(Event::<T, I>::ClosingBridge {
 					bridge_id: *locations.bridge_id(),
-					lane_id: bridge.lane_id,
+					lane_id: bridge.lane_id.into(),
 					pruned_messages,
 					enqueued_messages,
 				});
@@ -438,7 +438,7 @@ pub mod pallet {
 			// deposit the `BridgePruned` event
 			Self::deposit_event(Event::<T, I>::BridgePruned {
 				bridge_id: *locations.bridge_id(),
-				lane_id: bridge.lane_id,
+				lane_id: bridge.lane_id.into(),
 				bridge_deposit: released_deposit,
 				pruned_messages,
 			});
@@ -541,7 +541,7 @@ pub mod pallet {
 				remote_endpoint: Box::new(
 					locations.bridge_destination_universal_location().clone(),
 				),
-				lane_id,
+				lane_id: lane_id.into(),
 			});
 
 			Ok(())
@@ -805,14 +805,14 @@ pub mod pallet {
 			/// Universal location of remote bridge endpoint.
 			remote_endpoint: Box<InteriorLocation>,
 			/// Lane identifier.
-			lane_id: LaneId,
+			lane_id: LaneIdBytes,
 		},
 		/// Bridge is going to be closed, but not yet fully pruned from the runtime storage.
 		ClosingBridge {
 			/// Bridge identifier.
 			bridge_id: BridgeId,
 			/// Lane identifier.
-			lane_id: LaneId,
+			lane_id: LaneIdBytes,
 			/// Number of pruned messages during the close call.
 			pruned_messages: MessageNonce,
 			/// Number of enqueued messages that need to be pruned in follow up calls.
@@ -824,7 +824,7 @@ pub mod pallet {
 			/// Bridge identifier.
 			bridge_id: BridgeId,
 			/// Lane identifier.
-			lane_id: LaneId,
+			lane_id: LaneIdBytes,
 			/// Amount of deposit released.
 			bridge_deposit: BalanceOf<ThisChainOf<T, I>>,
 			/// Number of pruned messages during the close call.
@@ -1221,7 +1221,7 @@ mod tests {
 							remote_endpoint: Box::new(
 								locations.bridge_destination_universal_location().clone()
 							),
-							lane_id
+							lane_id: lane_id.into()
 						}),
 						topics: vec![],
 					}),
@@ -1364,7 +1364,7 @@ mod tests {
 					phase: Phase::Initialization,
 					event: RuntimeEvent::XcmOverBridge(Event::ClosingBridge {
 						bridge_id: *locations.bridge_id(),
-						lane_id: bridge.lane_id,
+						lane_id: bridge.lane_id.into(),
 						pruned_messages: 16,
 						enqueued_messages: 16,
 					}),
@@ -1412,7 +1412,7 @@ mod tests {
 					phase: Phase::Initialization,
 					event: RuntimeEvent::XcmOverBridge(Event::ClosingBridge {
 						bridge_id: *locations.bridge_id(),
-						lane_id: bridge.lane_id,
+						lane_id: bridge.lane_id.into(),
 						pruned_messages: 8,
 						enqueued_messages: 8,
 					}),
@@ -1453,7 +1453,7 @@ mod tests {
 					phase: Phase::Initialization,
 					event: RuntimeEvent::XcmOverBridge(Event::BridgePruned {
 						bridge_id: *locations.bridge_id(),
-						lane_id: bridge.lane_id,
+						lane_id: bridge.lane_id.into(),
 						bridge_deposit: expected_deposit,
 						pruned_messages: 8,
 					}),

bridges/primitives/messages/src/lane.rs

@@ -108,53 +108,66 @@ impl core::fmt::Debug for LaneId {
 	}
 }
 
-impl AsRef<[u8]> for LaneId {
-	fn as_ref(&self) -> &[u8] {
-		self.0.as_ref()
-	}
-}
-
 impl TypeId for LaneId {
 	const TYPE_ID: [u8; 4] = *b"blan";
 }
 
-#[derive(
-	Clone, Copy, Eq, Ord, PartialOrd, PartialEq, TypeInfo, MaxEncodedLen, Serialize, Deserialize,
-)]
+#[derive(Clone, Copy, Eq, Ord, PartialOrd, PartialEq, TypeInfo, Serialize, Deserialize)]
 enum InnerLaneId {
-	/// Old format (for backwards compatibility).
-	Array([u8; 4]),
 	/// New format 32-byte hash generated by `blake2_256`.
 	Hash(H256),
+	/// Old format (for backwards compatibility).
+	Array([u8; 4]),
+}
+
+// Prefix used to differentiate `LaneId` for backward compatibility.
+// Hex value: `48615368`
+const INNER_LANE_ID_AS_HASH_PREFIX: &[u8; 4] = b"HaSh";
+
+impl MaxEncodedLen for InnerLaneId {
+	fn max_encoded_len() -> usize {
+		INNER_LANE_ID_AS_HASH_PREFIX
+			.encoded_size()
+			.saturating_add(H256::max_encoded_len())
+	}
 }
 
 impl Encode for InnerLaneId {
 	fn encode(&self) -> sp_std::vec::Vec<u8> {
 		match self {
-			InnerLaneId::Array(array) => array.encode(),
-			InnerLaneId::Hash(hash) => hash.encode(),
+			InnerLaneId::Hash(hash) => {
+				// prefix new hash, so we can easily decode
+				(INNER_LANE_ID_AS_HASH_PREFIX, hash).encode()
+			},
+			InnerLaneId::Array(array) => {
+				// encode backwards compatible
+				array.encode()
+			},
 		}
 	}
 }
 
 impl Decode for InnerLaneId {
 	fn decode<I: Input>(input: &mut I) -> Result<Self, CodecError> {
-		// check backwards compatibly first
-		if input.remaining_len() == Ok(Some(4)) {
-			let array: [u8; 4] = Decode::decode(input)?;
-			return Ok(InnerLaneId::Array(array))
+		// read 4 bytes first
+		let prefix_or_array: [u8; 4] = Decode::decode(input)?;
+
+		// if matches prefix, it is a new format
+		if prefix_or_array.eq(INNER_LANE_ID_AS_HASH_PREFIX) {
+			// now read more 32 bytes for hash
+			return H256::decode(input).map(InnerLaneId::Hash)
 		}
 
-		// else check new format
-		H256::decode(input).map(InnerLaneId::Hash)
+		// return prefix `[u8; 4]` for backwards compatibly as a best effort
+		Ok(InnerLaneId::Array(prefix_or_array))
 	}
 }
 
 impl core::fmt::Display for InnerLaneId {
 	fn fmt(&self, f: &mut core::fmt::Formatter) -> core::fmt::Result {
 		match self {
-			InnerLaneId::Array(array) => write!(f, "Array({:?})", array),
-			InnerLaneId::Hash(hash) => write!(f, "Hash({:?})", hash),
+			InnerLaneId::Array(array) => write!(f, "InnerLaneId::Array({:?})", array),
+			InnerLaneId::Hash(hash) => write!(f, "InnerLaneId::Hash({:?})", hash),
 		}
 	}
 }
@@ -168,11 +181,14 @@ impl core::fmt::Debug for InnerLaneId {
 	}
 }
 
-impl AsRef<[u8]> for InnerLaneId {
-	fn as_ref(&self) -> &[u8] {
-		match self {
-			InnerLaneId::Array(array) => array.as_ref(),
-			InnerLaneId::Hash(hash) => hash.as_ref(),
+/// The representation of `LaneId`'s inner bytes.
+#[derive(Clone, Encode, Decode, RuntimeDebug, PartialEq, Eq, TypeInfo)]
+pub struct LaneIdBytes(sp_std::vec::Vec<u8>);
+impl From<LaneId> for LaneIdBytes {
+	fn from(lane_id: LaneId) -> Self {
+		match lane_id.inner() {
+			Either::Left(hash) => Self(hash.as_bytes().to_vec()),
+			Either::Right(array) => Self(array.to_vec()),
 		}
 	}
 }
@@ -202,6 +218,7 @@ impl LaneState {
 #[cfg(test)]
 mod tests {
 	use super::*;
+	use crate::MessageNonce;
 
 	#[test]
 	fn lane_id_debug_format_matches_inner_hash_format() {
@@ -216,35 +233,74 @@ mod tests {
 	}
 
 	#[test]
-	fn lane_id_as_ref_works() {
+	fn encode_decode_works() {
+		// simple encode/decode - new format
+		let lane_id = LaneId(InnerLaneId::Hash(H256::from([1u8; 32])));
+		let encoded_lane_id = lane_id.encode();
+		let decoded_lane_id = LaneId::decode(&mut &encoded_lane_id[..]).expect("decodable");
+		assert_eq!(lane_id, decoded_lane_id);
 		assert_eq!(
-			"0101010101010101010101010101010101010101010101010101010101010101",
-			hex::encode(LaneId(InnerLaneId::Hash(H256::from([1u8; 32]))).as_ref()),
+			"486153680101010101010101010101010101010101010101010101010101010101010101",
+			hex::encode(encoded_lane_id)
 		);
-		assert_eq!("00000001", hex::encode(LaneId(InnerLaneId::Array([0, 0, 0, 1])).as_ref()),);
+
+		// simple encode/decode - old format
+		let lane_id = LaneId(InnerLaneId::Array([0, 0, 0, 1]));
+		let encoded_lane_id = lane_id.encode();
+		let decoded_lane_id = LaneId::decode(&mut &encoded_lane_id[..]).expect("decodable");
+		assert_eq!(lane_id, decoded_lane_id);
+		assert_eq!("00000001", hex::encode(encoded_lane_id));
+
+		// decode sample
+		let bytes = vec![0, 0, 0, 2, 1, 0, 0, 0, 0, 0, 0, 0, 1, 0, 0, 0, 0, 0, 0, 0];
+		let (lane, nonce_start, nonce_end): (LaneId, MessageNonce, MessageNonce) =
+			Decode::decode(&mut &bytes[..]).unwrap();
+		assert_eq!(lane, LaneId(InnerLaneId::Array([0, 0, 0, 2])));
+		assert_eq!(nonce_start, 1);
+		assert_eq!(nonce_end, 1);
+
+		// run encode/decode for `LaneId` with different positions
+		let test_data = vec![
+			(LaneId(InnerLaneId::Array([0, 0, 0, 1])), 1088_u64, 9185_u64),
+			(LaneId(InnerLaneId::Hash(H256::from([1u8; 32]))), 1088_u64, 9185_u64),
+		];
+		for (expected_lane, expected_nonce_start, expected_nonce_end) in test_data {
+			// decode: LaneId,Nonce,Nonce
+			let bytes = (expected_lane, expected_nonce_start, expected_nonce_end).encode();
+			let (lane, nonce_start, nonce_end): (LaneId, MessageNonce, MessageNonce) =
+				Decode::decode(&mut &bytes[..]).unwrap();
+			assert_eq!(lane, expected_lane);
+			assert_eq!(nonce_start, expected_nonce_start);
+			assert_eq!(nonce_end, expected_nonce_end);
+
+			// decode: Nonce,LaneId,Nonce
+			let bytes = (expected_nonce_start, expected_lane, expected_nonce_end).encode();
+			let (nonce_start, lane, nonce_end): (MessageNonce, LaneId, MessageNonce) =
+				Decode::decode(&mut &bytes[..]).unwrap();
+			assert_eq!(lane, expected_lane);
+			assert_eq!(nonce_start, expected_nonce_start);
+			assert_eq!(nonce_end, expected_nonce_end);
+
+			// decode: Nonce,Nonce,LaneId
+			let bytes = (expected_nonce_start, expected_nonce_end, expected_lane).encode();
+			let (nonce_start, nonce_end, lane): (MessageNonce, MessageNonce, LaneId) =
+				Decode::decode(&mut &bytes[..]).unwrap();
+			assert_eq!(lane, expected_lane);
+			assert_eq!(nonce_start, expected_nonce_start);
+			assert_eq!(nonce_end, expected_nonce_end);
+		}
 	}
 
 	#[test]
-	fn lane_id_encode_decode_works() {
-		let test_encode_decode = |expected_hex, lane_id: LaneId| {
-			let enc = lane_id.encode();
-			let decoded_lane_id = LaneId::decode(&mut &enc[..]).expect("decodable");
-			assert_eq!(lane_id, decoded_lane_id);
-
-			assert_eq!(expected_hex, hex::encode(lane_id.as_ref()),);
-			assert_eq!(expected_hex, hex::encode(decoded_lane_id.as_ref()),);
-
-			let hex_bytes = hex::decode(expected_hex).expect("valid hex");
-			let hex_decoded_lane_id = LaneId::decode(&mut &hex_bytes[..]).expect("decodable");
-			assert_eq!(hex_decoded_lane_id, lane_id);
-			assert_eq!(hex_decoded_lane_id, decoded_lane_id);
-		};
-
-		test_encode_decode(
+	fn lane_id_bytes_works() {
+		let lane_id_bytes: LaneIdBytes = LaneId(InnerLaneId::Array([0, 0, 0, 1])).into();
+		assert_eq!("00000001", hex::encode(lane_id_bytes.0));
+
+		let lane_id_bytes: LaneIdBytes = LaneId(InnerLaneId::Hash(H256::from([1u8; 32]))).into();
+		assert_eq!(
 			"0101010101010101010101010101010101010101010101010101010101010101",
-			LaneId(InnerLaneId::Hash(H256::from([1u8; 32]))),
+			hex::encode(lane_id_bytes.0)
 		);
-		test_encode_decode("00000001", LaneId(InnerLaneId::Array([0, 0, 0, 1])));
 	}
 
 	#[test]