-
Notifications
You must be signed in to change notification settings - Fork 99
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Send take #1
base: master
Are you sure you want to change the base?
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 <[email protected]>
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
good, but unrelated
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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; | |||
|
|||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
whitespace only
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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()) | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
unrelated changes
|
||
coin_debit, | ||
native_pc_debit, | ||
} = proceeds; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
also use checked_add/checked_sub, since the other order placing code does
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
does this actually work? I thought invoke failed unrecoverably on cpi instruction error
Recreate PR for enabling jupiter compatibility instruction