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

feat(x/poolmanager): split routes swap message #4886

Merged
merged 26 commits into from
Apr 14, 2023
Merged

Conversation

p0mvn
Copy link
Member

@p0mvn p0mvn commented Apr 10, 2023

Closes: #4869

What is the purpose of the change

Implementing new swap message for split routes.

Each route can be thought of as a separate multi-hop swap.

Splitting swaps across multiple pools for the same token pair can be beneficial for several reasons,
primarily relating to reduced slippage, price impact, and potentially lower fees.

Here's a detailed explanation of these advantages:

  • Reduced slippage: When a large trade is executed in a single pool, it can be significantly affected if someone else executes a large swap against that pool.

  • Lower price impact: when executing a large trade in a single pool, the price impact can be substantial, leading to a less favorable exchange rate for the trader.
    By splitting the swap across multiple pools, the price impact in each pool is minimized, resulting in a better overall exchange rate.

  • Improved liquidity utilization: Different pools may have varying levels of liquidity, fees, and price curves. By splitting swaps across multiple pools,
    the router can utilize liquidity from various sources, allowing for more efficient execution of trades. This is particularly useful when the liquidity in
    a single pool is not sufficient to handle a large trade or when the price curve of one pool becomes less favorable as the trade size increases.

  • Potentially lower fees: In some cases, splitting swaps across multiple pools may result in lower overall fees. This can happen when different pools
    have different fee structures, or when the total fee paid across multiple pools is lower than the fee for executing the entire trade in a single pool with
    higher slippage.

Note, that the actual split happens off-chain. The router is only responsible for executing the swaps in the order and quantities of token in provided
by the routes.

Brief Changelog

  • generate message and helpers
  • add validate basic checks
  • implement message route
  • add happy path test

Testing and Verifying

This change is a trivial rework / code cleanup without any test coverage.

TODO

  • Add edge case tests
  • Add similar implementation for swap amount in given out
  • Clean up
  • Add changelog entry

Documentation and Release Note

  • Does this pull request introduce a new feature or user-facing behavior changes? yes
  • Is a relevant changelog entry added to the Unreleased section in CHANGELOG.md? TBD
  • How is the feature or change documented? not applicable

@p0mvn p0mvn changed the title feat(x/poolmanager): split routes message feat(x/poolmanager): split routes swap message Apr 10, 2023
@p0mvn p0mvn requested a review from jonator April 10, 2023 23:23
@p0mvn p0mvn added the V:state/compatible/no_backport State machine compatible PR, depends on prior breaks label Apr 10, 2023
Copy link
Member

@jonator jonator left a comment

Choose a reason for hiding this comment

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

A few nits, but overall looks good from a quick scan

proto/osmosis/poolmanager/v1beta1/tx.proto Outdated Show resolved Hide resolved
x/poolmanager/router.go Outdated Show resolved Hide resolved
x/poolmanager/types/msgs.go Outdated Show resolved Hide resolved
@jonator jonator self-requested a review April 12, 2023 03:23
Copy link
Member

@czarcas7ic czarcas7ic left a comment

Choose a reason for hiding this comment

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

Nice job on this, clear and well tested

proto/osmosis/poolmanager/v1beta1/tx.proto Show resolved Hide resolved
proto/osmosis/poolmanager/v1beta1/tx.proto Show resolved Hide resolved
x/poolmanager/router.go Show resolved Hide resolved
TokenInAmount: sdk.NewInt(twentyFiveBaseUnitsAmount.Int64() * 3),
}

priceImpactThreshold = sdk.NewInt(97866545)
Copy link
Member

Choose a reason for hiding this comment

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

Can you add a comment explaining where this number comes from? It made sense after I read through the tests, but would be nice to have the context prior to then

"valid solo route multi hop": {
routes: []types.SwapAmountInSplitRoute{defaultSingleRouteTwoHops},
tokenInDenom: foo,
tokenOutMinAmount: sdk.OneInt(),
Copy link
Member

Choose a reason for hiding this comment

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

asking for my own learning, what is significant about one here? Can this not be 0?

Copy link
Member Author

Choose a reason for hiding this comment

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

It can, just small value that we know well not trigger the price impact protection

x/poolmanager/router_test.go Outdated Show resolved Hide resolved
x/poolmanager/router_test.go Outdated Show resolved Hide resolved
x/poolmanager/router_test.go Outdated Show resolved Hide resolved
x/poolmanager/router_test.go Outdated Show resolved Hide resolved
@github-actions github-actions bot added the C:docs Improvements or additions to documentation label Apr 12, 2023
@p0mvn p0mvn marked this pull request as ready for review April 12, 2023 05:29
@p0mvn p0mvn requested a review from mattverse as a code owner April 12, 2023 05:30
Copy link
Member

@mattverse mattverse left a comment

Choose a reason for hiding this comment

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

LGTM

x/poolmanager/README.md Show resolved Hide resolved
x/poolmanager/router.go Outdated Show resolved Hide resolved
x/poolmanager/router_test.go Outdated Show resolved Hide resolved
@p0mvn p0mvn closed this Apr 14, 2023
@p0mvn p0mvn reopened this Apr 14, 2023
@p0mvn p0mvn merged commit 1c5f166 into main Apr 14, 2023
@p0mvn p0mvn deleted the roman/split-routes-msg branch April 14, 2023 06:01
@github-actions github-actions bot mentioned this pull request May 1, 2024
@github-actions github-actions bot mentioned this pull request May 15, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
C:docs Improvements or additions to documentation C:x/poolmanager V:state/compatible/no_backport State machine compatible PR, depends on prior breaks
Projects
None yet
Development

Successfully merging this pull request may close these issues.

feat: split route multi hop swap message
4 participants