Add with_fees boolean parameter for Swaps getSpotPrices()

[vivekvpandya]

Fixes #840

[maltekliemann]

You should also add a note to the changelog.

[maltekliemann]

This line is too long.

[maltekliemann]

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?

[sea212]

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.

[maltekliemann]

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.

[Chralt98]

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

[maltekliemann]

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)]

[maltekliemann]

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)]

[Chralt98]

Aren't the RPC calls defined here?

zeitgeist/zrml/swaps/rpc/src/lib.rs

Lines 40 to 201 in 5033991

	#[rpc(client, server)]
	pub trait SwapsApi<BlockHash, BlockNumber, PoolId, AccountId, Balance, MarketId>
	where
	Balance: FromStr + Display + parity_scale_codec::MaxEncodedLen,
	MarketId: FromStr + Display + parity_scale_codec::MaxEncodedLen,
	PoolId: FromStr + Display,
	{
	#[method(name = "swaps_poolSharesId", aliases = ["swaps_poolSharesIdAt"])]
	async fn pool_shares_id(
	&self,
	pool_id: PoolId,
	at: Option<BlockHash>,
	) -> RpcResult<Asset<SerdeWrapper<MarketId>>>;
	#[method(name = "swaps_poolAccountId", aliases = ["swaps_poolAccountIdAt"])]
	async fn pool_account_id(&self, pool_id: PoolId, at: Option<BlockHash>)
	-> RpcResult<AccountId>;
	#[method(name = "swaps_getSpotPrice", aliases = ["swaps_getSpotPriceAt"])]
	async fn get_spot_price(
	&self,
	pool_id: PoolId,
	asset_in: Asset<MarketId>,
	asset_out: Asset<MarketId>,
	at: Option<BlockHash>,
	) -> RpcResult<SerdeWrapper<Balance>>;
	#[method(name = "swaps_getSpotPrices")]
	async fn get_spot_prices(
	&self,
	pool_id: PoolId,
	asset_in: Asset<MarketId>,
	asset_out: Asset<MarketId>,
	blocks: Vec<BlockNumber>,
	) -> RpcResult<Vec<SerdeWrapper<Balance>>>;
	}
	/// A struct that implements the [`SwapsApi`].
	pub struct Swaps<C, B> {
	client: Arc<C>,
	_marker: core::marker::PhantomData<B>,
	}
	impl<C, B> Swaps<C, B> {
	/// Create a new `PredictionMarkets` with the given reference to
	/// the client.
	pub fn new(client: Arc<C>) -> Self {
	Swaps { client, _marker: Default::default() }
	}
	}
	pub enum Error {
	/// The call to the runtime failed.
	RuntimeError,
	}
	impl From<Error> for i32 {
	fn from(e: Error) -> i32 {
	match e {
	Error::RuntimeError => 1,
	}
	}
	}
	#[async_trait]
	impl<C, Block, PoolId, AccountId, Balance, MarketId>
	SwapsApiServer<<Block as BlockT>::Hash, NumberFor<Block>, PoolId, AccountId, Balance, MarketId>
	for Swaps<C, Block>
	where
	Block: BlockT,
	C: Send + Sync + 'static + ProvideRuntimeApi<Block> + HeaderBackend<Block>,
	C::Api: SwapsRuntimeApi<Block, PoolId, AccountId, Balance, MarketId>,
	PoolId: Clone + Codec + MaybeDisplay + MaybeFromStr + Send + 'static,
	AccountId: Clone + Display + Codec + Send + 'static,
	Balance: Codec + MaybeDisplay + MaybeFromStr + MaxEncodedLen + Send + 'static,
	MarketId: Clone + Codec + MaybeDisplay + MaybeFromStr + MaxEncodedLen + Send + 'static,
	{
	async fn pool_shares_id(
	&self,
	pool_id: PoolId,
	at: Option<<Block as BlockT>::Hash>,
	) -> RpcResult<Asset<SerdeWrapper<MarketId>>> {
	let api = self.client.runtime_api();
	let at = BlockId::hash(at.unwrap_or_else(||
	//if the block hash is not supplied assume the best block
	self.client.info().best_hash));
	let res = api.pool_shares_id(&at, pool_id).map_err(|e| {
	CallError::Custom(ErrorObject::owned(
	Error::RuntimeError.into(),
	"Unable to get pool shares identifier.",
	Some(e.to_string()),
	))
	})?;
	Ok(res)
	}
	async fn pool_account_id(
	&self,
	pool_id: PoolId,
	at: Option<<Block as BlockT>::Hash>,
	) -> RpcResult<AccountId> {
	let api = self.client.runtime_api();
	let at = BlockId::hash(at.unwrap_or_else(||
	//if the block hash is not supplied assume the best block
	self.client.info().best_hash));
	let res = api.pool_account_id(&at, &pool_id).map_err(|e| {
	CallError::Custom(ErrorObject::owned(
	Error::RuntimeError.into(),
	"Unable to get pool account identifier.",
	Some(e.to_string()),
	))
	})?;
	Ok(res)
	}
	/// If block hash is not supplied, the best block is assumed.
	async fn get_spot_price(
	&self,
	pool_id: PoolId,
	asset_in: Asset<MarketId>,
	asset_out: Asset<MarketId>,
	at: Option<<Block as BlockT>::Hash>,
	) -> RpcResult<SerdeWrapper<Balance>> {
	let api = self.client.runtime_api();
	let at = BlockId::hash(at.unwrap_or_else(|| self.client.info().best_hash));
	let res = api.get_spot_price(&at, &pool_id, &asset_in, &asset_out).map_err(|e| {
	CallError::Custom(ErrorObject::owned(
	Error::RuntimeError.into(),
	"Unable to get spot price.",
	Some(e.to_string()),
	))
	})?;
	Ok(res)
	}
	async fn get_spot_prices(
	&self,
	pool_id: PoolId,
	asset_in: Asset<MarketId>,
	asset_out: Asset<MarketId>,
	blocks: Vec<NumberFor<Block>>,
	) -> RpcResult<Vec<SerdeWrapper<Balance>>> {
	let api = self.client.runtime_api();
	blocks
	.into_iter()
	.map(|block| {
	let hash = BlockId::number(block);
	let res =
	api.get_spot_price(&hash, &pool_id, &asset_in, &asset_out).map_err(|e| {
	CallError::Custom(ErrorObject::owned(
	Error::RuntimeError.into(),
	"Unable to get spot price.",
	Some(e.to_string()),
	))
	})?;
	Ok(res)
	})
	.collect()
	}
	}

[Chralt98]

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

[vivekvpandya]

@Chralt98 https://github.com/zeitgeistpm/zeitgeist/pull/878/files#diff-188fb32939fe9cc9416edd214b3ab316def04705fad5aa33b9130b8d49220244R1415 TODO added here.

[Chralt98]

Aren't the RPC calls defined here?

Completely my fault. I overlooked your changes made to this file, because I had this PR open from yesterday, when your changes were not there then.