Validate order amounts - #76
Conversation
| sell: u64, | ||
| buy: u64, | ||
| kind: OrderKind, | ||
| partially_fillable: bool, |
There was a problem hiding this comment.
it later becomes obvious that these last 4 fields are just inputs to intent_with, but its kind of hard to tell from here.
additionally, a lot of the tests below (ex. in accepts_fills_within_amounts) use the same or nearly the same intent
I like the overall pattern (we use it elsewhere in the codebase to great success!), but I wonder if there is some way to simplify the overall number of fields necessary in the structure or make it more obvious that these parameters are just order inputs without making it too wordy.
There was a problem hiding this comment.
Yes, I think this is a problem we see in many tests where there are a lot of small helpers. Each of them is fine, locally. but probably we can find a better abstraction. We could even use a builder pattern for intents as well.
At this point I wouldn't address this in this PR as I don't think the issue is really here, but happy to update it if you have a suggestion.
There was a problem hiding this comment.
I think it would be a lot better if we just had a way to group them together. Like, if we did:
struct IntentWithArgs {
sell: u64,
buy: u64,
kind: OrderKind,
partially_fillable: bool,
}
struct FillCase {
withdrawn: u64,
received: u64,
amount_in: u64,
amount_out: u64,
// turn this into a tuple. the grouping inand of itself is what makes this more clear
intent: &IntentWithArgs
}
// and then later
let intent = intent_with(intent.kind, intent.partially_fillable, intent.sell, intent.buy);works even better if intent_with takes in an actual struct or tuple
Co-authored-by: Kaze <230549489+kaze-cow@users.noreply.github.com>
kaze-cow
left a comment
There was a problem hiding this comment.
none of my remaining comments are important; I did have one followup though.
| impl TryFrom<&[u8]> for OrderAccount { | ||
| type Error = ProgramError; | ||
|
|
||
| fn try_from(bytes: &[u8]) -> Result<Self, Self::Error> { | ||
| let bytes: &[u8; EncodedOrderAccount::SIZE] = bytes | ||
| .try_into() | ||
| .map_err(|_| ProgramError::InvalidAccountData)?; | ||
| OrderAccount::try_from(*bytes) | ||
| } | ||
| } |
There was a problem hiding this comment.
I just realized this was added as part of this PR. surely there are more places across the codebase where decoding an order from bytes can come in handy? are we going to add the analogue for OrderIntent (or maybe I missed that too)?
There was a problem hiding this comment.
Nope, there aren't other places! Otherwise I'd have done it in its own PR. Well, the CLI uses it (added: 1ee96cb) but this wasn't available until that PR was merged to main.
There was a problem hiding this comment.
are we going to add the analogue for OrderIntent (or maybe I missed that too)?
It makes sense, we can do it once needed.
In that case there's still no need for self-CPI then. The indexer can replay the past transactions when re-indexing/back-filling. (I still need to review this PR properly) |
That's right. It still needs to replay all past transactions first, since the actual execution of partially fillable orders now depends on the already-filled amounts stored in the account (but only for reverts). |
After this PR, a user intent is fully respected in a settlement: fill-or-kill orders become actually fill-or-kill; the maximum sell/buy amount is respected. And accounting is done so that partial fills add up across settlements.
This concludes all checks needed to execute a basic settlement. This PR should be reviewed with the idea in mind that all protections for a user intent are in place. Any way to exploit user intents by solvers would be a security issue at this point for reviewing purposes.
Notably, DESIGN.md changes a bit: full-on snapshots aren't needed anymore, the cumulative amounts bought/sold are enough for everything we need to do on-chain. (@tilacog: the partial amount traded so far is only present in the PDA, should we emit something here for the back-end? In principle it can be derived from the past settlements though).
This allows us to simplify the finalize instruction: we don't need any further checks in
FinalizeSettle, we're doing everything we need inBeginSettlealready.I'm also sneaking in a small change:
impl TryFrom<&[u8]> for OrderAccount. It's just a way to not force the caller to check the size themselves. Interestingly, we didn't really need it so far (except once in the settle CLI) and even now we only need it in tests. I still think it's nice to offer it and I added relevant tests.The checks
For each order, after its pulls execute,
process_ordercomputesamount_in(total pulled) and hands it, withamount_out(the paired push), to a purecheck_order_amountshelper that enforces:amount_in <= sell_amountfor sell orders.amount_out <= buy_amountfor buy orders.partially_fillableorder's exact side must be filled completely (sell:amount_in == sell_amount; buy:amount_out == buy_amount).Notably, thanks to the way the math is structured, there's no place where we need to worry about rounding issues anywhere in the programs.
Tests
Added integration tests to
settle_limit_prices.rs.As before, some errors (notably overflows) are only checked in unit tests.