search

tdex-network / tdex-sdk

🛠 JavaScript SDK for building applications on top of TDEX
https://tdex.network
MIT License
5 stars 12 forks source link

Add price to TradeOrder #134

Open Janaka-Steph opened 2 years ago

Janaka-Steph commented 2 years ago

Currently we call discoverer.discover({ asset, amount: sats }) to get the best order, then we have to call MarketPrice to get the price preview. Instead TradeOrder could include the price.

@tiero @louisinger

louisinger commented 2 years ago

nACK: price is not a "constant from the Daemon point of view". It means that a price at time t can be different than the price at time t+1. That's why we must call the price preview just before making the swap (in order to be sure we'll trade at the right price)

tiero commented 2 years ago

Since we are sending the payload asset and amount already and with bestPrice strategy we do NOT only need to call Balances endpoint, but also some info from client, we basically doing already the preview when picking a provider&market combo: I do think it's fine to return the price right away, if the user is making a trade just right after. If it fails for bad pricing, he then call again as normal

louisinger commented 2 years ago

I do think it's fine to return the price right away, if the user is making a trade just right after. If it fails for bad pricing, he then call again as normal

@tiero do u mean return the price in discover function's result right ?

Note that it means make a marketPrice call for each orders selected by the discoverer (in case of equality, discover may return several orders then the user has to choose one). But this is a rare case now so it's ok

ACK but not sure to see a huge gain (in tdex-app for instance, it seems to me that the gain for this breaking change is: remove 1 line of code (am I wrong @Janaka-Steph?) but add a risk for useless requests to daemon :thinking:

Janaka-Steph commented 2 years ago

Afaik with bestPrice we already compute price for each order

louisinger commented 2 years ago

Afaik with bestPrice we already compute price for each order

Right. But Discoverer is an abstraction of discovery functions. it must work with any Discoveries, including the ones without marketPrice requests (like bestBalance only for instance). and I really don't know how to handle this properly in Discoverer abstraction -> that's why the solution is make additional marketPrice reqs at the end of discover, which is not optimal in my opinion.

I think in case of only bestPrice is used, better to do not use Discoverer and write your own "abstraction" caching the marketPrice requests (and reuse it later for the trade)