Validate order limit prices - #75
Conversation
99a169c to
b535496
Compare
b535496 to
306f91f
Compare
kaze-cow
left a comment
kaze-cow
left a comment
There was a problem hiding this comment.
in the equation you gave amount_out * sell_amount >= buy_amount * amount_in, why not flip it so the terms are matching up order like amount_out * sell_amount >= amount_in * buy_amount? My brain wants to think about it like amounts should be corresponding. This is the case in the code too.
in any case, the equation makes good sense when thinking in terms of a sell price order, you would want to ensure the limit price is respected, i.e.
amount_out >= amount_in * (buy_amount / sell_amount)
the buy_amount / sell_amount becomes an effective trading price for the output and thats scaled against the amount_in. Opposite happens on buy orders naturally. so the maths is good (as if it needed to be questioned!)
I love the new order builder. Now we need a SettleBuilder or so 😆
| .buy_amount(800_000) | ||
| .build(); | ||
| let violating = OrderBuilder::new(&mut svm, &program_id, &payer) | ||
| .kind(OrderKind::Buy) |
There was a problem hiding this comment.
I notice on the sell test we use only sells and on the buy test we use only buys. Any reason not to mix and match? otherwise I don't understand why we test to this extent in the first place.
There was a problem hiding this comment.
Mmh, I agree the two tests seem kind of weird. I also noticed that they use different tokens, which make them kind of moot. I changed this so that we mix and match and in general have this test more meaningful, wdyt?
5d045ff
|
I updated the explanation of the formula to match your comment: be30897 |
kaze-cow
left a comment
kaze-cow
left a comment
There was a problem hiding this comment.
all comments addressed. just left a couple misc "fyi" comments in the individual commits for the most recent changes.
3 participants
Make a user intent more meaningful by disallowing the contract to pull funds from the user if the price is worse than what the intent specifies.
Until now the program moved an order's sell tokens (pulls) and paid its buy tokens (pushes) without checking that the amounts respected the order. This adds the per-order checks that back DESIGN.md's user guarantee that the user receives at least the specified buy amount, proportional to how much of the sell amount was used.
This is the first of two PRs: it does the limit price only. The per-order sell/buy caps, fill-or-kill, and cumulative cross-settlement accounting follow separately.
Parsing the push amounts
BeginSettlenow also parses the push amounts through introspection, similarly to what was done for the accounts before.Consistency with parsing is validated by sharing helpers and a proptest.
The check
Note that it doesn't matter for this check whether an order is sell or buy.
I run the check after the pulls rather than before to make the code simpler.
I'm a bit concerned for security once token2022 is introduced and CPI on transfer becomes a thing, but for now this code should be safe.
Tests
Added
settle_limit_prices.rswith integration tests.Some of the edge cases are only covered by unit tests, but by the way the code is structured I think it should be enough.