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: add spot price provider for specified route #796

Merged
merged 91 commits into from
May 8, 2024

Conversation

dmoka
Copy link
Contributor

@dmoka dmoka commented Apr 2, 2024

Explanation for spot price calculations, including fees.

We calculate spot price based on sell trade as it should yield very similar result like buy, and it also simplifies the implementation

XYK:

  • Since in the trade the amount_out is reduced by fee, it makes asset_out more expensive, so the asset_in/asset_out spot price should be increased. So divide spot-price-without-fee by 1-fee to reflect correct amount out after the fee deduction

LBP:

  1. When distributed asset is bought, it is very similar to XYK, so the fee is deducted from asset_out, so divide spot-price-without-fee by 1-fee
  2. When accumulated asset is bought, fee does not change spot price as user receives the whole amount_out calculated based on whole amount_in. So the result should be the same to spot_price_without_fee

Omnipool

  1. When selling LRNA, no protocol fee is involved. Fee is taken from asset_out, so we need to increase the asset_in/asset_out spot price with dividing by 1-asset_fee to reflect correct amount_out after the fee deduction
  2. When trading any other assets, Both protocol fee and asset fee reduce the asset_out amount received, both making the asset_in/asset_out spot price higher. So we increase the spot price with dividing by (1-protocol_fee)*(1-asset_fee) to reflect correct amount out after the fee deduction

Stableswap

For stableswap there is no formula for trades involving share assets, so we need to do a trade with small amount.

  1. When no share involved in direct trade, amount_out is reduced by fee in SELL, making asset_out more expensive, so the asset_in/asset_out spot price should be increased. So divide spot-price-without-fee by 1-fee to reflect correct amount out after the fee deduction
  2. When share asset is sold, then we execute a small buy with specifying exact stable asset as amount_out, which under the hood leads to withdraw_asset_amount. We do this to prevent too low shares in calculations
  3. When share asset is bought, then we execute a small sell with specifying exact stable asset as amount_in, which under the hood leads to remove_liquidity_one_asset. We do this to prevent too low shares in calculations

TODOs:

  • Lower the amounts in the tests so only the slippage can lead to differences
  • Rework providers to include fees
  • Use math calculation instead of dry-run trade for stableswap spot price
  • Add missing spot price providers for all AMMs
  • Add functionality to get spot price for given route
  • write tests for new lbp spot price provider

Copy link

github-actions bot commented Apr 2, 2024

Crate versions that have been updated:

  • runtime-integration-tests: v1.21.2 -> v1.21.3
  • hydra-dx-math: v8.0.2 -> v8.1.1
  • pallet-dca: v1.4.5 -> v1.4.6
  • pallet-lbp: v4.7.5 -> v4.8.1
  • pallet-omnipool: v4.1.7 -> v4.2.0
  • pallet-route-executor: v2.2.2 -> v2.3.1
  • pallet-stableswap: v3.5.2 -> v3.6.1
  • pallet-transaction-multi-payment: v10.0.2 -> v10.0.3
  • pallet-xyk: v6.4.1 -> v6.4.2
  • hydradx-runtime: v232.0.0 -> v233.0.0
  • hydradx-traits: v3.2.1 -> v3.3.0

Runtime version has been increased.

@dmoka dmoka self-assigned this Apr 2, 2024
@dmoka dmoka marked this pull request as ready for review April 3, 2024 11:56
@dmoka dmoka requested a review from mrq1911 May 1, 2024 05:58
pallets/route-executor/src/lib.rs Outdated Show resolved Hide resolved
pallets/route-executor/src/lib.rs Outdated Show resolved Hide resolved
amount: amount_in,
}];

let share_amount = Self::calculate_shares(pool_id, &assets)
Copy link
Member

Choose a reason for hiding this comment

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

this case is not calculated by the math

asset_a: T::AssetId,
asset_b: T::AssetId,
) -> Result<FixedU128, ExecutorError<Self::Error>> {
match pool_type {
Copy link
Member

Choose a reason for hiding this comment

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

would be ideal to have this whole method covered by single math function so it doesn't have to be reimplemented in SDK as well

@mrq1911 mrq1911 merged commit 586453b into master May 8, 2024
4 checks passed
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