Parse pushes in FinalizeSettle#47
Conversation
fedgiac
force-pushed
the
refactor-use-struct-for-ix-builders
branch
from
4989c8f to
d70c2c1
Compare
fedgiac
force-pushed
the
push-funds-to-user-parsing
branch
from
4b2ba50 to
c313817
Compare
fedgiac
force-pushed
the
push-funds-to-user-parsing
branch
from
c313817 to
c202fc0
Compare
fedgiac
changed the base branch from
main
to
refactor-test-simplification-helper-extraction-renaming
fedgiac
force-pushed
the
push-funds-to-user-parsing
branch
from
c202fc0 to
b8b3804
Compare
fedgiac
changed the title
FinalizeSettleFinalizeSettle
kaze-cow
left a comment
kaze-cow
left a comment
There was a problem hiding this comment.
I couldn't find any notable issues. One thing worth mentioning is that it was comented in the code that BeginSettle will need to validate the provided buy_token_account correspond to the actual order's buy token account, as the finalize instruction doesn't do this validation. I am assuming this is handled in a separate ticket I haven't seen yet.
| /// canonical source buffer, and `amount` is the quantity to push. | ||
| pub struct FinalizedIntent<'a> { | ||
| pub intent: &'a OrderIntent, | ||
| pub mint: Pubkey, |
There was a problem hiding this comment.
I was surprised that this argument is needed, since the OrderIntent should contain buy_token_account which itself contains the data needed to access the mint. I'm guessing the reason we can't/don't want to exclude the mint from here is because pulling the would require an async operation to pull the buy_token_account data?
| pub struct Push<'a> { | ||
| pub source_buffer: &'a AccountView, | ||
| pub destination: &'a AccountView, | ||
| pub bump: u8, | ||
| pub amount: &'a [u8; 8], | ||
| } | ||
|
|
||
| /// Struct storing accounts, bumps, and amounts from parsing the input of | ||
| /// `FinalizeSettle`, laid out as a flat list of `[source_buffer, destination]` | ||
| /// account pairs parallel to `bumps` and `amounts`. The parsing step that created | ||
| /// this struct guarantees `push_accounts.len() == 2 * amounts.len()` and | ||
| /// `bumps.len() == amounts.len()`, so the offsets below never run short. | ||
| pub struct Pushes<'a> { | ||
| /// `[source_buffer, destination]` per push, flattened. | ||
| push_accounts: &'a [AccountView], | ||
| bumps: &'a [u8], | ||
| /// One push amount (big-endian `u64`) per push, parallel to `bumps`. | ||
| amounts: &'a [[u8; 8]], | ||
| } | ||
|
|
||
| impl<'a> Pushes<'a> { | ||
| /// Returns an iterator yielding one [`Push`] per step. | ||
| #[allow( | ||
| clippy::arithmetic_side_effects, | ||
| reason = "offsets are bounded by tx limits" | ||
| )] | ||
| pub fn iter(&self) -> impl Iterator<Item = Push<'a>> + '_ { | ||
| let push_count = self.bumps.len(); | ||
| let mut i = 0usize; | ||
| let mut account_offset = 0usize; | ||
| std::iter::from_fn(move || { | ||
| if i >= push_count { | ||
| return None; | ||
| } | ||
| let bump = self.bumps[i]; | ||
| let amount = &self.amounts[i]; | ||
| i += 1; | ||
|
|
||
| let source_buffer = &self.push_accounts[account_offset]; | ||
| let destination = &self.push_accounts[account_offset + 1]; | ||
| account_offset += 2; | ||
|
|
||
| Some(Push { | ||
| source_buffer, | ||
| destination, | ||
| bump, | ||
| amount, | ||
| }) | ||
| }) | ||
| } | ||
| } |
There was a problem hiding this comment.
this is the same pattern that was discussed back in BeginSettle PR (if I recall correctly) that we discussed may be improvable (mentioned here)
We can save fixing this for separate PR but for now just wanted to note
| accounts.first().ok_or(ProgramError::NotEnoughAccountKeys)?; | ||
| let (begin_ix_index, body) = recover_counterpart(instruction_data)?; | ||
|
|
||
| let [instructions_sysvar_account, state_pda_account, token_program_account, push_accounts @ ..] = |
There was a problem hiding this comment.
may be a bit late to mention this, but for consistency in programming languages "self" parameters to a function are usually put first. In this case, the state_pda serves that same sort of purpose, but its the second argument here (presumably because we put the sysvar porgram first). Does solana have a particular convention here? If not, I would opt to put the state_pda account first on every instruction that requires it.
| let sysvar = Address::new_from_array([1u8; 32]); | ||
| let state = Address::new_from_array([0xa1u8; 32]); | ||
| let token_program = Address::new_from_array([0xa2u8; 32]); | ||
| // The same source buffer funds both pushes: parsing makes no uniqueness | ||
| // assumption about source buffers. | ||
| let source = Address::new_from_array([3u8; 32]); | ||
| let dest0 = Address::new_from_array([4u8; 32]); | ||
| let dest1 = Address::new_from_array([5u8; 32]); |
There was a problem hiding this comment.
random note: in my recent PRs I have been favoring simply Address::new_unique(). While I realize this makes it harder to match accounts to purpose, it seems less error prone.
Technically accounts are base58 alphabet which includes most letters, so it would be cool if instead we had a Address::fromStringPrefix("Source") or so which allowed for literally making the address name itself.
| // The three fixed accounts (`[0xff..]`, `[0xfe..]`, `[0xfd..]`) differ | ||
| // from every source/destination address above. | ||
| let mut accounts = vec![ | ||
| fake_account_from_array([0xff; 32]), |
There was a problem hiding this comment.
in particular this account is 0xffff... which might look like a null/invalid account in certain conditions.
| let (mut svm, program_id, payer) = setup(); | ||
| let mint_1 = token::create_mint(&mut svm, &payer); | ||
| let mint_2 = token::create_mint(&mut svm, &payer); | ||
| let intent0 = OrderBuilder::new(&mut svm, &program_id, &payer, &mint_1).build(); | ||
| let intent1 = OrderBuilder::new(&mut svm, &program_id, &payer, &mint_2).build(); |
There was a problem hiding this comment.
underscores and then no underscores?
| fn rejects_too_few_accounts() { | ||
| let (mut svm, program_id, payer) = setup(); | ||
|
|
||
| // A well-formed single-push finalize... | ||
| let mut finalize = Instruction::from(FinalizeSettle { | ||
| program_id, | ||
| begin_ix_index: BEGIN_INDEX.into(), | ||
| orders: &[], | ||
| }); | ||
| // ...with one account popped. | ||
| finalize.accounts.pop(); | ||
|
|
||
| let result = send_settlement(&mut svm, &program_id, &payer, &[], finalize); | ||
| let Err(TransactionError::InstructionError(index, ix_error)) = result else { | ||
| panic!("expected an instruction error, got {result:?}"); | ||
| }; | ||
| assert_eq!(index, FINALIZE_INDEX); | ||
| assert_eq!( | ||
| // This unusual way to test is because `InstructionError::NotEnoughAccountKeys` | ||
| // is deprecated, while `ProgramError::NotEnoughAccountKeys` is not. | ||
| // Rather than silencing a linting, let's use the program error. | ||
| ProgramError::try_from(ix_error), | ||
| Ok(ProgramError::NotEnoughAccountKeys), | ||
| ); | ||
| } |
There was a problem hiding this comment.
i might be missing it elsewhere but since every push corresponds to two accounts, it means the behavior could change if two accounts were popped instead of one.
2 participants
Parsing pushes of funds to the user, that is, the
Finalizeversion of PR #38.A settlement "push" is a transfer of a solver-specified token funds from a settlement buffer account to a destination account.
The funds are (after the processing PR) intended to go from the buffer account of the buy token mint (as derived from the buy token account) to the user-specified buy token account.
This is the parsing-and-validation half of pushing funds to users: the transfers are decoded and the layout is checked, but no funds move yet and nothing is validated against runtime context. It mirrors the pull-parse step that folded pulling into
BeginSettle, on the symmetric side of the settlement.As discussed previously, and similarly to what I did for the
Pulls, I folded the dedicatedPushinstruction intoFinalizeSettle, rather than having a separate instruction, so that we don't need to do some extra instruction scanning for transfers, which takes extra transaction space and CU.The instruction carries no order identity, at least directly. It does so indirectly: every pull corresponds to exactly one order coming from
BeginSettle, specified in the same sequence. This isn't directly relevant for this PR but it explains why there aren't multiple pushes per order: the idea is that the solver accumulates the needed funds in the buffer account and then sends everything to the user in one go.We need to send funds from an account we control to make sure that the funds we pay to the user are indeed coming from the execution of the protocol and not from executing some other unrelated protocol that happens to have a transfer as a side effect.
The instruction carries, per push, the source buffer's canonical bump, so the program (in the next PR) re-derives the buffer PDA with a single hash rather than a
find_program_addresssearch. Unlike in other cases so far, issue #49 doesn't apply because we don't control the buffer account data (it's owned by the token program) and so we'll always need to get the bumps from the user or rederive them.The order PDA isn't in the instruction or the interface builder either; but the push order still has to line up with
BeginSettle's order list for the (deferred) positional cross-check, so the client (which has the intents) sorts the pushes by canonical order PDA before emitting them, supplying each bump fromfind_buffer_pda.The sorting in
BeginSettleis done in the interface instead, but for this instruction the interface doesn't have access to the actual order accounts, so the sorting cannot be done there. I don't like this inconsistency, so if you prefer as an alternative I can move back the sorting ofBeginSettleto the client and keep the interface unsorted. Eventually all of this will be solved by introducing a combined Begin+Finalize builder.Design doc
Updated
DESIGN.md(§"The settlement transaction") to fold pushing intoFinalizeSettleand drop the standalonePushentry, mirroring how the pull step folded pulling intoBeginSettle. That describes the end state (pushing proceeds to buy token accounts via the state PDA's authority over the buffers); this PR is the parsing toward it.How to test
New unit/integration tests covering the parsing. A round-trip proptest in the client: builds via the client
FinalizeSettlebuilder and parses back.