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]

simpler:

Suggested change

	(FINALIZE_FIXED_ACCOUNTS + 1..)
	.step_by(2)
	.map_while(|destination_index| {
	instruction
	.get_instruction_account_at(destination_index)
	.ok()
	.map(|account| &account.key)
	})
	(FINALIZE_FIXED_ACCOUNTS + 1..instruction.num_account_metas())
	.step_by(2)
	.map(|i| instruction.get_instruction_account_at(i).unwrap())

checking the code, the only way that this function can return an error is if the requested index is out of bounds.

If you want to actually optimize this further and are OK with going a little unsafe:

    (FINALIZE_FIXED_ACCOUNTS + 1..instruction.num_account_metas())
        .step_by(2)
        .map(|i| unsafe { instruction.get_instruction_account_at_unchecked(i) })

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

[kaze-cow]

why does settle default to a failing settlement tx? I understand this is intended to be used with testing BeginSettle, but when a (I assume working) function exists to have it be successful, for developer convenience and understanding it seems best to just stick with a successful result on the basic named helper.

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

[kaze-cow]

this comments logic seems a bit twisted. I think what its trying to say is that we want to validate that, even if the order species a mint, that doesn't mean it should be touched if there is no pull instruction (even if there is a push on the other side)

[kaze-cow]

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

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

[kaze-cow]

the count checks seem redundant with the following array equality checks.

[kaze-cow]

took me a while to realize but this function actually primarily focuses to test correctness of push_destinations and not any other parts of the process function. I feel like the name of the test function should reflect that like push_destinations_output_matches_finalize_parser or so.

(lmk if I have the wrong idea of what is happening here)