Add automatic price calculation in `market.{buy|sell}`
espendk opened this issue · 4 comments
Is your feature request related to a problem? Please describe.
market.{buy|sell} used to allow price to be set to null which meant "accept any price" (= simulate a market order). This is dangerous, as such tx's are likely to be sandwich attacked:
- Attacker borrows liquidity to front-run the victim
- Attacker buys/sells the requested volume on Mangrove and inserts an offer at the top of the book with the requested volume and a price that is just a tiny bit better than the remainder on the book
- Victim's tx is executed and buys the attacker's offer
- Attacker repays the loan and pockets the difference between the borrowed amount and what the victim paid.
While we cannot prevent users from sending such orders, it should not be encouraged in the API. Therefore this feature was removed in commit 1a0d9de.
However, having automatic calculation of the price is useful, so a replacement feature should be implemented.
Describe the solution you'd like
When price=null, estimate the avg price for the order based on the cache and then use that price.
Describe alternatives you've considered
Best offer price + 5%. However, that seems like a non-obvious behavior to me. You'll ask to buy a certain quantity and behind the scenes the SDK chooses an an arbitrary price.
Additional context
N/A
My take on this is that exploratory use of the APISDK (eg in a REPL) can often lead to trying out mgv.marketOrder({wants: amount, price:???}) and the exact price is not important or difficult to figure out (because the user doesn't know the units yet, or because the tokens are fake, prices are arbitrary, etc).
The natural solution is to say: no price means pure market order, but indeed it is too dangerous to leave that as the default.
Uglier but maybe good enough:
- if
chainId=31337ornetwork=localhost, pure market order - error otherwise
wdyt?
I think having the behaviour differ locally and in production is a recipe for disaster 😅
I think we have the necessary price estimation functions already, so I think it's just a matter of invoking that when price=null. But it's still not a safe practice, so I'm actually not that keen on making such automagic too easy in the SDK.