feat: enable trigger orders on Swift signed message flow#2171
feat: enable trigger orders on Swift signed message flow#2171ChesterSim wants to merge 3 commits into
Conversation
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (1)
🚧 Files skipped from review as they are similar to previous changes (1)
WalkthroughRequire Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Poem
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. Comment |
…ned message flow - Extend has_valid_auction_params to accept trigger order types with no auction params - Add trigger_price validation in place_signed_msg_taker_order - Fix max_slot calculation to use unwrap_or(0) for trigger orders - Use existing is_trigger_order() helper for DRY enum checks - Add bankrun tests for TriggerMarket, TriggerLimit, and rejection without trigger_price Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
ChesterSim
force-pushed
the
chester/be-254-enable-trigger-orders-on-swift
branch
from
c4242ff to
b9286a0
Compare
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
programs/drift/src/instructions/keeper.rs (1)
836-843:⚠️ Potential issue | 🟠 MajorTrigger orders without
auction_durationsilently fail to place due to zero TTL.For trigger orders without auction parameters,
unwrap_or(0)at line 842 setsmax_slot == order_slot, creating zero time-to-live. At line 855, whenmax_slot < clock.slot(inevitable with async placement delays), the keeper silently returnsOk(())without placing the order. This makes trigger order placement fragile across async flows like Swift→keeper delays.💡 Proposed fix (add default TTL for trigger orders when auction duration is absent)
- let max_slot = if matching_taker_order_params.order_type == OrderType::Limit - || matching_taker_order_params.is_trigger_order() - { - order_slot.safe_add( - matching_taker_order_params - .auction_duration - .unwrap_or(0) - .cast::<u64>()?, - )? + let max_slot = if matching_taker_order_params.order_type == OrderType::Limit + || matching_taker_order_params.is_trigger_order() + { + let ttl_slots: u64 = if matching_taker_order_params.is_trigger_order() { + matching_taker_order_params + .auction_duration + .unwrap_or(500) + .cast::<u64>()? + } else { + matching_taker_order_params + .auction_duration + .unwrap_or(0) + .cast::<u64>()? + }; + order_slot.safe_add(ttl_slots)? } else {🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@programs/drift/src/instructions/keeper.rs` around lines 836 - 843, The code sets max_slot using matching_taker_order_params.auction_duration.unwrap_or(0), which makes trigger orders have zero TTL and silently fail; change the logic in the max_slot calculation (around matching_taker_order_params, order_slot, and auction_duration) to supply a sensible default TTL when matching_taker_order_params.is_trigger_order() is true and auction_duration is None (e.g., use a DEFAULT_TRIGGER_TTL constant or a small nonzero slot delta) so max_slot > order_slot and the keeper does not immediately return Ok(()) for delayed placements.
🧹 Nitpick comments (1)
tests/placeAndMakeSignedMsgBankrun.ts (1)
1866-1943: Strengthen the rejection test with a state assertion.After the expected failure, add an explicit check that no open order was created (Line 1935 catch path currently only checks error text).
✅ Suggested assertion addition
} catch (e) { // Expected to fail with InvalidSignedMsgOrderParam assert(e.toString().includes('0x1890') || e.toString().includes('InvalidSignedMsgOrderParam'), `Expected InvalidSignedMsgOrderParam error, got: ${e.toString()}`); } + await takerDriftClientUser.fetchAccounts(); + assert( + takerDriftClient.getUser().getOpenOrders().length === 0, + 'No trigger order should be placed when triggerPrice is missing' + );🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tests/placeAndMakeSignedMsgBankrun.ts` around lines 1866 - 1943, The test currently only asserts the error text when placeSignedMsgTakerOrder fails; after the catch, fetch the taker state and assert no open order was created: call takerDriftClient.fetchAccounts() and inspect takerDriftClient.getUserAccount() (or getUserAccountPublicKey()) to ensure there is no order matching the test's uuid/marketIndex (or that openOrders/orders length is unchanged/zero). Place this assertion inside the catch path before unsubscribing to guarantee the failed signed order did not produce any on‑chain open order.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Outside diff comments:
In `@programs/drift/src/instructions/keeper.rs`:
- Around line 836-843: The code sets max_slot using
matching_taker_order_params.auction_duration.unwrap_or(0), which makes trigger
orders have zero TTL and silently fail; change the logic in the max_slot
calculation (around matching_taker_order_params, order_slot, and
auction_duration) to supply a sensible default TTL when
matching_taker_order_params.is_trigger_order() is true and auction_duration is
None (e.g., use a DEFAULT_TRIGGER_TTL constant or a small nonzero slot delta) so
max_slot > order_slot and the keeper does not immediately return Ok(()) for
delayed placements.
---
Nitpick comments:
In `@tests/placeAndMakeSignedMsgBankrun.ts`:
- Around line 1866-1943: The test currently only asserts the error text when
placeSignedMsgTakerOrder fails; after the catch, fetch the taker state and
assert no open order was created: call takerDriftClient.fetchAccounts() and
inspect takerDriftClient.getUserAccount() (or getUserAccountPublicKey()) to
ensure there is no order matching the test's uuid/marketIndex (or that
openOrders/orders length is unchanged/zero). Place this assertion inside the
catch path before unsubscribing to guarantee the failed signed order did not
produce any on‑chain open order.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 8b36c7c1-e504-4cc3-a6b7-3d8983bd892e
📒 Files selected for processing (3)
programs/drift/src/instructions/keeper.rsprograms/drift/src/state/order_params.rstests/placeAndMakeSignedMsgBankrun.ts
Code reviewFound 1 issue:
protocol-v2/programs/drift/src/instructions/keeper.rs Lines 836 to 862 in ebdafdc In production, the keeper reads the signed message and submits a transaction in a later slot, so A fix would be to use a separate expiry window for trigger orders (e.g., a fixed offset such as the existing 500-slot staleness threshold) rather than deriving 🤖 Generated with Claude Code - If this code review was useful, please react with 👍. Otherwise, react with 👎. |
1 participant
Summary
OrderType::TriggerMarketandOrderType::TriggerLimitto be placed on-chain via the Swift signed message flow (placeSignedMsgTakerOrder)signSignedMsgOrderParamsMessageandplaceSignedMsgTakerOrderwork with trigger order params as-isChanges
Program (
order_params.rs):has_valid_auction_paramsto accept trigger order types with no auction params (using existingis_trigger_order()helper)Program (
keeper.rs):trigger_pricevalidation for trigger orders inplace_signed_msg_taker_ordermax_slotcalculation to useunwrap_or(0)for trigger orders (prevents panic on missingauction_duration)Tests:
placeSignedMsgTakerOrderplaceSignedMsgTakerOrdertrigger_priceis rejected withInvalidSignedMsgOrderParamTest plan
cargo check🤖 Generated with Claude Code
Summary by CodeRabbit
New Features
Bug Fixes
Tests