Validate pushes to users in BeginSettle

[fedgiac]

Pair each settled orderin BeginSettle with its FinalizeSettle push.

A settlement is a [BeginSettle, FinalizeSettle] pair: BeginSettle pulls the users' sell funds, FinalizeSettle pushes the proceeds back out of the buffers. Until now the two instructions validated their own halves in isolation, so nothing tied a settled order to the push that is supposed to pay it. This change makes BeginSettle cross-check the paired FinalizeSettle: every order must be paid by exactly one push, and that push must send to the order's buy token account. The actual transfers stay in FinalizeSettle and belongs in a follow-up; here I only add the validation that belongs to BeginSettle and its tests.

Notable missing validation steps, to be added in a later PR: checking that the origin of the funds is the canonical buffer PDA associated to the mint of the buy token account.

How the pairing works

BeginSettle already loads the instructions sysvar to locate its counterpart, so I reuse that introspection to read the finalize's push destinations directly off its accounts.

Orders and pushes are both laid out sorted by order PDA, so order i is paid by push i.

I deliberately don't validate the push structure here (a dangling source buffer, a destination that isn't a real token account): FinalizeSettle re-parses the same instruction from its own data and holds the destination accounts, so those checks belong to it. BeginSettle only has the orders, so it owns the count and destination-correspondence checks.

Shared factoring

The count-of-fixed-accounts constant that push_destinations needs was a private test constant in the interface's finalize module; I promoted it to a public FINALIZE_FIXED_ACCOUNTS and added a test that pins it to what the builder actually emits, so the two can't drift.

Tests

The BeginSettle unit tests gain a proptest that builds a random well-formed FinalizeSettle and asserts push_destinations (FinalizeSettle introspection) recovers the same push count and destinations as FinalizeSettleInput (standard parsing used in FinalizeSettle).

Unfortunately, this introduced a new unsafe in the tests which I wasn't able to drop. The problem is always the same as last time: there's no safe way to build an introspected transaction without starting a full Solana environment.

The integration test helpers grew to make testing easier. Please suggest improvement, it was a lot of back and forth so I could have easily missed something!

[kaze-cow]

Overall looking good but the tests appear to need some refinement. I think there were some functions/branch controls in the tests that were designed to increase the efficiency, but I think we should focus on developer convenience instead.

[kaze-cow]

here if we left the sending of the transaction on the svm to the user, this function could build the settlement transaction and then I imagine the receiving function can make any modifications/trigger any error it may need prior to sending, kind of like what we do in the unit test.

[fedgiac]

Updated, indeed we were able to make a few tests simpler. What do you think? a81feaa

[kaze-cow]

Following the suggestion in my other comment to remove svm.send_transaction(tx).map_err(|e| e.err) line from the end of the function settle_and_pay, the desired failing state could be simulated it seems without needing this function.

[fedgiac]

The problem mostly disappeared with the changes from a81feaa. I still added a helper with the error unwrapping because it's a common pattern. I wonder if this should change and we should add more tests. Or maybe we should do the further tests in the helper, I imagine they are always the same.

[kaze-cow]

isnt buffer creation already idempotent in the instruction? if so, we could simplify the test code and remove this branch

[fedgiac]

Yeah, but unfortunately transaction creation isn't so if I drop this check then the test can (and does) fail because I'm trying to send the exact same transaction twice. In practice the fix would be sending only one transaction in a block, mine it, and then go to the next block with the same transaction, which now passes. But I think this is more work than keeping this check.

[kaze-cow]

the majority of the helper functions for the files in this folder depend upon svmand program_id as their first arguments. Consider making a shared struct

struct SvmContext {
  svm: LiteSVM,
  program_id: Pubkey

  // optionally add more fields ex.
  // mints: Map<String, Pubkey>
}

and then have all these functions be like impl Buffer for SvmContext {} to reduce the number of args for these functions.

If we are OK with a larger refactor, then an even bigger change could be made such that the chain fixture needed for a test is defined declaratively rather than functionally, sort of like what is already happening with OrderBuilder (which I really like the pattern for). Though this is probably overkill.

[fedgiac]

I'm not really a fan of this, but I also don't have a strong opinion.
OrderBuilder fits much more nicely because it's a single-use struct that's then destroyed once the order is created. It's nice because it lets us set defaults in readable ways.

Here, SvmContext works differently.
First, it takes a mutable pointer. In Rust there can ever be a singe instance of a mutable reference, so we either switch all helpers using it to this pattern or we destroy the builder each time after use to reclaim the mutable reference.
The latter is too verbose, while the former imho creates a new restriction: "if you want to use svm, it must be through an explicit helper of SvmContext or destroying it to recover the reference."
The benefits feel small. In practice:

- helper(svm, input)
+ svm.helper(input)

Note also that you mention including the program in the context, but it could be troublesome to do that because there are helpers that don't need it, so we may have to add a fake program id just to call the helper in some cases.

In short: I'm strongly against making this change in this PR because doing it only locally for the new functions is inconsistent and I believe messier than it seems (I didn't try though). I'm weakly against this change in general, but I can be convinced if the resulting code diff looks nice enough. 🙂

[tilacog]

I like the context struct idea but it seems most useful for complex setups where a setup function returns it for us.

[tilacog]

I like the context struct idea but it seems most useful for complex setups where a setup function returns it for us.