Skip to content

Validate order amounts - #76

Open
fedgiac wants to merge 26 commits into
mainfrom
amounts-check
Open

Validate order amounts#76
fedgiac wants to merge 26 commits into
mainfrom
amounts-check

Conversation

@fedgiac

@fedgiac fedgiac commented Jul 17, 2026

Copy link
Copy Markdown
Contributor

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 in BeginSettle already.

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_order computes amount_in (total pulled) and hands it, with amount_out (the paired push), to a pure check_order_amounts helper that enforces:

  • amount_in <= sell_amount for sell orders.
  • amount_out <= buy_amount for buy orders.
  • Fill-or-kill: a non-partially_fillable order's exact side must be filled completely (sell: amount_in == sell_amount; buy: amount_out == buy_amount).
  • u64 overflow checks.

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.

@fedgiac
fedgiac requested a review from a team as a code owner July 17, 2026 21:35
Comment thread client/src/instructions.rs Outdated
Comment thread interface/src/instruction/settle/begin.rs Outdated
Comment thread interface/src/instruction/settle/begin.rs Outdated
Comment thread interface/src/instruction/settle/begin.rs Outdated
Comment thread interface/src/instruction/settle/begin.rs Outdated
Comment thread programs/settlement/src/settle/begin.rs Outdated
Comment on lines +508 to +511
sell: u64,
buy: u64,
kind: OrderKind,
partially_fillable: bool,

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Comment thread programs/settlement/src/settle/begin.rs
Comment thread programs/settlement/tests/settle_limit_prices.rs Outdated
Comment thread programs/settlement/tests/settle_limit_prices.rs
Base automatically changed from limit-price-check to main July 31, 2026 08:02
@kaze-cow
kaze-cow self-requested a review July 31, 2026 08:11

@kaze-cow kaze-cow left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

none of my remaining comments are important; I did have one followup though.

Comment on lines +227 to +236
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)
}
}

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

are we going to add the analogue for OrderIntent (or maybe I missed that too)?

It makes sense, we can do it once needed.

@tilacog

tilacog commented Jul 31, 2026

Copy link
Copy Markdown
Contributor

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

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)

@fedgiac

fedgiac commented Jul 31, 2026

Copy link
Copy Markdown
Contributor Author

The indexer can replay the past transactions when re-indexing/back-filling.

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

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants