Send take #1
Send take #1
Conversation
* send-take implemented * send take * remove comments, return err * Replace 2 more checks with is_send_take flag * attempted cargo fix * Variable cleanup and code modification to handle fee updates in matching.rs * Remove unused variable and add assert * revert Cargo.lock * Anchor version * revert anchor version to 0.18.2 * update CI script * cargo.lock * Added test cases to more throroughly stress test SendTake * Remove unnecessary variable in test * Revert PR changes related to build * Skip permissioned markets test * Fix indentation * cargo fmt the state.rs file * Add overflow checks * variable rename * open orders accounting seems correct * Added print statements to understand the SendTake test case * get rid of overflow checks and calculate fees in matching Co-authored-by: jarry-xiao <jarry.xiao@gmail.com>
There was a problem hiding this comment.
What happens to referrer rebates on send_take? Currently, it looks like they are accounted for on the market_state but can't be extracted because they don't get added to any open orders account.
Either the instruction doesn't provide rebates, or we need to pass in another account that the rebates should be withdrawn into.
| # - <<: *defaults | ||
| # name: Permissioned Dex tests | ||
| # script: | ||
| # - cd dex/tests/permissioned/ && yarn && yarn build && yarn test && cd ../../../ |
There was a problem hiding this comment.
changes in this file sound unrelated
| post_allowed = true; | ||
| // Remove this block of code as it can lead to undefined behavior where | ||
| // an ImmediateOrCancel order is allowed to place orders on the book | ||
| // post_allowed = true; |
There was a problem hiding this comment.
definitely a good change to have though
| @@ -531,6 +548,7 @@ impl<'ob> OrderBookState<'ob> { | |||
| self.market_state.pc_fees_accrued += net_fees; | |||
| self.market_state.pc_deposits_total -= net_fees_before_referrer_rebate; | |||
|
|
|||
|
|
|||
| event_q | ||
| .push_back(out) | ||
| .map_err(|_| DexErrorCode::EventQueueFull)?; | ||
| } | ||
|
|
||
| if pc_qty_to_keep_locked > 0 { | ||
| let bids = self.orders_mut(Side::Bid); |
There was a problem hiding this comment.
the other side has a check_assert!(!is_send_take)?; here to guard against putting an order on the book accidentally (even if post_allowed should just always be false). Maybe nice to assert!(!(post_allowed && send_take)) further up.
| ) -> Result<Pubkey, ProgramError> { | ||
| gen_vault_signer_seeds(&nonce, market); | ||
| Ok(Pubkey::default()) | ||
| } |
|
|
||
| coin_debit, | ||
| native_pc_debit, | ||
| } = proceeds; |
There was a problem hiding this comment.
maybe it's possible to sanity check that coin_unlocked and native_pc_unlocked are zero?
|
|
||
| market_state.pc_deposits_total += deposit_amount; | ||
| check_assert!(market_state.coin_deposits_total >= withdraw_amount)?; | ||
| market_state.coin_deposits_total -= withdraw_amount; |
There was a problem hiding this comment.
also use checked_add/checked_sub, since the other order placing code does
There was a problem hiding this comment.
so i think the reason to do this was to actually exploit the underflow possibility (idr the exact explanation). I also believe that these fields are only used for stats
| &[], | ||
| ) | ||
| .map_err(|err| match err { | ||
| ProgramError::Custom(i) => match TokenError::from_u32(i) { |
There was a problem hiding this comment.
does this actually work? I thought invoke failed unrecoverably on cpi instruction error
Recreate PR for enabling jupiter compatibility instruction