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.

Sign up for GitHub

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

Jump to bottom

Add with_fees boolean parameter for Swaps getSpotPrices() #878

Merged
merged 7 commits into from
Nov 24, 2022
Merged

Add with_fees boolean parameter for Swaps getSpotPrices() #878

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
7 commits
Select commit Hold shift + click to select a range
6465e9b
Add with_fees boolean parameter for Swaps getSpotPrices()
vivekvpandya Nov 21, 2022
289812f
Fix runtime build issue
vivekvpandya Nov 22, 2022
7b0717e
Don't use with_fees flag for rikiddo in get_spot_price()
vivekvpandya Nov 23, 2022
f00c42d
Add with_fees parameter to SwapsApiServer
vivekvpandya Nov 23, 2022
119b358
Update changelog_for_devs.md
vivekvpandya Nov 23, 2022
dab1691
Fix format issues
vivekvpandya Nov 23, 2022
fbc8da4
Fix format issue
vivekvpandya Nov 24, 2022
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 1 addition & 1 deletion zrml/swaps/runtime-api/src/lib.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -31,6 +31,6 @@ sp_api::decl_runtime_apis! {
{
fn pool_shares_id(pool_id: PoolId) -> Asset<SerdeWrapper<MarketId>>;
fn pool_account_id(pool_id: &PoolId) -> AccountId;
fn get_spot_price(pool_id: &PoolId, asset_in: &Asset<MarketId>, asset_out: &Asset<MarketId>) -> SerdeWrapper<Balance>;
fn get_spot_price(pool_id: &PoolId, asset_in: &Asset<MarketId>, asset_out: &Asset<MarketId>, with_fees: bool) -> SerdeWrapper<Balance>;
Copy link
Member

Choose a reason for hiding this comment

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

This line is too long.

}
}
42 changes: 33 additions & 9 deletions zrml/swaps/src/lib.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1383,6 +1383,7 @@ mod pallet {
pool_id: &PoolId,
asset_in: &Asset<MarketIdOf<T>>,
asset_out: &Asset<MarketIdOf<T>>,
with_fees: bool,
) -> Result<BalanceOf<T>, DispatchError> {
let pool = Self::pool_by_id(*pool_id)?;
ensure!(pool.assets.binary_search(asset_in).is_ok(), Error::<T>::AssetNotInPool);
Expand All @@ -1394,7 +1395,12 @@ mod pallet {
let balance_out = T::AssetManager::free_balance(*asset_out, &pool_account);
let in_weight = Self::pool_weight_rslt(&pool, asset_in)?;
let out_weight = Self::pool_weight_rslt(&pool, asset_out)?;
let swap_fee = pool.swap_fee.ok_or(Error::<T>::SwapFeeMissing)?;

let swap_fee = if with_fees {
pool.swap_fee.ok_or(Error::<T>::SwapFeeMissing)?
} else {
BalanceOf::<T>::zero()
};

return Ok(crate::math::calc_spot_price(
balance_in.saturated_into(),
Expand Down Expand Up @@ -1434,7 +1440,17 @@ mod pallet {
}

if *asset_in == base_asset {
T::RikiddoSigmoidFeeMarketEma::price(*pool_id, balance_out, &balances)
let price_with_fee =
T::RikiddoSigmoidFeeMarketEma::price(*pool_id, balance_out, &balances)?;
if with_fees {
Ok(price_with_fee.saturated_into())
} else {
let fee_pct = T::RikiddoSigmoidFeeMarketEma::fee(*pool_id)?.saturated_into();
let fee_plus_one = BASE.saturating_add(fee_pct);
let price_without_fee: u128 =
bdiv(price_with_fee.saturated_into(), fee_plus_one)?;
Ok(price_without_fee.saturated_into())
}
Copy link
Member

Choose a reason for hiding this comment

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

I think all these changes regarding Rikiddo should be reverted. It doesn't really matter what the Rikiddo behavior is right now, and it seems to me like this calculation is not correct.

@sea212 It seems like the term fee is used as a name for min(min_revenue, initial_fee, eta(r)), where eta is the sigmoid function (ignoring the fact that r is not always well-defined), so if you multiply this with the total issuance of all outcome tokens, you get the exponent in the cost/price formula. It seems like it is used here as something like an approximation of the actual "fee" of Rikiddo. This seems off.

I'm fairly sure that one shouldn't even speak of a "fee" when dealing with LMSR and its variants. For example, in terms of LS-LMSR, if you buy from the market and then immediately sell back to the market, you make no profit or loss. This wouldn't be the case if there are fees. On low volume trades, the same should be true for Rikiddo. Have you or @numacoding ever defined an actual fee for Rikiddo in abstract terms?

Copy link
Member

Choose a reason for hiding this comment

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

The first usage of the term "fee" was defined in the Rikiddo research report from Numa.

It is actually min(min_revenue, initial_fee + eta(r)). min_revenue is what Numa calls "minimal revenue coefficient (omega)".

It is some kind of approximation. This part definitely has to be improved, but for now we can leave it like that and raise an issue to handle that imo. (since Rikiddo is not active).
I think in a path-independent algorithm like LS-LMSR the assumption that there is no profit or loss is correct, because the amount of base currency to pay for buying C(q0) -> C(q1) is C(q1) - C(q0) and the amount received for selling C(q1) -> C(q0) is C(q1) - C(q0). However, with Rikiddo this does not hold, because the "fee" parameter can vary in between two trades and change C(q). It depends on additional data, I think it would be cleaner if this would be indicated in the function, like C(q, r).
Although the question was raised what kind of implications the loss of the path-independence property has, it was never properly examined.

Copy link
Member

Choose a reason for hiding this comment

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

Thanks for the answer. Can you provide access to the research report. I seem to be locked out. :(

Indeed, we should start writing C(q, r) or C_r(q) (note: C_r, with fixed r is actually path-independent). As you say, for path-independent buying and then selling results in an exact break-even (because of, well, path-independence). Which means that there are no fees involved.

My issue here is that if the buy and the sell price go up, then whenever you sell to the market, the market creators pay you a fee. That just feels wrong. I think that 3.3.1 of A Practical Liquidity-Sensitive Automated Market Maker provides a somewhat servicable solution to this problem.

Copy link
Member

Choose a reason for hiding this comment

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

@vivekvpandya so maybe a TODO with an issue number would be appropriate.

} else if *asset_out == base_asset {
let price_with_inverse_fee = bdiv(
BASE,
Expand All @@ -1444,9 +1460,13 @@ mod pallet {
.saturated_into();
let fee_pct = T::RikiddoSigmoidFeeMarketEma::fee(*pool_id)?.saturated_into();
let fee_plus_one = BASE.saturating_add(fee_pct);
let price_with_fee: u128 =
bmul(fee_plus_one, bmul(price_with_inverse_fee, fee_plus_one)?)?;
Ok(price_with_fee.saturated_into())
let price_without_fee: u128 = bmul(price_with_inverse_fee, fee_plus_one)?;
Chralt98 marked this conversation as resolved.
Show resolved Hide resolved
if with_fees {
let price_with_fee: u128 = bmul(fee_plus_one, price_without_fee)?;
Ok(price_with_fee.saturated_into())
} else {
Ok(price_without_fee.saturated_into())
}
} else {
let price_without_fee = bdiv(
T::RikiddoSigmoidFeeMarketEma::price(*pool_id, balance_out, &balances)?
Expand All @@ -1455,10 +1475,14 @@ mod pallet {
.saturated_into(),
)?
.saturated_into();
let fee_pct = T::RikiddoSigmoidFeeMarketEma::fee(*pool_id)?.saturated_into();
let fee_plus_one = BASE.saturating_add(fee_pct);
let price_with_fee: u128 = bmul(fee_plus_one, price_without_fee)?;
Ok(price_with_fee.saturated_into())
if with_fees {
let fee_pct = T::RikiddoSigmoidFeeMarketEma::fee(*pool_id)?.saturated_into();
let fee_plus_one = BASE.saturating_add(fee_pct);
let price_with_fee: u128 = bmul(fee_plus_one, price_without_fee)?;
Ok(price_with_fee.saturated_into())
} else {
Ok(price_without_fee.saturated_into())
}
Chralt98 marked this conversation as resolved.
Show resolved Hide resolved
}
}

Expand Down
3 changes: 2 additions & 1 deletion zrml/swaps/src/mock.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -236,8 +236,9 @@ sp_api::mock_impl_runtime_apis! {
pool_id: &PoolId,
asset_in: &Asset<MarketId>,
asset_out: &Asset<MarketId>,
with_fees: bool,
) -> SerdeWrapper<Balance> {
SerdeWrapper(Swaps::get_spot_price(pool_id, asset_in, asset_out).ok().unwrap_or(0))
SerdeWrapper(Swaps::get_spot_price(pool_id, asset_in, asset_out, with_fees).ok().unwrap_or(0))
}

fn pool_account_id(pool_id: &PoolId) -> AccountIdTest {
Expand Down
47 changes: 27 additions & 20 deletions zrml/swaps/src/tests.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -186,7 +186,7 @@ fn allows_the_full_user_lifecycle() {
let asset_b_bal = Currencies::free_balance(ASSET_B, &ALICE);

// swap_exact_amount_in
let spot_price = Swaps::get_spot_price(&0, &ASSET_A, &ASSET_B).unwrap();
let spot_price = Swaps::get_spot_price(&0, &ASSET_A, &ASSET_B, true).unwrap();
assert_eq!(spot_price, _1);

let pool_account = Swaps::pool_account_id(&0);
Expand Down Expand Up @@ -689,25 +689,26 @@ fn most_operations_fail_if_pool_is_clean() {
});
}

#[test_case(_3, _3, _100, _100, 0, 10_000_000_000)]
#[test_case(_3, _3, _100, _150, 0, 6_666_666_667)]
#[test_case(_3, _4, _100, _100, 0, 13_333_333_333)]
#[test_case(_3, _4, _100, _150, 0, 8_888_888_889)]
#[test_case(_3, _6, _125, _150, 0, 16_666_666_667)]
#[test_case(_3, _6, _125, _100, 0, 25_000_000_000)]
#[test_case(_3, _3, _100, _100, _1_10, 11_111_111_111)]
#[test_case(_3, _3, _100, _150, _1_10, 7_407_407_408)]
#[test_case(_3, _4, _100, _100, _1_10, 14_814_814_814)]
#[test_case(_3, _4, _100, _150, _1_10, 9_876_543_210)]
#[test_case(_3, _6, _125, _150, _1_10, 18_518_518_519)]
#[test_case(_3, _6, _125, _100, _1_10, 27_777_777_778)]
#[test_case(_3, _3, _100, _100, 0, 10_000_000_000, 10_000_000_000)]
#[test_case(_3, _3, _100, _150, 0, 6_666_666_667, 6_666_666_667)]
#[test_case(_3, _4, _100, _100, 0, 13_333_333_333, 13_333_333_333)]
#[test_case(_3, _4, _100, _150, 0, 8_888_888_889, 8_888_888_889)]
#[test_case(_3, _6, _125, _150, 0, 16_666_666_667, 16_666_666_667)]
#[test_case(_3, _6, _125, _100, 0, 25_000_000_000, 25_000_000_000)]
#[test_case(_3, _3, _100, _100, _1_10, 11_111_111_111, 10_000_000_000)]
#[test_case(_3, _3, _100, _150, _1_10, 7_407_407_408, 66_666_666_67)]
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
#[test_case(_3, _3, _100, _150, _1_10, 7_407_407_408, 66_666_666_67)]
#[test_case(_3, _3, _100, _150, _1_10, 7_407_407_408, 6_666_666_667)]

#[test_case(_3, _4, _100, _100, _1_10, 14_814_814_814, 13_333_333_333)]
#[test_case(_3, _4, _100, _150, _1_10, 9_876_543_210, 88_888_888_89)]
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
#[test_case(_3, _4, _100, _150, _1_10, 9_876_543_210, 88_888_888_89)]
#[test_case(_3, _4, _100, _150, _1_10, 9_876_543_210, 8_888_888_889)]

#[test_case(_3, _6, _125, _150, _1_10, 18_518_518_519, 16_666_666_667)]
#[test_case(_3, _6, _125, _100, _1_10, 27_777_777_778, 25_000_000_000)]
fn get_spot_price_returns_correct_results_cpmm(
weight_in: u128,
weight_out: u128,
balance_in: BalanceOf<Runtime>,
balance_out: BalanceOf<Runtime>,
swap_fee: BalanceOf<Runtime>,
expected_spot_price: BalanceOf<Runtime>,
expected_spot_price_with_fees: BalanceOf<Runtime>,
expected_spot_price_without_fees: BalanceOf<Runtime>,
) {
ExtBuilder::default().build().execute_with(|| {
// We always swap ASSET_A for ASSET_B, but we vary the weights, balances and swap fees.
Expand All @@ -734,8 +735,13 @@ fn get_spot_price_returns_correct_results_cpmm(

let abs_tol = 100;
assert_approx!(
Swaps::get_spot_price(&pool_id, &ASSET_A, &ASSET_B).unwrap(),
expected_spot_price,
Swaps::get_spot_price(&pool_id, &ASSET_A, &ASSET_B, true).unwrap(),
expected_spot_price_with_fees,
abs_tol,
);
assert_approx!(
Swaps::get_spot_price(&pool_id, &ASSET_A, &ASSET_B, false).unwrap(),
expected_spot_price_without_fees,
abs_tol,
);
});
Expand All @@ -747,23 +753,24 @@ fn get_spot_price_returns_correct_results_rikiddo() {
create_initial_pool(ScoringRule::RikiddoSigmoidFeeMarketEma, None, false);
let pool_id = 0;
assert_noop!(
Swaps::get_spot_price(&pool_id, &ASSETS[0], &ASSETS[0]),
Swaps::get_spot_price(&pool_id, &ASSETS[0], &ASSETS[0], true),
crate::Error::<Runtime>::PoolIsNotActive
);
subsidize_and_start_rikiddo_pool(pool_id, &ALICE, 0);

// Asset out, base currency in. Should receive about 1/3 -> price about 3
let price_base_in =
Swaps::get_spot_price(&pool_id, &ASSETS[0], ASSETS.last().unwrap()).unwrap();
Swaps::get_spot_price(&pool_id, &ASSETS[0], ASSETS.last().unwrap(), true).unwrap();
// Between 0.3 and 0.4
assert!(price_base_in > 28 * BASE / 10 && price_base_in < 31 * BASE / 10);
// Base currency in, asset out. Price about 3.
let price_base_out =
Swaps::get_spot_price(&pool_id, ASSETS.last().unwrap(), &ASSETS[0]).unwrap();
Swaps::get_spot_price(&pool_id, ASSETS.last().unwrap(), &ASSETS[0], true).unwrap();
// Between 2.9 and 3.1
assert!(price_base_out > 3 * BASE / 10 && price_base_out < 4 * BASE / 10);
// Asset in, asset out. Price about 1.
let price_asset_in_out = Swaps::get_spot_price(&pool_id, &ASSETS[0], &ASSETS[1]).unwrap();
let price_asset_in_out =
Swaps::get_spot_price(&pool_id, &ASSETS[0], &ASSETS[1], true).unwrap();
// Between 0.9 and 1.1
assert!(price_asset_in_out > 9 * BASE / 10 && price_asset_in_out < 11 * BASE / 10);
});
Expand Down
6 changes: 4 additions & 2 deletions zrml/swaps/src/utils.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -177,7 +177,8 @@ where
ensure!(p.pool.bound(&p.asset_out), Error::<T>::AssetNotBound);
}

let spot_price_before = Pallet::<T>::get_spot_price(&p.pool_id, &p.asset_in, &p.asset_out)?;
let spot_price_before =
Pallet::<T>::get_spot_price(&p.pool_id, &p.asset_in, &p.asset_out, true)?;
if let Some(max_price) = p.max_price {
ensure!(spot_price_before <= max_price, Error::<T>::BadLimitPrice);
}
Expand Down Expand Up @@ -212,7 +213,8 @@ where
}
}

let spot_price_after = Pallet::<T>::get_spot_price(&p.pool_id, &p.asset_in, &p.asset_out)?;
let spot_price_after =
Pallet::<T>::get_spot_price(&p.pool_id, &p.asset_in, &p.asset_out, true)?;

// Allow little tolerance
match p.pool.scoring_rule {
Expand Down