Merge pull request #1452 from galacticcouncil/fix/extrinsic-index

fix: account weight for trade with no route

Author: dmoka

Cargo.lock

@@ -1,6 +1,6 @@
 [package]
 name = "runtime-integration-tests"
-version = "1.82.0"
+version = "1.83.0"
 description = "Integration tests"
 authors = ["GalacticCouncil"]
 edition = "2021"

integration-tests/Cargo.toml

@@ -5740,3 +5740,140 @@ fn populate_oracle(
 	));
 	go_to_block(block.unwrap_or(10));
 }
+
+mod weight_uses_resolved_route {
+	use super::*;
+	use frame_support::dispatch::GetDispatchInfo;
+
+	fn stored_multi_hop_route() -> Vec<Trade<AssetId>> {
+		vec![
+			Trade {
+				pool: PoolType::Omnipool,
+				asset_in: HDX,
+				asset_out: DAI,
+			},
+			Trade {
+				pool: PoolType::XYK,
+				asset_in: DAI,
+				asset_out: DOT,
+			},
+			Trade {
+				pool: PoolType::Omnipool,
+				asset_in: DOT,
+				asset_out: ETH,
+			},
+		]
+	}
+
+	#[test]
+	fn sell_weight_should_reflect_stored_route_when_input_route_is_empty() {
+		TestNet::reset();
+		Hydra::execute_with(|| {
+			let stored = stored_multi_hop_route();
+			assert_ok!(Router::force_insert_route(
+				RuntimeOrigin::root(),
+				Pair::new(HDX, ETH),
+				BoundedVec::truncate_from(stored.clone()),
+			));
+
+			let call = pallet_route_executor::Call::<Runtime>::sell {
+				asset_in: HDX,
+				asset_out: ETH,
+				amount_in: UNITS,
+				min_amount_out: 0,
+				route: BoundedVec::new(),
+			};
+
+			let charged = call.get_dispatch_info().call_weight;
+			let expected = RouterWeightInfo::sell_weight(&stored);
+			assert_eq!(charged, expected);
+		});
+	}
+
+	#[test]
+	fn buy_weight_should_reflect_stored_route_when_input_route_is_empty() {
+		TestNet::reset();
+		Hydra::execute_with(|| {
+			let stored = stored_multi_hop_route();
+			assert_ok!(Router::force_insert_route(
+				RuntimeOrigin::root(),
+				Pair::new(HDX, ETH),
+				BoundedVec::truncate_from(stored.clone()),
+			));
+
+			let call = pallet_route_executor::Call::<Runtime>::buy {
+				asset_in: HDX,
+				asset_out: ETH,
+				amount_out: UNITS,
+				max_amount_in: u128::MAX,
+				route: BoundedVec::new(),
+			};
+
+			let charged = call.get_dispatch_info().call_weight;
+			let expected = RouterWeightInfo::buy_weight(&stored);
+			assert_eq!(charged, expected);
+		});
+	}
+
+	#[test]
+	fn sell_all_weight_should_reflect_stored_route_when_input_route_is_empty() {
+		TestNet::reset();
+		Hydra::execute_with(|| {
+			let stored = stored_multi_hop_route();
+			assert_ok!(Router::force_insert_route(
+				RuntimeOrigin::root(),
+				Pair::new(HDX, ETH),
+				BoundedVec::truncate_from(stored.clone()),
+			));
+
+			let call = pallet_route_executor::Call::<Runtime>::sell_all {
+				asset_in: HDX,
+				asset_out: ETH,
+				min_amount_out: 0,
+				route: BoundedVec::new(),
+			};
+
+			let charged = call.get_dispatch_info().call_weight;
+			let expected = RouterWeightInfo::sell_weight(&stored);
+			assert_eq!(charged, expected);
+		});
+	}
+
+	#[test]
+	fn sell_weight_should_be_higher_with_stored_multi_hop_route_than_without() {
+		TestNet::reset();
+		Hydra::execute_with(|| {
+			let call_without_stored = pallet_route_executor::Call::<Runtime>::sell {
+				asset_in: HDX,
+				asset_out: ETH,
+				amount_in: UNITS,
+				min_amount_out: 0,
+				route: BoundedVec::new(),
+			};
+			let weight_without_stored = call_without_stored.get_dispatch_info().call_weight;
+
+			assert_ok!(Router::force_insert_route(
+				RuntimeOrigin::root(),
+				Pair::new(HDX, ETH),
+				BoundedVec::truncate_from(stored_multi_hop_route()),
+			));
+
+			let call_with_stored = pallet_route_executor::Call::<Runtime>::sell {
+				asset_in: HDX,
+				asset_out: ETH,
+				amount_in: UNITS,
+				min_amount_out: 0,
+				route: BoundedVec::new(),
+			};
+			let weight_with_stored = call_with_stored.get_dispatch_info().call_weight;
+
+			assert!(
+				weight_with_stored.ref_time() > weight_without_stored.ref_time(),
+				"expected stored-route call to charge more ref_time than no-stored-route call \
+				 (got {} vs {})",
+				weight_with_stored.ref_time(),
+				weight_without_stored.ref_time(),
+			);
+		});
+	}
+}

integration-tests/src/router.rs

@@ -1,6 +1,6 @@
 [package]
 name = "pallet-route-executor"
-version = "2.12.0"
+version = "2.12.1"
 description = "A pallet to execute a route containing a sequence of trades"
 authors = ["GalacticCouncil"]
 edition = "2021"
@@ -35,7 +35,6 @@ sp-io = { workspace = true }
 pretty_assertions = { workspace = true }
 orml-tokens = { workspace = true }
 pallet-currencies = { workspace = true }
-hydradx-adapters = { workspace = true }
 test-utils = { workspace = true }
 
 [features]
@@ -54,9 +53,9 @@ std = [
   "frame-system/std",
   "orml-tokens/std",
   "orml-traits/std",
-  "hydradx-adapters/std",
   "pallet-balances/std",
   "pallet-broadcast/std",
+  "pallet-currencies/std",
   "hydradx-traits/std",
   "hydra-dx-math/std",
   "frame-benchmarking/std",

pallets/route-executor/Cargo.toml

@@ -189,7 +189,7 @@ pub mod pallet {
 		///
 		/// Emits `RouteExecuted` when successful.
 		#[pallet::call_index(0)]
-		#[pallet::weight(T::WeightInfo::sell_weight(route))]
+		#[pallet::weight(T::WeightInfo::sell_weight(&Pallet::<T>::get_route_or_default(*asset_in, *asset_out, route)))]
 		#[transactional]
 		pub fn sell(
 			origin: OriginFor<T>,
@@ -216,7 +216,7 @@ pub mod pallet {
 		///
 		/// Emits `RouteExecuted` when successful.
 		#[pallet::call_index(1)]
-		#[pallet::weight(T::WeightInfo::buy_weight(route))]
+		#[pallet::weight(T::WeightInfo::buy_weight(&Pallet::<T>::get_route_or_default(*asset_in, *asset_out, route)))]
 		#[transactional]
 		pub fn buy(
 			origin: OriginFor<T>,
@@ -232,7 +232,7 @@ pub mod pallet {
 			Self::ensure_route_size(route.len())?;
 
 			let asset_pair = AssetPair::new(asset_in, asset_out);
-			let route = Self::get_route_or_default(route, asset_pair)?;
+			let route = Self::get_route_or_default(asset_in, asset_out, &route);
 			Self::ensure_route_arguments(&asset_pair, &route)?;
 
 			let trade_amounts = Self::calculate_buy_trade_amounts(&route, amount_out)?;
@@ -423,7 +423,7 @@ pub mod pallet {
 		/// Emits `RouteExecuted` when successful.
 		///
 		#[pallet::call_index(4)]
-		#[pallet::weight(T::WeightInfo::sell_weight(route))]
+		#[pallet::weight(T::WeightInfo::sell_weight(&Pallet::<T>::get_route_or_default(*asset_in, *asset_out, route)))]
 		#[transactional]
 		pub fn sell_all(
 			origin: OriginFor<T>,
@@ -460,7 +460,7 @@ impl<T: Config> Pallet<T> {
 		Self::ensure_route_size(route.len())?;
 
 		let asset_pair = AssetPair::new(asset_in, asset_out);
-		let route = Self::get_route_or_default(route, asset_pair)?;
+		let route = Self::get_route_or_default(asset_in, asset_out, &route);
 		Self::ensure_route_arguments(&asset_pair, &route)?;
 
 		let trader_account = Self::router_account();
@@ -558,16 +558,16 @@ impl<T: Config> Pallet<T> {
 		Ok(())
 	}
 
-	fn get_route_or_default(
-		route: Route<T::AssetId>,
-		asset_pair: AssetPair<T::AssetId>,
-	) -> Result<Route<T::AssetId>, DispatchError> {
-		let route = if !route.is_empty() {
-			route
+	pub fn get_route_or_default(
+		asset_in: T::AssetId,
+		asset_out: T::AssetId,
+		route: &Route<T::AssetId>,
+	) -> Route<T::AssetId> {
+		if !route.is_empty() {
+			route.clone()
 		} else {
-			<Pallet<T> as RouteProvider<T::AssetId>>::get_route(asset_pair)
-		};
-		Ok(route)
+			<Self as RouteProvider<T::AssetId>>::get_route(AssetPair::new(asset_in, asset_out))
+		}
 	}
 
 	fn validate_route(route: &Route<T::AssetId>) -> Result<(T::Balance, T::Balance), DispatchError> {

pallets/route-executor/src/lib.rs

@@ -1,6 +1,6 @@
 [package]
 name = "hydradx-runtime"
-version = "417.0.0"
+version = "418.0.0"
 authors = ["GalacticCouncil"]
 edition = "2021"
 license = "Apache 2.0"

runtime/hydradx/Cargo.toml

@@ -128,7 +128,7 @@ pub const VERSION: RuntimeVersion = RuntimeVersion {
 	spec_name: Cow::Borrowed("hydradx"),
 	impl_name: Cow::Borrowed("hydradx"),
 	authoring_version: 1,
-	spec_version: 417,
+	spec_version: 418,
 	impl_version: 0,
 	apis: RUNTIME_API_VERSIONS,
 	transaction_version: 1,