Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

RouterV2 #117

Merged
merged 34 commits into from
Sep 26, 2024
Merged

RouterV2 #117

merged 34 commits into from
Sep 26, 2024

Conversation

JanKuczma
Copy link
Contributor

@JanKuczma JanKuczma commented Sep 3, 2024

The RouterV2 contract supports swaps along paths that include Pair and StablePool types of pools.

Swap Path

Each swap* method takes as the arguments path: Vec<Step> and token_out: AccountId, except for the swap*_for_native - here we know that the token_out is always the wrapped_native token.

The Step struct has two fields: Step { token_in: AccountId, pool_id: AccountId }. A valid path must consist of at least one Step.

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

        pool_1          pool_2
       /      \        /      \ 
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 via the Factory contract.

In the new Router, when adding liquidity to a Pair pool, the caller has an option to provide the Pair address. If the address is not provided, the contract calls the Factory to create a new Pair. It fails if Pair for given tokens already exists.

The new Router caches pools solely for optimization purposes. It is allowed to swap through any valid pool contract (Pair or StablePool). The pools are cached when swapping through or managing liquidity in a valid pool (permissionless).

@krzysztofziobro krzysztofziobro self-requested a review September 3, 2024 09:46
@ggawryal ggawryal self-requested a review September 3, 2024 09:48
@JanKuczma JanKuczma marked this pull request as ready for review September 4, 2024 08:53
Copy link
Contributor

@ggawryal ggawryal left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Generally looks good, but I'm not sure the calculate_amounts_in is correct.
Also, as you noted, adding liquidity to stable pools with wrapping requires two actions, so I'm afraid it has to be added there

amm/contracts/router_v2/pair.rs Outdated Show resolved Hide resolved
amm/contracts/router_v2/pair.rs Show resolved Hide resolved
amm/contracts/router_v2/lib.rs Outdated Show resolved Hide resolved
amm/contracts/router_v2/lib.rs Outdated Show resolved Hide resolved
amm/contracts/router_v2/lib.rs Outdated Show resolved Hide resolved
amm/drink-tests/src/router_v2_tests.rs Outdated Show resolved Hide resolved
amm/contracts/router_v2/pool.rs Outdated Show resolved Hide resolved
Copy link
Contributor

@krzysztofziobro krzysztofziobro left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

One general idea (as discussed offline): it seems that caching in the current form is largely unnecessary and adds complexity - since we get AccountId in each Step path anyway there should be no need to keep a permissioned record of pools (as long as we are able to distinguish between pair and stable pool using on-chain data eg making a cross-contract call).

It might be advantageous to create an automatic cache, which role would be to limit number of cross-contract calls, but would not require contract owner and would be simple on the implementation side.

For adding liquidity to pair contract we can create an interface that takes Option<AccountId> of the pool - if None, then new pair would be created.

@ggawryal what do you think?

///
/// Starts with `amount_in` and token address under `path[0].token_in`.
///
/// If `path[i].pool_id` is `None`, it attemps the swap through
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

From what I see Step now has no Option field

let wazero = wazero::setup(&mut session);
let router = router_v2::setup(&mut session, factory.into(), wazero.into());

let initial_reserves = vec![100000 * ONE_USDT, 100000 * ONE_USDC];
Copy link
Contributor

@krzysztofziobro krzysztofziobro Sep 5, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Would suggest to create constants instead of using numerical values directly whenever possible

vec![],
);

_ = session
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggestion: not to use _ = , if it's an error then it should be checked/unwrapped, if it's just an unused value then ignore it altogether

Suggested change
_ = session
session

@ggawryal
Copy link
Contributor

ggawryal commented Sep 6, 2024

there should be no need to keep a permissioned record of pools

Agreed. Decoding the pool type might be a bit harder, but even if we had to add an extra flag to the Step returned from the pathfinder and frontend, it would be worth it imho.

It might be advantageous to create an automatic cache, which role would be to limit number of cross-contract calls, but would not require contract owner and would be simple on the implementation side.

It might, but currently cache doesn't limit anything, it only helps to find the correct pool address for a given pair of tokens, right? If so, I wouldn't bother.

Copy link
Contributor

@ggawryal ggawryal left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM!

amm/contracts/router_v2/lib.rs Outdated Show resolved Hide resolved
Copy link
Contributor

@krzysztofziobro krzysztofziobro left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

lgtm

amm/contracts/router_v2/pair.rs Outdated Show resolved Hide resolved
amm/contracts/router_v2/stable_pool.rs Outdated Show resolved Hide resolved
amm/drink-tests/src/utils.rs Outdated Show resolved Hide resolved
amm/contracts/router_v2/lib.rs Outdated Show resolved Hide resolved
amm/contracts/router_v2/lib.rs Outdated Show resolved Hide resolved
@JanKuczma JanKuczma merged commit 0f50a8d into main Sep 26, 2024
1 check passed
@JanKuczma JanKuczma deleted the router-v2 branch September 26, 2024 09:55
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants