From 516719f1517eb29e8d10410c97f93e4df128258a Mon Sep 17 00:00:00 2001 From: Dusan Stanivukovic Date: Mon, 29 Jul 2024 11:11:37 +0200 Subject: [PATCH] Fix scaling factor in driver (#2840) # Description Furucombo people noticed the scaling of prices in the driver is not working properly. Example auction: 9172009 [Example log](https://production-6de61f.kb.eu-central-1.aws.cloud.es.io/app/discover#/doc/c0e240e0-d9b3-11ed-b0e6-e361adffce0b/cowlogs-prod-2024.07.23?id=cpXk3JABP0v0FZqw807z) of two solutions that should have been merged. The `fn scaling_factor` function description is correct: > /// Given two solutions returns the factors with > /// which prices of the second solution would have to be multiplied so that the > /// given token would have the same price in both solutions. which means, we need to calculate `factor = first_price / second_price` so that `factor` represents the number with which `second_price` should be multiplied to get `first_price` --- .../src/domain/competition/solution/mod.rs | 2 +- .../src/tests/cases/merge_settlements.rs | 24 +++++++++++ crates/driver/src/tests/cases/mod.rs | 3 ++ crates/driver/src/tests/setup/mod.rs | 43 ++++++++++++++++++- 4 files changed, 69 insertions(+), 3 deletions(-) diff --git a/crates/driver/src/domain/competition/solution/mod.rs b/crates/driver/src/domain/competition/solution/mod.rs index 26d133d112..c2ca52a0c0 100644 --- a/crates/driver/src/domain/competition/solution/mod.rs +++ b/crates/driver/src/domain/competition/solution/mod.rs @@ -495,8 +495,8 @@ fn scaling_factor(first: &Prices, second: &Prices) -> Option { let first_price = first[token]; let second_price = second[token]; BigRational::new( - number::conversions::u256_to_big_int(&second_price), number::conversions::u256_to_big_int(&first_price), + number::conversions::u256_to_big_int(&second_price), ) }) .collect(); diff --git a/crates/driver/src/tests/cases/merge_settlements.rs b/crates/driver/src/tests/cases/merge_settlements.rs index b110a0e964..dc3293f612 100644 --- a/crates/driver/src/tests/cases/merge_settlements.rs +++ b/crates/driver/src/tests/cases/merge_settlements.rs @@ -5,6 +5,9 @@ use crate::tests::{ ab_order, ab_pool, ab_solution, + ad_order, + ad_pool, + ad_solution, cd_order, cd_pool, cd_solution, @@ -45,6 +48,27 @@ async fn possible() { .await; } +/// Test that settlements can be merged with two solutions containing a common +/// token. +#[tokio::test] +#[ignore] +async fn possible_common_token() { + let ab_order = ab_order(); + let ad_order = ad_order(); + let test: Test = setup::setup() + .solvers(vec![test_solver().merge_solutions()]) + .pool(ab_pool()) + .pool(ad_pool()) + .order(ab_order.clone()) + .order(ad_order.clone()) + .solution(ab_solution()) + .solution(ad_solution()) + .done() + .await; + + test.solve().await.ok().orders(&[ab_order, ad_order]); +} + /// Test that settlements are not merged if the clearing prices don't permit it. #[tokio::test] #[ignore] diff --git a/crates/driver/src/tests/cases/mod.rs b/crates/driver/src/tests/cases/mod.rs index f7b94467d7..915a64b4c0 100644 --- a/crates/driver/src/tests/cases/mod.rs +++ b/crates/driver/src/tests/cases/mod.rs @@ -38,6 +38,9 @@ pub const DEFAULT_POOL_AMOUNT_D: u64 = 6000; /// The order amount for orders selling token "A" for "B". pub const AB_ORDER_AMOUNT: u64 = 50; +/// The order amount for orders selling token "A" for "D". +pub const AD_ORDER_AMOUNT: u64 = 48; + /// The order amount for orders selling token "C" for "D". pub const CD_ORDER_AMOUNT: u64 = 40; diff --git a/crates/driver/src/tests/setup/mod.rs b/crates/driver/src/tests/setup/mod.rs index 9cdc20a60c..03adbc7d2a 100644 --- a/crates/driver/src/tests/setup/mod.rs +++ b/crates/driver/src/tests/setup/mod.rs @@ -14,8 +14,10 @@ use { }, tests::{ cases::{ + is_approximately_equal, EtherExt, AB_ORDER_AMOUNT, + AD_ORDER_AMOUNT, CD_ORDER_AMOUNT, DEFAULT_POOL_AMOUNT_A, DEFAULT_POOL_AMOUNT_B, @@ -627,6 +629,37 @@ pub fn ab_solution() -> Solution { } } +/// An example order which sells token "A" for token "D". +pub fn ad_order() -> Order { + Order { + name: "A-D order", + sell_amount: AD_ORDER_AMOUNT.ether().into_wei(), + sell_token: "A", + buy_token: "D", + ..Default::default() + } +} + +/// A pool between tokens "A" and "D". +pub fn ad_pool() -> Pool { + Pool { + token_a: "A", + token_b: "D", + amount_a: DEFAULT_POOL_AMOUNT_A.ether().into_wei(), + amount_b: DEFAULT_POOL_AMOUNT_D.ether().into_wei(), + } +} + +/// A solution solving the [`ad_order`]. +pub fn ad_solution() -> Solution { + Solution { + calldata: Calldata::Valid { + additional_bytes: 0, + }, + orders: vec!["A-D order"], + } +} + /// A pool between tokens "C" and "D". pub fn cd_pool() -> Pool { Pool { @@ -1175,8 +1208,14 @@ impl<'a> SolveOk<'a> { Some(executed_amounts) => (executed_amounts.sell, executed_amounts.buy), None => (quoted_order.sell, quoted_order.buy), }; - assert!(u256(trade.get("sellAmount").unwrap()) == expected_sell); - assert!(u256(trade.get("buyAmount").unwrap()) == expected_buy); + assert!(is_approximately_equal( + u256(trade.get("sellAmount").unwrap()), + expected_sell + )); + assert!(is_approximately_equal( + u256(trade.get("buyAmount").unwrap()), + expected_buy + )); } self }