Support swaps with stablepool #114

JanKuczma · 2024-08-21T08:59:51Z

The new Router contract will support swaps along paths that include Pair and StablePool types of pools.

Swap Path

The path is a vector of Steps and must consist of at least 2 Steps. Each step is a struct Step(PoolId, TokenId). The TokenId describes the token in of the swap for the given PoolId, except for the last Step - the last Step in the path always describes the token out. The PoolId of the last step is ignored.

E.g path [(pool_1, token_a) , (pool_2, token_b) , (pool_x, token_c)] can be visualized like this:

        pool_1          pool_2          pool_x - ignored
       /      \        /      \ 
token_a   ->   token_b    ->    token_c

Compatibility

The old Router had a "pair caching" mechanism which allowed caching custom Pair contracts. Also, when adding liquidity to a non-existent pair, the pair was created through the Factory contract.

To keep these features, the new Router has to cache the StablePool contracts before they can be used for swaps.
When swapping, the Router checks for the pool in the following order:

Cached pools by PoolId
Cached pairs by (token_in,token_out)
Pairs created through Factory by (token_in,token_out)

deuszx

Let's start with a general comment – I'd add a new router alongside the old one, not replace its code. Call it however you want (maybe even RouterV2) but let's not replace the current code (including traits). The Router(v1) is already deployed and live, we have its metadata (artifacts) committed to the repo, with deployment addresses, so let's not "erase" its existence from the main.

deuszx

Some small errors around caching.

deuszx · 2024-08-23T11:27:25Z

amm/contracts/router/pool.rs

+                let (reserve_0, reserve_1, _) = pair.get_reserves();
+                (vec![reserve_0, reserve_1], *fee)
+            }
+            Pool::StablePool(_, _) => unimplemented!(),


Can you add here a comment with the reason for which it's not implemented? Right now it looks like a temporary solution.

deuszx · 2024-08-23T11:54:29Z

amm/contracts/router/pool.rs

+};
+use traits::{MathError, Pair, RouterError, StablePool};
+
+const TRADING_FEE_DENOM: u128 = 1000;


Suggested change

const TRADING_FEE_DENOM: u128 = 1000;

const TRADING_FEE_DENOM_PAIR: u128 = 1000;

(Or however you prefer) but let's make sure it's clear that this is for V2 style pools only.

deuszx · 2024-08-23T11:55:08Z

amm/contracts/router/pool.rs

+        }
+    }
+
+    pub fn get_reserves_with_fee(&self) -> (Vec<u128>, u8) {


Does it have to be pub? You're risking caller using this on a Pool::Stable which has unimplemented!().

deuszx · 2024-08-23T11:56:55Z

amm/contracts/router/lib.rs

+            let pool_ref: contract_ref!(StablePool) = pool_id.into();
+            let tokens = pool_ref.tokens();


Which line will fail if pool_id turns out to not be a Stable pool? ie pool_ref.tokens() should then return an error, right?

Yes, if pool_id does not implement StablePool then that line should panic

deuszx · 2024-08-23T12:00:31Z

amm/contracts/router/lib.rs

+                cached_pairs: Default::default(),
+                cached_pools: Mapping::default(),


nit: let's make it consistent.

amm/contracts/router/lib.rs

deuszx · 2024-08-23T12:45:53Z

amm/contracts/router/lib.rs

+                match pool {
+                    Pool::Pair(tokens, _, _) => {
+                        self.cached_pairs.remove(tokens);
+                        self.cached_pairs.remove(tokens);


I think you need to destructure the tuple and remove twice – switching the order.

krzysztofziobro

Haven't read the whole thing yet, first few things that caught my eye

krzysztofziobro · 2024-08-23T13:41:51Z

amm/drink-tests/src/router_tests.rs

@@ -220,7 +220,10 @@ fn test_fees(mut session: Session) {
        .execute(router.swap_exact_tokens_for_tokens(
            swap_amount,
            0,
-            vec![ice.into(), wood.into()],
+            vec![
+                (ice_wood_pair.into(), ice.into()),


Unless I missed something, there are no new tests for interacting with stable pools - I think it's important to include some.

krzysztofziobro · 2024-08-23T13:47:55Z

amm/traits/router.rs

@@ -87,7 +92,7 @@ pub trait Router {
        deadline: u64,
    ) -> Result<(u128, Balance), RouterError>;

-    /// Exchanges tokens along `path` tokens.
+    /// Exchanges tokens along `path` pools and tokens.
    /// Starts with `amount_in` and pair under `(path[0], path[1])` address.


Looks like the comment is still a bit outdated

it should be explained what are the expected values (eg. you could add a new type Path and explain that there), since without reading the code I wouldn't know whether its [(unused_pool, token_1), (pool_1, token_2)] or [(pool_1, token_1), (token_2, unused_pool)] - for me the first version seems more intuitive, but turns out it's wrong.

would be nice to have a mention about what the return value is supposed to represent

krzysztofziobro · 2024-08-23T13:50:07Z

amm/traits/router.rs

@@ -96,12 +101,12 @@ pub trait Router {
        &mut self,
        amount_in: u128,
        amount_out_min: u128,
-        path: Vec<AccountId>,
+        path: Vec<Step>,


I think that with this type you need to pass at least one "unused" pool address, I would change it to either:

path: Vec<Step> + first_token: AccountId, or

make Step field holding the pool Option<AccountId>

Agree. Imho the path: Vec<Step>, token_in: AccountId is cleaner and easier to understand.

krzysztofziobro · 2024-08-23T13:52:49Z

amm/traits/router.rs

@@ -185,45 +190,13 @@ pub trait Router {
    #[ink(message)]
    fn quote(&self, amount_0: u128, reserve_0: u128, reserve_1: u128) -> Result<u128, RouterError>;


Seems weird for me to have this as a message when we have different pool types - do you know if we are currently using it somewhere?

Agree. I'm not sure if this method is used somewhere by FE but I doubt it (it does a simple calculation which can be done off-chain).

JanKuczma · 2024-09-03T08:49:46Z

The development continued in #117.

support swaps with stablepool

a4e21fb

JanKuczma requested a review from deuszx August 21, 2024 08:59

JanKuczma added 2 commits August 21, 2024 17:15

Merge branch 'main' into new-router

2f6f8f8

fix router tests

65e1203

JanKuczma marked this pull request as ready for review August 22, 2024 13:50

fix amounts in calculation

69a7e00

deuszx suggested changes Aug 23, 2024

View reviewed changes

krzysztofziobro reviewed Aug 23, 2024

View reviewed changes

JanKuczma closed this Sep 3, 2024

JanKuczma deleted the new-router branch November 12, 2024 17:41

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Support swaps with stablepool #114

Support swaps with stablepool #114

JanKuczma commented Aug 21, 2024 •

edited

Loading

deuszx left a comment •

edited

Loading

deuszx left a comment

deuszx Aug 23, 2024

deuszx Aug 23, 2024

deuszx Aug 23, 2024

deuszx Aug 23, 2024

JanKuczma Aug 26, 2024

deuszx Aug 23, 2024

deuszx Aug 23, 2024

krzysztofziobro left a comment

krzysztofziobro Aug 23, 2024

krzysztofziobro Aug 23, 2024 •

edited

Loading

krzysztofziobro Aug 23, 2024

JanKuczma Aug 26, 2024

krzysztofziobro Aug 23, 2024

JanKuczma Aug 26, 2024

JanKuczma commented Sep 3, 2024

	const TRADING_FEE_DENOM: u128 = 1000;
	const TRADING_FEE_DENOM_PAIR: u128 = 1000;

		let pool_ref: contract_ref!(StablePool) = pool_id.into();
		let tokens = pool_ref.tokens();

		cached_pairs: Default::default(),
		cached_pools: Mapping::default(),

		@@ -185,45 +190,13 @@ pub trait Router {
		#[ink(message)]
		fn quote(&self, amount_0: u128, reserve_0: u128, reserve_1: u128) -> Result<u128, RouterError>;

Support swaps with stablepool #114

Support swaps with stablepool #114

Conversation

JanKuczma commented Aug 21, 2024 • edited Loading

Swap Path

Compatibility

deuszx left a comment • edited Loading

Choose a reason for hiding this comment

deuszx left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

krzysztofziobro left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

krzysztofziobro Aug 23, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

JanKuczma commented Sep 3, 2024

JanKuczma commented Aug 21, 2024 •

edited

Loading

deuszx left a comment •

edited

Loading

krzysztofziobro Aug 23, 2024 •

edited

Loading