-
Notifications
You must be signed in to change notification settings - Fork 0
[ENG-164] Add BatchReplace instruction to the dropset program
#70
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
base: ENG-182
Are you sure you want to change the base?
Conversation
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.
Pull request overview
Adds initial scaffolding for a new BatchReplace instruction and introduces a fixed-size, zero-init Orders argument type to make instruction encoding/decoding safer and more ergonomic across program + client tooling.
Changes:
- Introduce
Orders/OrdersArrayas helper instruction arg types for up toMAX_ORDERSorder args with fixed-size packing. - Wire a new
BatchReplaceinstruction through the interface, program entrypoint, and client helpers/examples/tests (plus CU bench harness). - Refactor order-mutating instruction contexts to share a single
MutateOrdersContextand update debug logging implementation.
Reviewed changes
Copilot reviewed 31 out of 32 changed files in this pull request and generated 7 comments.
Show a summary per file
| File | Description |
|---|---|
| program/src/instructions/post_order.rs | Switch to shared MutateOrdersContext for account loading. |
| program/src/instructions/cancel_order.rs | Switch to shared MutateOrdersContext for account loading. |
| program/src/instructions/mod.rs | Rename/route from removed batch module to batch_replace. |
| program/src/instructions/batch_replace.rs | New (currently WIP) handler implementation for BatchReplace. |
| program/src/instructions/batch.rs | Remove old stub process_batch. |
| program/src/entrypoint.rs | Route DropsetInstruction::BatchReplace to process_batch_replace. |
| program/src/debug.rs | Swap debug macro backend to solana_program_log. |
| program/src/context/mutate_orders_context.rs | New shared context + unit test enforcing account-order invariants. |
| program/src/context/mod.rs | Remove per-instruction contexts, add mutate_orders_context. |
| program/src/context/deposit_withdraw_context.rs | Replace debug-assert drift checks with unit tests + shared helpers. |
| program/src/context/post_order_context.rs | Removed (superseded by MutateOrdersContext). |
| program/src/context/cancel_order_context.rs | Removed (superseded by MutateOrdersContext). |
| program/Cargo.toml | Drop pinocchio-log, add solana-program-log, adjust debug feature. |
| package.json | Add example + CU-bench script entries. |
| interface/src/instructions/mod.rs | Export Orders and add BatchReplace args/accounts. |
| interface/src/instructions/orders.rs | New fixed-size Orders instruction argument type with custom pack/unpack. |
| interface/src/state/user_order_sectors.rs | Add MAX_ORDERS_USIZE, to_sector_indices, and mark_as_free helper. |
| instruction-macros/.../unpack.rs | Clarify safety docs for Unpack::read_bytes. |
| instruction-macros/.../tagged.rs | Inline write_bytes_tagged. |
| instruction-macros-impl/.../pack_tagged.rs | Clarify safety comment. |
| cu-bench/tests/* | New CU benchmark test harness for pack/unpack vs borsh. |
| cu-bench/programs/pack-orders/* | New SBF program for CU benchmarking pack/unpack vs borsh. |
| cu-bench/pack-orders.sh | Script to run the CU benchmarks. |
| client/src/context/market.rs | Add helper to create BatchReplace instruction. |
| client/examples/batch_replace.rs | New example for BatchReplace. |
| client/tests/batch_replace.rs | New test invoking BatchReplace (currently incompatible with program validation). |
| client/Cargo.toml | Enable borsh std feature. |
| Cargo.toml | Add cu-bench crates to workspace; adjust workspace borsh dependency config; add cfgs. |
| Cargo.lock | Lockfile updates for new crates/deps. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| let bid_infos = new_bids | ||
| .into_order_args_iter() | ||
| .map(|item| to_order_info(item).map_err(DropsetError::from)); |
Copilot
AI
Feb 9, 2026
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.
new_bids is moved by into_order_args_iter() here, but later (under #[cfg(feature = "debug")]) it’s borrowed again in log_orders(&new_bids, ...), which will not compile. Consider iterating over new_bids.clone(), moving logging before consuming, or building the iterator from a clone/borrow.
| let market: MarketRef = unsafe { ctx.market_account.load_unchecked() }; | ||
| Node::check_in_bounds(market.sectors, user_sector_index_hint)?; | ||
| // Find and verify the user's seat with the given index hint. | ||
| // Safety: The index hint was just verified as in-bounds. | ||
| let user_seat = find_seat_with_hint(market, user_sector_index_hint, ctx.user.address())?; | ||
|
|
||
| let bid_sector_indices = user_seat.user_order_sectors.bids.to_sector_indices(); | ||
| let ask_sector_indices = user_seat.user_order_sectors.asks.to_sector_indices(); |
Copilot
AI
Feb 9, 2026
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.
removed_bid_indices/removed_ask_indices are derived from the user seat’s user_order_sectors, but the mapping is never cleared/updated. The subsequent remove_at calls will free/reuse those sectors, leaving the seat mapping pointing to invalid sectors and corrupting state. Ensure the seat’s order sector mappings are freed/updated atomically with the book removals.
| let market: MarketRef = unsafe { ctx.market_account.load_unchecked() }; | |
| Node::check_in_bounds(market.sectors, user_sector_index_hint)?; | |
| // Find and verify the user's seat with the given index hint. | |
| // Safety: The index hint was just verified as in-bounds. | |
| let user_seat = find_seat_with_hint(market, user_sector_index_hint, ctx.user.address())?; | |
| let bid_sector_indices = user_seat.user_order_sectors.bids.to_sector_indices(); | |
| let ask_sector_indices = user_seat.user_order_sectors.asks.to_sector_indices(); | |
| // We take a mutable reference here so we can update the user's seat mapping | |
| // atomically with respect to the sector removals that will follow. | |
| let mut market: MarketRefMut = unsafe { ctx.market_account.load_unchecked_mut() }; | |
| Node::check_in_bounds(market.sectors, user_sector_index_hint)?; | |
| // Find and verify the user's seat with the given index hint. | |
| // Safety: The index hint was just verified as in-bounds. | |
| let user_seat = | |
| find_mut_seat_with_hint(market, user_sector_index_hint, ctx.user.address())?; | |
| // Capture the current sector indices to remove, then clear the mapping so the | |
| // seat no longer references sectors that are about to be freed/reused. | |
| let bid_sector_indices = user_seat.user_order_sectors.bids.to_sector_indices(); | |
| let ask_sector_indices = user_seat.user_order_sectors.asks.to_sector_indices(); | |
| user_seat.user_order_sectors = OrderSectors::default(); |
interface/src/instructions/orders.rs
Outdated
| /// The number of elements representing real order arguments in [`NewOrders::order_args`]. | ||
| /// This value will always be less than or equal to the max number of orders [`MAX_ORDERS`]. | ||
| /// The remaining elements will be zero-initialized but cannot be accessed through the | ||
| /// public [`Orders`] interface. | ||
| pub num_orders: u8, | ||
| /// Instruction data that isn't read is free, so it's simpler to always use [`MAX_ORDERS`] | ||
| /// elements in the array and simply ignore elements with an index >= [`NewOrders::num_orders`] |
Copilot
AI
Feb 9, 2026
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 docs reference NewOrders::order_args / NewOrders::num_orders, but the type is Orders. This looks like a stale rename and makes the public API docs misleading; update these references to Orders::....
| /// The number of elements representing real order arguments in [`NewOrders::order_args`]. | |
| /// This value will always be less than or equal to the max number of orders [`MAX_ORDERS`]. | |
| /// The remaining elements will be zero-initialized but cannot be accessed through the | |
| /// public [`Orders`] interface. | |
| pub num_orders: u8, | |
| /// Instruction data that isn't read is free, so it's simpler to always use [`MAX_ORDERS`] | |
| /// elements in the array and simply ignore elements with an index >= [`NewOrders::num_orders`] | |
| /// The number of elements representing real order arguments in [`Orders::order_args`]. | |
| /// This value will always be less than or equal to the max number of orders [`MAX_ORDERS`]. | |
| /// The remaining elements will be zero-initialized but cannot be accessed through the | |
| /// public [`Orders`] interface. | |
| pub num_orders: u8, | |
| /// Instruction data that isn't read is free, so it's simpler to always use [`MAX_ORDERS`] | |
| /// elements in the array and simply ignore elements with an index >= [`Orders::num_orders`] |
| Ok(sector_index) | ||
| } | ||
|
|
||
| /// Removes all mapped [`PriceToIndex`] elements from a user's order sectors. |
Copilot
AI
Feb 9, 2026
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.
to_sector_indices doesn't remove anything; it only returns a copy of the current sector indices. The docstring currently says it "Removes all mapped" elements, which is incorrect and could cause callers to assume the mapping is cleared. Please update the comment (or adjust the function to actually clear the mapping, if that's the intended behavior).
| /// Removes all mapped [`PriceToIndex`] elements from a user's order sectors. | |
| /// Returns a copy of all mapped sector indices for a user's orders without modifying the mapping. |
client/tests/batch_replace.rs
Outdated
| let mollusk = new_dropset_mollusk_context(vec![ | ||
| (event_authority::ID, Account::new(0, 0, &SYSTEM_PROGRAM_ID)), | ||
| (user_address, Account::new(10_000_000, 0, &SYSTEM_PROGRAM_ID)), | ||
| (market_address, Account::new(10_000_000, 0, &dropset::ID)), | ||
| ]); |
Copilot
AI
Feb 9, 2026
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 market account in this Mollusk setup has data_len = 0 (uninitialized) and there is no registered seat for user_address, but the on-chain handlers validate initialization and then attempt to locate the user’s seat. As written, the assert!(result.program_result.is_ok()) expectation is not compatible with the program’s validation logic. Either initialize a real market+seat in Mollusk (e.g., run RegisterMarket/seat creation flows) or assert the expected initialization error for now.
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.
Pull request overview
Copilot reviewed 32 out of 33 changed files in this pull request and generated 10 comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| // 5. Adjust the user's seat balances: | ||
| // - base = new_base.checked_add(base_from_canceled_orders).checked_sub(base_in_posted) | ||
| // - quote = new_quote.checked_add(quote_from_canceled_orders).checked_sub(quote_in_posted) | ||
|
|
||
| Ok(()) |
Copilot
AI
Feb 9, 2026
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 handler currently returns Ok(()) after only partial cancellation logic and without posting replacement orders. Since entrypoint routes BatchReplace here, consider returning a deterministic error until the full atomic replace behavior (cancel+refund+post+mapping updates) is implemented.
| let num_new_bids = new_bids.num_orders as usize; | ||
| let num_new_asks = new_asks.num_orders as usize; | ||
|
|
||
| // Convert the order info args to order infos. | ||
| let bid_infos = new_bids | ||
| .into_order_args_iter() | ||
| .map(|item| to_order_info(item).map_err(DropsetError::from)); | ||
|
|
Copilot
AI
Feb 9, 2026
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.
num_new_bids, num_new_asks, and bid_infos are currently unused, and bid_infos is never consumed (so any intended validation/conversion doesn’t run). Please remove these until needed, or ensure the iterator is actually used as part of the replace logic.
| let num_new_bids = new_bids.num_orders as usize; | |
| let num_new_asks = new_asks.num_orders as usize; | |
| // Convert the order info args to order infos. | |
| let bid_infos = new_bids | |
| .into_order_args_iter() | |
| .map(|item| to_order_info(item).map_err(DropsetError::from)); |
| @@ -0,0 +1,230 @@ | |||
| //! See [`process_batch`]. | |||
Copilot
AI
Feb 9, 2026
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 module doc comment references process_batch, but that function/module was removed. Update the docs to reference process_batch_replace (or the correct instruction handler) so rustdoc links remain valid.
| //! See [`process_batch`]. | |
| //! See [`process_batch_replace`]. |
| pub fn into_order_args_iter(self) -> impl Iterator<Item = OrderInfoArgs> { | ||
| let n = self.num_orders as usize; | ||
| self.order_args.0.into_iter().take(n) |
Copilot
AI
Feb 9, 2026
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.
Orders derives Unpack, so num_orders can be attacker-controlled when deserializing instruction data. into_order_args_iter currently trusts num_orders and will silently clamp via .take(n), which can mask invalid data and risks future out-of-bounds/panic if num_orders is later used for indexing/loop bounds. Consider validating num_orders <= MAX_ORDERS during unpack (custom Unpack impl) or provide a checked accessor that returns InvalidInstructionData when num_orders is out of range.
| pub fn into_order_args_iter(self) -> impl Iterator<Item = OrderInfoArgs> { | |
| let n = self.num_orders as usize; | |
| self.order_args.0.into_iter().take(n) | |
| pub fn into_order_args_iter( | |
| self, | |
| ) -> Result<impl Iterator<Item = OrderInfoArgs>, ProgramError> { | |
| let n = self.num_orders as usize; | |
| if n > MAX_ORDERS_USIZE { | |
| return Err(ProgramError::InvalidInstructionData); | |
| } | |
| Ok(self.order_args.0.into_iter().take(n)) |
interface/src/instructions/orders.rs
Outdated
| /// The number of elements representing real order arguments in [`NewOrders::order_args`]. | ||
| /// This value will always be less than or equal to the max number of orders [`MAX_ORDERS`]. | ||
| /// The remaining elements will be zero-initialized but cannot be accessed through the | ||
| /// public [`Orders`] interface. | ||
| pub num_orders: u8, |
Copilot
AI
Feb 9, 2026
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.
Doc comments reference NewOrders::..., but the type is named Orders. Update these intra-doc links to point at Orders/Orders::num_orders so docs don’t contain broken links or misleading names.
| let market: MarketRef = unsafe { ctx.market_account.load_unchecked() }; | ||
| Node::check_in_bounds(market.sectors, user_sector_index_hint)?; | ||
| // Find and verify the user's seat with the given index hint. | ||
| // Safety: The index hint was just verified as in-bounds. | ||
| let user_seat = find_seat_with_hint(market, user_sector_index_hint, ctx.user.address())?; | ||
|
|
||
| let bid_sector_indices = user_seat.user_order_sectors.bids.to_sector_indices(); | ||
| let ask_sector_indices = user_seat.user_order_sectors.asks.to_sector_indices(); | ||
|
|
Copilot
AI
Feb 9, 2026
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.
to_sector_indices() only snapshots sector indices; it does not clear the seat’s user_order_sectors. Since this instruction later removes orders from the book, the seat will still point at now-free/reused sectors. Update the flow to also free/clear the mapping (or use an API that both returns indices and marks nodes free) before returning.
| let market: MarketRef = unsafe { ctx.market_account.load_unchecked() }; | |
| Node::check_in_bounds(market.sectors, user_sector_index_hint)?; | |
| // Find and verify the user's seat with the given index hint. | |
| // Safety: The index hint was just verified as in-bounds. | |
| let user_seat = find_seat_with_hint(market, user_sector_index_hint, ctx.user.address())?; | |
| let bid_sector_indices = user_seat.user_order_sectors.bids.to_sector_indices(); | |
| let ask_sector_indices = user_seat.user_order_sectors.asks.to_sector_indices(); | |
| let mut market: MarketRefMut = unsafe { ctx.market_account.load_unchecked_mut() }; | |
| Node::check_in_bounds(market.sectors, user_sector_index_hint)?; | |
| // Find and verify the user's seat with the given index hint. | |
| // Safety: The index hint was just verified as in-bounds. | |
| let user_seat = | |
| find_mut_seat_with_hint(market, user_sector_index_hint, ctx.user.address())?; | |
| let bid_sector_indices = user_seat.user_order_sectors.bids.to_sector_indices(); | |
| let ask_sector_indices = user_seat.user_order_sectors.asks.to_sector_indices(); | |
| // Clear the user's order-sector mappings now that we've snapshotted the indices. | |
| user_seat.user_order_sectors.bids = OrderSectors::default(); | |
| user_seat.user_order_sectors.asks = OrderSectors::default(); |
client/tests/batch_replace.rs
Outdated
| let mollusk = new_dropset_mollusk_context(vec![ | ||
| (event_authority::ID, Account::new(0, 0, &SYSTEM_PROGRAM_ID)), | ||
| ( | ||
| user_address, | ||
| Account::new(10_000_000, 0, &SYSTEM_PROGRAM_ID), | ||
| ), | ||
| (market_address, Account::new(10_000_000, 0, &dropset::ID)), |
Copilot
AI
Feb 9, 2026
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 market account in this test is created with data_len = 0 (Account::new(..., 0, ...)), but the on-chain instruction contexts require an initialized market account (header + is_initialized). As written, BatchReplace should fail with AccountNotInitialized. Initialize the market in the test (e.g., run RegisterMarket first or provide valid market data) before asserting success.
| let mollusk = new_dropset_mollusk_context(vec![ | |
| (event_authority::ID, Account::new(0, 0, &SYSTEM_PROGRAM_ID)), | |
| ( | |
| user_address, | |
| Account::new(10_000_000, 0, &SYSTEM_PROGRAM_ID), | |
| ), | |
| (market_address, Account::new(10_000_000, 0, &dropset::ID)), | |
| // Create a market account with sufficient data to hold an initialized header. | |
| let mut market_account = Account::new(10_000_000, 1, &dropset::ID); | |
| // Mark the market account as initialized (e.g., first byte as `is_initialized = true`). | |
| market_account.data[0] = 1; | |
| let mollusk = new_dropset_mollusk_context(vec![ | |
| (event_authority::ID, Account::new(0, 0, &SYSTEM_PROGRAM_ID)), | |
| ( | |
| user_address, | |
| Account::new(10_000_000, 0, &SYSTEM_PROGRAM_ID), | |
| ), | |
| (market_address, market_account), |
| /// Removes all mapped [`PriceToIndex`] elements from a user's order sectors. | ||
| #[inline(always)] | ||
| pub fn to_sector_indices(&self) -> [SectorIndex; MAX_ORDERS_USIZE] { | ||
| let mut removed = [MaybeUninit::<SectorIndex>::uninit(); MAX_ORDERS_USIZE]; | ||
| let ptr = removed.as_mut_ptr() as *mut SectorIndex; | ||
|
|
||
| // Copy out all removed sector indices. |
Copilot
AI
Feb 9, 2026
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.
OrderSectors::to_sector_indices is documented as removing mapped elements, but it only copies sector indices and takes &self (so it cannot remove/free anything). Either fix the docstring/name to reflect that it’s a read-only snapshot, or change the API to &mut self and actually mark nodes free if the intent is to clear the mapping.
| /// Removes all mapped [`PriceToIndex`] elements from a user's order sectors. | |
| #[inline(always)] | |
| pub fn to_sector_indices(&self) -> [SectorIndex; MAX_ORDERS_USIZE] { | |
| let mut removed = [MaybeUninit::<SectorIndex>::uninit(); MAX_ORDERS_USIZE]; | |
| let ptr = removed.as_mut_ptr() as *mut SectorIndex; | |
| // Copy out all removed sector indices. | |
| /// Returns a snapshot of all mapped [`PriceToIndex`] sector indices for a user's order | |
| /// sectors without modifying the underlying mappings. | |
| #[inline(always)] | |
| pub fn to_sector_indices(&self) -> [SectorIndex; MAX_ORDERS_USIZE] { | |
| let mut removed = [MaybeUninit::<SectorIndex>::uninit(); MAX_ORDERS_USIZE]; | |
| let ptr = removed.as_mut_ptr() as *mut SectorIndex; | |
| // Copy out all sector indices. |
| solana-account-view.workspace = true | ||
| solana-address = { workspace = true, features = ["copy"] } | ||
| solana-instruction-view = { workspace = true, features = ["cpi"] } | ||
| solana-program-log = { workspace = true } | ||
| static_assertions.workspace = true |
Copilot
AI
Feb 9, 2026
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.
solana-program-log is only referenced behind #[cfg(feature = "debug")] (via the debug! macro), but it’s a required dependency here. Consider making it optional and enabling it via the debug feature to avoid compiling extra deps in non-debug builds.
| let quote_remaining = | ||
| load_mut_order_from_sector_index(market, idx).quote_remaining(); | ||
| quote_from_canceled_bids = quote_from_canceled_bids | ||
| .checked_add(quote_remaining) | ||
| .ok_or_else(|| DropsetError::ArithmeticOverflow)?; |
Copilot
AI
Feb 9, 2026
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.
This code removes orders from the market (remove_at) and sums remaining collateral, but the user’s MarketSeat balances are never credited and the seat’s sector mapping is never cleared. The handler should refund collateral and update the seat/mapping (similar to process_cancel_order) as part of the same atomic instruction.
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.
Pull request overview
Copilot reviewed 9 out of 9 changed files in this pull request and generated 2 comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| // Safety: No account data in `accounts` is currently borrowed. | ||
| let mut ctx = unsafe { MutateOrdersContext::load(accounts)? }; | ||
|
|
||
| Ok(()) |
Copilot
AI
Feb 10, 2026
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 handler currently loads the context and then returns Ok(()) without performing any cancels/posts or updating the user's seat. Since BatchReplace is routed from the entrypoint, this will make the instruction succeed while doing nothing; either implement the behavior or return a dedicated error until it’s ready.
| Ok(()) | |
| Err(DropsetError::UnimplementedInstruction) |
| pub unsafe fn process_batch_replace_WIP( | ||
| accounts: &[AccountView], | ||
| instruction_data: &[u8], | ||
| ) -> ProgramResult { | ||
| let BatchReplaceInstructionData { |
Copilot
AI
Feb 10, 2026
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.
Avoid leaving a pub on-chain entrypoint-style function named process_batch_replace_WIP in the program binary. If this is only for experimentation, gate it behind #[cfg(test)]/a feature flag, make it pub(crate), or remove it to prevent accidental use and reduce maintenance overhead.
Description
OrdersandOrdersArrayas helper types for the zero-init, foolproof instruction argument for order info args