Region pallet fixes (#129)

* Region pallet fixes

* fix

* cleanup & fixes

* emit request commitment

* fixes

* reduce timeout

pallets/regions/src/lib.rs

@@ -29,6 +29,7 @@ use ismp_parachain::PARACHAIN_CONSENSUS_ID;
 pub use pallet::*;
 use pallet_broker::RegionId;
 use scale_info::prelude::{format, vec, vec::Vec};
+use sp_core::H256;
 use sp_runtime::traits::Zero;
 
 #[cfg(test)]
@@ -135,6 +136,8 @@ pub mod pallet {
 			region_id: RegionId,
 			/// The account who requested the region record.
 			account: T::AccountId,
+			/// The ismp get request commitment.
+			request_commitment: H256,
 		},
 	}
 
@@ -153,6 +156,8 @@ pub mod pallet {
 		NotUnavailable,
 		/// The given region id is not valid.
 		InvalidRegionId,
+		/// Failed to get the latest height of the Coretime chain.
+		LatestHeightInaccessible,
 	}
 
 	#[pallet::call]
@@ -225,7 +230,7 @@ pub mod pallet {
 		pub(crate) fn do_request_region_record(
 			region_id: RegionId,
 			who: <T as frame_system::Config>::AccountId,
-		) -> DispatchResult {
+		) -> Result<H256, DispatchError> {
 			let pallet_hash = sp_io::hashing::twox_128("Broker".as_bytes());
 			let storage_hash = sp_io::hashing::twox_128("Regions".as_bytes());
 			let region_id_hash = sp_io::hashing::blake2_128(&region_id.encode());
@@ -238,32 +243,40 @@ pub mod pallet {
 			let key = [pallet_hash, storage_hash, region_id_hash, region_id_encoded].concat();
 
 			let coretime_chain_height =
-				T::StateMachineHeightProvider::get_latest_state_machine_height(StateMachineId {
+				T::StateMachineHeightProvider::latest_state_machine_height(StateMachineId {
 					state_id: T::CoretimeChain::get(),
 					consensus_state_id: PARACHAIN_CONSENSUS_ID,
-				});
+				})
+				.ok_or(Error::<T>::LatestHeightInaccessible)?;
 
 			// TODO: should requests be coupled in the future?
 			let get = DispatchGet {
 				dest: T::CoretimeChain::get(),
 				from: PALLET_ID.into(),
 				keys: vec![key],
-				height: coretime_chain_height,
+				// We require data following the cross-chain transfer, which will be available in
+				// the subsequent block. However, if the core time chain has a block production rate
+				// of 6 seconds, we will only have the commitment from the block after the next one.
+				height: coretime_chain_height.saturating_add(2),
 				timeout: T::Timeout::get(),
 			};
 
 			let dispatcher = T::IsmpDispatcher::default();
 
-			dispatcher
+			let commitment = dispatcher
 				.dispatch_request(
 					DispatchRequest::Get(get),
 					FeeMetadata { payer: who.clone(), fee: Zero::zero() },
 				)
 				.map_err(|_| Error::<T>::IsmpDispatchError)?;
 
-			Self::deposit_event(Event::RegionRecordRequested { region_id, account: who });
+			Self::deposit_event(Event::RegionRecordRequested {
+				region_id,
+				account: who,
+				request_commitment: commitment,
+			});
 
-			Ok(())
+			Ok(commitment)
 		}
 	}
 }
@@ -293,10 +306,10 @@ impl<T: Config> IsmpModule for IsmpModuleCallback<T> {
 					let mut region_id_encoded = &key[max(0, key.len() as isize - 16) as usize..];
 
 					let region_id = RegionId::decode(&mut region_id_encoded)
-						.map_err(|_| IsmpCustomError::DecodeFailed)?;
+						.map_err(|_| IsmpCustomError::KeyDecodeFailed)?;
 
 					let record = RegionRecordOf::<T>::decode(&mut value.as_slice())
-						.map_err(|_| IsmpCustomError::DecodeFailed)?;
+						.map_err(|_| IsmpCustomError::ResponseDecodeFailed)?;
 
 					crate::Pallet::<T>::set_record(region_id, record)
 						.map_err(|e| IsmpError::Custom(format!("{:?}", e)))?;
@@ -316,7 +329,7 @@ impl<T: Config> IsmpModule for IsmpModuleCallback<T> {
 				let mut region_id_encoded = &key[max(0, key.len() as isize - 16) as usize..];
 
 				let region_id = RegionId::decode(&mut region_id_encoded)
-					.map_err(|_| IsmpCustomError::DecodeFailed)?;
+					.map_err(|_| IsmpCustomError::KeyDecodeFailed)?;
 
 				let Some(mut region) = Regions::<T>::get(region_id) else {
 					return Err(IsmpCustomError::RegionNotFound.into());

pallets/regions/src/mock.rs

@@ -90,8 +90,8 @@ parameter_types! {
 
 pub struct MockStateMachineHeightProvider;
 impl StateMachineHeightProvider for MockStateMachineHeightProvider {
-	fn get_latest_state_machine_height(_id: StateMachineId) -> u64 {
-		0
+	fn latest_state_machine_height(_id: StateMachineId) -> Option<u64> {
+		Some(0)
 	}
 }
 

pallets/regions/src/nonfungible_impls.rs

@@ -57,7 +57,7 @@ impl<T: Config> Mutate<T::AccountId> for Pallet<T> {
 
 		// We don't want the region to get trapped, so we will handle the error.
 		let record = match Self::do_request_region_record(region_id, who.clone()) {
-			Ok(_) => Record::Pending,
+			Ok(commitment) => Record::Pending(commitment),
 			Err(_) => {
 				log::error!(
 					target: LOG_TARGET,

pallets/regions/src/primitives.rs

@@ -17,5 +17,5 @@ use ismp::consensus::StateMachineId;
 
 pub trait StateMachineHeightProvider {
 	/// Return the latest height of the state machine
-	fn get_latest_state_machine_height(id: StateMachineId) -> u64;
+	fn latest_state_machine_height(id: StateMachineId) -> Option<u64>;
 }

pallets/regions/src/tests.rs

@@ -41,7 +41,7 @@ fn nonfungibles_implementation_works() {
 		assert_ok!(Regions::mint_into(&region_id.into(), &2));
 		assert_eq!(
 			Regions::regions(&region_id).unwrap(),
-			Region { owner: 2, record: Record::Pending }
+			Region { owner: 2, record: Record::Pending(Default::default()) }
 		);
 
 		// The user is not required to set the region record to withdraw the asset back to the coretime
@@ -132,7 +132,12 @@ fn request_region_record_works() {
 		assert_eq!(key_prefix, REGION_PREFIX_KEY);
 
 		System::assert_last_event(
-			Event::<Test>::RegionRecordRequested { region_id, account: 1 }.into(),
+			Event::<Test>::RegionRecordRequested {
+				region_id,
+				account: 1,
+				request_commitment: Default::default(),
+			}
+			.into(),
 		);
 	});
 }
@@ -177,7 +182,7 @@ fn on_response_works() {
 		assert_ok!(Regions::mint_into(&region_id.into(), &2));
 		assert_eq!(
 			Regions::regions(&region_id).unwrap(),
-			Region { owner: 2, record: Record::Pending }
+			Region { owner: 2, record: Record::Pending(Default::default()) }
 		);
 
 		let request = &requests()[0];
@@ -212,7 +217,7 @@ fn on_response_works() {
 					Some(mock_record.clone().encode())
 				)]),
 			})),
-			IsmpCustomError::DecodeFailed
+			IsmpCustomError::KeyDecodeFailed
 		);
 
 		// Fails when invalid region record is passed as response:
@@ -221,7 +226,7 @@ fn on_response_works() {
 				get: get.clone(),
 				values: BTreeMap::from([(get.keys[0].clone(), Some(vec![0x42; 20]))]),
 			})),
-			IsmpCustomError::DecodeFailed
+			IsmpCustomError::ResponseDecodeFailed
 		);
 	});
 }
@@ -257,7 +262,7 @@ fn on_timeout_works() {
 		assert_ok!(Regions::mint_into(&region_id.into(), &2));
 		assert_eq!(
 			Regions::regions(&region_id).unwrap(),
-			Region { owner: 2, record: Record::Pending }
+			Region { owner: 2, record: Record::Pending(Default::default()) }
 		);
 
 		let request = &requests()[0];
@@ -277,7 +282,7 @@ fn on_timeout_works() {
 		invalid_get_req.keys.push(vec![0u8; 15]);
 		assert_err!(
 			module.on_timeout(Timeout::Request(Request::Get(invalid_get_req.clone()))),
-			IsmpCustomError::DecodeFailed
+			IsmpCustomError::KeyDecodeFailed
 		);
 
 		// invalid id: region not found

pallets/regions/src/types.rs

@@ -18,6 +18,7 @@ use codec::{Decode, Encode, MaxEncodedLen};
 use frame_support::traits::fungible::Inspect;
 use pallet_broker::RegionRecord;
 use scale_info::{prelude::format, TypeInfo};
+use sp_core::H256;
 
 pub type BalanceOf<T> = <<T as crate::Config>::NativeCurrency as Inspect<
 	<T as frame_system::Config>::AccountId,
@@ -29,7 +30,9 @@ pub type RegionRecordOf<T> = RegionRecord<<T as frame_system::Config>::AccountId
 #[scale_info(skip_type_params(T))]
 pub enum Record<T: crate::Config> {
 	/// An ISMP request was made to query the region record and we are now anticipating a response.
-	Pending,
+	///
+	/// The hash represents the commitment of the ISMP get request.
+	Pending(H256),
 	/// An ISMP request was made, but we failed to get a response.
 	Unavailable,
 	/// Successfully retrieved the region record.
@@ -38,7 +41,7 @@ pub enum Record<T: crate::Config> {
 
 impl<T: crate::Config> Record<T> {
 	pub fn is_pending(&self) -> bool {
-		matches!(self, Record::Pending)
+		matches!(self, Record::Pending(_))
 	}
 
 	pub fn is_unavailable(&self) -> bool {
@@ -76,8 +79,10 @@ pub struct Region<T: crate::Config> {
 pub enum IsmpCustomError {
 	/// Operation not supported.
 	NotSupported,
-	/// Failed to decode data.
-	DecodeFailed,
+	/// Failed to decode ismp request key.
+	KeyDecodeFailed,
+	/// Failed to decode ismp response.
+	ResponseDecodeFailed,
 	/// Couldn't find the region with the associated `RegionId`
 	RegionNotFound,
 	/// Couldn't find the corresponding value of a key in the ISMP result.
@@ -90,7 +95,8 @@ impl core::fmt::Display for IsmpCustomError {
 	fn fmt(&self, f: &mut core::fmt::Formatter<'_>) -> core::fmt::Result {
 		match self {
 			Self::NotSupported => write!(f, "NotSupported"),
-			Self::DecodeFailed => write!(f, "DecodeFailed"),
+			Self::KeyDecodeFailed => write!(f, "KeyDecodeFailed"),
+			Self::ResponseDecodeFailed => write!(f, "ResponseDecodeFailed"),
 			Self::RegionNotFound => write!(f, "RegionNotFound"),
 			Self::ValueNotFound => write!(f, "ValueNotFound"),
 			Self::EmptyValue => write!(f, "EmptyValue"),

runtime/regionx/src/lib.rs

@@ -596,8 +596,8 @@ impl pallet_collator_selection::Config for Runtime {
 
 pub struct StateMachineHeightProvider;
 impl StateMachineHeightProviderT for StateMachineHeightProvider {
-	fn get_latest_state_machine_height(id: StateMachineId) -> u64 {
-		Ismp::latest_state_machine_height(id).unwrap_or(0) // TODO
+	fn latest_state_machine_height(id: StateMachineId) -> Option<u64> {
+		Ismp::latest_state_machine_height(id)
 	}
 }
 
@@ -612,7 +612,7 @@ impl pallet_regions::Config for Runtime {
 	type CoretimeChain = CoretimeChain;
 	type IsmpDispatcher = Ismp;
 	type StateMachineHeightProvider = StateMachineHeightProvider;
-	type Timeout = ConstU64<1000>; // TODO: FIXME
+	type Timeout = ConstU64<300>; // 5 minutes
 	type WeightInfo = ();
 }