Push proceeds to users in FinalizeSettle#62
Conversation
| if token_program_account.address() != &SPL_TOKEN_PROGRAM_ID { | ||
| return Err(ProgramError::IncorrectProgramId); | ||
| } | ||
|
|
||
| // The buffers' SPL authority is the state PDA, so it must sign each transfer. | ||
| let seeds = state_pda_seeds(); | ||
| let (state_pda, state_bump) = Address::find_program_address(&seeds, program_id); | ||
| if state_pda_account.address() != &state_pda { | ||
| return Err(SettlementError::StateAccountMismatch.into()); | ||
| } | ||
|
|
||
| let [seed] = seeds; | ||
| let state_bump = [state_bump]; | ||
| let signer_seeds = [seed, &state_bump].map(Seed::from); | ||
| let state_pda_signer = Signer::from(&signer_seeds); |
There was a problem hiding this comment.
these first lines appear to be nearly, if not exactly the same as what is in begin.rs parallel function pull_funds
| let mint = { | ||
| let destination = TokenAccount::from_account_view(push.destination) | ||
| .map_err(|_| SettlementError::InvalidBuyTokenAccount)?; | ||
| *destination.mint() | ||
| }; | ||
| // Re-derive the buffer from the carried bump (one hash, not a full | ||
| // search). A buffer exists only at its canonical address, so a wrong | ||
| // bump yields an address the transfer can't draw from. | ||
| let derived = Address::create_program_address( | ||
| &buffer_pda_signer_seeds(mint.as_array(), &[push.bump]), | ||
| program_id, | ||
| ) | ||
| .map_err(|_| SettlementError::PushSourceNotBuffer)?; | ||
| if push.source_buffer.address() != &derived { | ||
| return Err(SettlementError::PushSourceNotBuffer.into()); | ||
| } |
There was a problem hiding this comment.
In places like this where we need to rederive the canonical buffer, it would be really nice if that happens via a function like Buffer::load_from_pda function or similar which simultaneously reads the program data while also verifying canonical. You can see an example of me doing this in my own PR here.
Technically this would change the functionality a bit such that the canonical address check would happen by checking the mint reported in the actual buffer account, and not the mint reported in the buy_token_account, but this should actually still be fine.
|
|
||
| #[test] | ||
| fn finalizes_with_single_push() { | ||
| fn pushes_a_single_order() { |
There was a problem hiding this comment.
the single order test probably actually isnt necessary when there is already a multiple order test that supersedes it (at least, the tests are structured nearly the same with the exception of the single order part)
| } | ||
|
|
||
| #[test] | ||
| fn rejects_push_to_wrong_destination() { |
There was a problem hiding this comment.
this is a test technically for the begin function validation so it seems weird to see it here. I didn't see a duplicate test pre-existing on the begin side though.
| // which `FinalizeSettle` rejects when it reads the destination's mint. | ||
| let orders = [FinalizedIntent { | ||
| intent: &intent, | ||
| mint: other_mint, |
There was a problem hiding this comment.
technically this is sending from a different buffer rather than a non-buffer source. Of course this case should still fail, but the reasoning for why it fails should probably be ultimately different considering my previous comment suggesting to create a load_from_pda function.
| } | ||
|
|
||
| #[test] | ||
| fn rejects_push_from_substituted_source() { |
There was a problem hiding this comment.
this test seems very similar ot the previous non_buffer_source test. Probably we could get all the test validation we need from just validating like is done with this test.
| // `FinalizeSettle` re-derives the buffer from the destination's mint and | ||
| // rejects the mismatch before touching the substituted account. | ||
| let source_index = 3; | ||
| finalize.accounts[source_index].pubkey = Pubkey::new_unique(); |
There was a problem hiding this comment.
maybe a good idea to make this test stronger by making this account look like an actual buffer in every way possible? for example copying the data of the original buffer account into this one somehow and transferring ownership to the settlement program (I am not sure if all of this is actually possible but seems interesting)
| } | ||
|
|
||
| #[test] | ||
| fn rejects_fewer_pushes_than_orders() { |
There was a problem hiding this comment.
i could have sworn I saw this test somewhere else but now I can't find it. in either case, this test again seems like it should ultimately actually go in the begin tests due to that being where the condition is.
| } | ||
|
|
||
| #[test] | ||
| fn rejects_more_pushes_than_orders() { |
2 participants
Push each order's proceeds out of the buffers in
FinalizeSettle.This is the final part of the settlement push flow. The previous PR taught
BeginSettleto pair each settled order with exactly one push and check the push pays the order's buy token account, but nothing moved yet. HereFinalizeSettleactually performs the transfers.What
push_fundsdoesBeginSettlealready guarantees, for the paired finalize, that the push count matches the order count and that each push's destination is the right buy token account, and the counterpart check guarantees that begin ran.So the only things left for
FinalizeSettleto check is that:And naturally, it needs to actually send funds.
How to test
finalize_settle_pushes.rsmoves from asserting that pushes merely parse to asserting they actually pay: single-order, several orders sharing one buffer, and several orders drawing from different buffers, each checking both the destinations' credited balances and the buffers' debited balances. It also covers new edge cases.