Skip to content
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

Open
wants to merge 2 commits into
base: master
Choose a base branch
from
Open

Send take #1

wants to merge 2 commits into from

Conversation

mschneider
Copy link
Contributor

@mschneider mschneider commented Nov 14, 2022

Recreate PR for enabling jupiter compatibility instruction

a-creation and others added 2 commits October 23, 2022 14:22
* 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]>
Copy link
Contributor

@ckamm ckamm left a 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 ../../../
Copy link
Contributor

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;
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

good, but unrelated

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;


Copy link
Contributor

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);
Copy link
Contributor

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())
}
Copy link
Contributor

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;
Copy link
Contributor

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;
Copy link
Contributor

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

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) {
Copy link
Contributor

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

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants