Repository navigation
feat(scenarios/phoenix): prepare state with Phoenix's own instructions - #31
92Infinitus92 wants to merge 7 commits into
Conversation
…k factors and share helpers
… turn and list a trader's positions
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
| fn uncross_done(before: &BboView, after: &BboView) -> bool { | ||
| !after.crossed() | ||
| || (after.best_bid_ticks, after.best_ask_ticks) | ||
| == (before.best_bid_ticks, before.best_ask_ticks) |
There was a problem hiding this comment.
uncross_done treats unchanged bid and ask prices as success even when the book is still crossed. If more than 64 crossed orders share the best price, one crank can fill 64 orders without changing either price. The move then stops and writes back a crossed book, so later trades or liquidations can still fill against old orders.
Keep cranking while after.crossed() is true. Use changes in orders or filled amounts, rather than prices alone, to detect a stalled crank.
Prompt To Fix With AI
This is a comment left during a code review.
Path: crates/core/src/scenarios/protocols/phoenix-eternal/v1/market.rs
Line: 408-411
Comment:
**Cranking stops before orders uncross**
`uncross_done` treats unchanged bid and ask prices as success even when the book is still crossed. If more than 64 crossed orders share the best price, one crank can fill 64 orders without changing either price. The move then stops and writes back a crossed book, so later trades or liquidations can still fill against old orders.
Keep cranking while `after.crossed()` is true. Use changes in orders or filled amounts, rather than prices alone, to detect a stalled crank.
---
For each issue above, determine whether it is valid and should be fixed. If so, fix it directly.
| let lots = view | ||
| .positions() | ||
| .find(|(asset, _)| *asset == context.market.asset_id) | ||
| .map(|(_, position)| position.base_lot_position().as_inner()) | ||
| .unwrap_or(0); | ||
| if (long && lots > 0) || (!long && lots < 0) { |
There was a problem hiding this comment.
prepare_cascade chooses holders from Trader::positions() before asking Hawkeye what they hold. A hot trader's account copy can miss a position that Phoenix keeps in ActiveTraderBuffer, so that trader is skipped before holdings_in runs. This can leave valid holders out or make the cascade report that no holder qualifies.
Read the chosen market's position through Hawkeye first. Apply the side filter and 24-holder limit to that result.
Prompt To Fix With AI
This is a comment left during a code review.
Path: crates/core/src/scenarios/protocols/phoenix-eternal/v1/trader.rs
Line: 505-510
Comment:
**Cascades miss current holders**
`prepare_cascade` chooses holders from `Trader::positions()` before asking Hawkeye what they hold. A hot trader's account copy can miss a position that Phoenix keeps in `ActiveTraderBuffer`, so that trader is skipped before `holdings_in` runs. This can leave valid holders out or make the cascade report that no holder qualifies.
Read the chosen market's position through Hawkeye first. Apply the side filter and 24-holder limit to that result.
---
For each issue above, determine whether it is valid and should be fixed. If so, fix it directly.
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
There was a problem hiding this comment.
Overview
This PR replaces the Phoenix Eternal byte-patch templates with templates that run Phoenix's own IDL instructions in a sandbox copy of the local VM. Each instruction is signed by its mainnet signer (oracle keys, makers, the trader, or a GlobalConfig role), and only the accounts the instructions changed are written back. It adds:
- market-move, liquidation-ready (several markets, one trader), cascade, open, cancel, withdraw and deposit templates;
- 12 configuration templates;
- a
list_phoenix_trader_positionsMCP tool; keep_markets_usable, which rewinds funding timestamps and restamps oracle readings.
The two issues Greptile raised are fixed in 044171a:
- The uncross loop now counts resting orders as well as the best bid and ask.
- Cascade holders are now found through Hawkeye instead of the Trader account's copy.
Assessment
Overall the design is sound. Because Phoenix checks every change itself, refused changes come back with Phoenix's reason, and the sandbox trial() pattern keeps the searches free of side effects. The main remaining concern is how the cascade handles errors: some failures for one holder are skipped, but others abort the whole override. Smaller points are below.
Notes that don't belong to a single line:
- Every Phoenix override rewrites the map.
keep_markets_usableruns before every Phoenix override, including the ones that fall through to the generic IDL path (phoenix-stop-loss-trigger,phoenix-permission-limits). Theaccountpassed to the generic path was read before the map was patched. No current generic template targets the PerpAssetMap, so this is only a trap for later. A future generic override on the map would write back the stale copy and undo the raised thresholds. - Partial writes on failure.
svm.rsstops applying a Phoenix override's writes at the firstset_scenario_override_accounterror but keeps the writes already applied. Since the point of this PR is accounts that agree with each other, a partial write leaves the book, trader and map out of step. Writing all of them or none would be safer. - Cost. Each
Sandbox::newandtrial()clones the whole SVM (clone_for_profiling). The cascade does this per holder, up to 24, plus two bisections per holder, so Play can get slow on busy markets. A short note in the README, or a log line with the elapsed time, would help users.
| mark | ||
| } else { | ||
| let Some(price) = | ||
| boundary(&mut search, &mut context, trader, far, precision, |view| { |
There was a problem hiding this comment.
Error handling is inconsistent within one holder's band search. Cancel and initial-margin failures continue to the next holder, but this boundary(...)? (and search.run(&context.oracle_reports(mark)?)? at 555, and the underwater call at 575) propagate errors. If Hawkeye's margin view fails at a deep search price for any single holder, the whole cascade override is skipped. The restamp_readings comment mentions exactly this: an EMA can leave no positive price. Consider treating an Err here like the other per-holder failures and skipping that holder.
| .collect(); | ||
|
|
||
| // A trader the book cannot absorb in turn is dropped, and the rest are prepared again. | ||
| for _ in 0..2 { |
There was a problem hiding this comment.
The retry loop has two weaknesses.
prepared.run(cancels)?at 624 aborts the cascade if the combined cancels fail, even though each holder's cancels passed on their own in the band phase.- After two passes the function returns an error even when the second pass still had liquidations that went through, because the survivors are never re-prepared.
Looping until liquidations.len() == chosen.len() (each pass strictly shrinks chosen) would be both simpler and more robust than a fixed 0..2.
| /// so 64 stay well inside a transaction's 1.4 million. | ||
| const UNCROSS_MATCH_LIMIT: u64 = 64; | ||
| /// A book side holds at most `ORDERBOOK_CAPACITY` resting orders, so no move needs more cranks. | ||
| const MAX_UNCROSS_CRANKS: usize = ORDERBOOK_CAPACITY / UNCROSS_MATCH_LIMIT as usize; |
There was a problem hiding this comment.
The bound assumes one book side. Each match uses up at least one order on either side, so a fully crossed book can take up to (num_bids + num_asks) / 64 cranks, which is about 2× this. Spline liquidity may add more on top. It's unlikely to be hit, but the error at line 399 would then blame a book that was still making progress. Consider 2 * ORDERBOOK_CAPACITY / UNCROSS_MATCH_LIMIT, since the no-progress check in uncross_done already guards against spinning.
| rpc_url: &str, | ||
| trader: &str, | ||
| ) -> Result<serde_json::Value, String> { | ||
| let trader = Pubkey::from_str(trader) |
There was a problem hiding this comment.
The removed collateral tool trimmed the input (params.trader.trim()), but this one doesn't. An address pasted with whitespace or a newline is now rejected as invalid. Use Pubkey::from_str(trader.trim()).
There was a problem hiding this comment.
Overview
This PR replaces the Phoenix Eternal byte patches with real Phoenix instructions. Each template runs those instructions on a sandboxed copy of the VM, signed by the key that signs them on mainnet (with signature checks off), and writes back only the accounts that changed. That design is much more robust than hand-patching bytes, since Phoenix validates every change itself. It also adds cross-margin and cascade liquidation preparation, twelve config levers, and an MCP positions tool.
Greptile raised two issues. Both look fixed in 044171a:
uncross_donenow also compares the number of resting orders.- The cascade now finds holders through Hawkeye instead of the Trader account's copy.
Main concerns
- Cascade robustness: inside the per-trader band loop, a failure for one trader stops the whole cascade (
?), even though candidate discovery carefully skips such traders. Details inline. - Cascade cost and side effects:
prepare_cascadeloads every trader in the GlobalTraderIndex into the live surfnet before it applies the 24-holder cap. Those writes are permanent, unlike the sandbox runs. The 24 holders are also the first ones in pubkey order, not the largest or the riskiest, so the cascade covers an arbitrary subset. keep_markets_usableruns for every Phoenix-owned account override, including ones on the generic IDL path (prepare_phoenix_overridegates onstarts_with("phoenix-") || owner == PHOENIX). As a result, an unrelated override silently rewrites the PerpAssetMap: thresholds, reading slots and funding timestamps. That may be intended, but it is worth confirming and documenting.markets_in_mapcost: it finds each market's offset by searching the ~1.6 MB map for the metadata bytes. It runs three times per override (raise, rewind, restamp). Computing the offsets once and passing them to all three would avoid the repeated scans.
Minor
- Input names are inconsistent:
phoenix-open-positiontakesBid/Ask, while the cascade takeslong/short. liquidation()usesPubkey::new_unique()for the liquidator wallet. It is a test-oriented helper and makes the logged instructions non-deterministic. A fixed placeholder key would be clearer.trader_positionsbuilds a fullSurfnetSvmfor every MCP call. That is acceptable, but it costs more than a read-only tool suggests.
Overall the design is solid. The points above are mostly about robustness and efficiency in the cascade path.
| continue; | ||
| } | ||
| search.next_slot(); | ||
| search.run(&context.oracle_reports(mark)?)?; |
There was a problem hiding this comment.
The trader-discovery loop above skips a trader on any failure (let Ok(..) else { continue }). Here, though, search.run(...)? (and the boundary(...)? calls on L567 and L575) fail the whole cascade if one holder's oracle report or margin view fails, for example because of a stale or inconsistent account. Treat these per-trader failures as continue too, so one bad holder doesn't block the rest.
| .collect(); | ||
| listed.sort_unstable(); | ||
| listed.dedup(); | ||
| hydrate(svm, remote_ctx, &listed).await?; |
There was a problem hiding this comment.
This loads every trader listed in the GlobalTraderIndex into the live surfnet. That can be thousands of accounts. Unlike the sandbox runs, these writes are permanent. The loop below then stops after MAX_CASCADE_CANDIDATES (24). Consider loading in chunks inside the loop and stopping once 24 holders are found. Separately, taking the first 24 in pubkey order is arbitrary: large holders can be left out of the cascade. It may be worth mentioning this in the docs or log.
| /// so 64 stay well inside a transaction's 1.4 million. | ||
| const UNCROSS_MATCH_LIMIT: u64 = 64; | ||
| /// A book side holds at most `ORDERBOOK_CAPACITY` resting orders, so no move needs more cranks. | ||
| const MAX_UNCROSS_CRANKS: usize = ORDERBOOK_CAPACITY / UNCROSS_MATCH_LIMIT as usize; |
There was a problem hiding this comment.
Each match removes an order from either side, so emptying the book can take up to 2 * ORDERBOOK_CAPACITY matches, not ORDERBOOK_CAPACITY. Integer division also truncates. The uncross_done progress check makes this mostly theoretical, but ORDERBOOK_CAPACITY * 2 / UNCROSS_MATCH_LIMIT + 1 (or div_ceil) would match the comment's claim.
| if !template_id.starts_with("phoenix-") && account.owner != PHOENIX_PROGRAM_ID { | ||
| return Ok(None); | ||
| } | ||
| keep_markets_usable(svm, remote_ctx, target_slot).await; |
There was a problem hiding this comment.
This guard also covers any Phoenix-owned account on the generic IDL path (the _ => Ok(None) arm). So keep_markets_usable rewrites the PerpAssetMap override (thresholds, restamped slots, rewound funding) even for an unrelated Phoenix account override. If that's intended, a comment would help. If not, move the call after the template_id dispatch so only instruction-based templates trigger it.
| for entry in map_entries(account_pubkey, data)? { | ||
| let metadata = entry.metadata; | ||
| let metadata_bytes = metadata.as_bytes(); | ||
| let offset = data[cursor..] |
There was a problem hiding this comment.
Each market's offset comes from a linear byte search over the ~1.6 MB map. keep_markets_usable calls markets_in_map three times (raise, rewind, restamp) on every Phoenix override. Compute the offsets once and pass them to all three patchers. Ideally, derive them from the layout instead of searching for matching bytes, which could in theory match the wrong place.
| ("phoenixProgram", PHOENIX_PROGRAM_ID), | ||
| ("phoenixLogAuthority", log_authority()), | ||
| ("globalConfiguration", PHOENIX_GLOBAL_CONFIG), | ||
| ("liquidatorWallet", Pubkey::new_unique()), |
There was a problem hiding this comment.
Pubkey::new_unique() is a test-oriented helper built on a global counter, so the logged and returned liquidation instructions differ on every run. A fixed placeholder liquidator key would keep them deterministic and make clear this wallet is a stand-in.
…ks together, and address review notes
There was a problem hiding this comment.
Overview
This PR replaces the Phoenix Eternal byte-patch templates with templates that run Phoenix's own instructions in a sandbox copy of the local VM. It adds market move, liquidation-ready (cross-margin), cascade, trading/collateral templates and 12 config templates, plus a list_phoenix_trader_positions MCP tool. Running everything through Phoenix keeps the book, oracles and trader index consistent, which is a clear improvement over the old approach. The searches and sandboxing are structured carefully.
Both Greptile findings appear fixed in the head commit:
uncross_donenow compares resting order counts too.prepare_cascadefilters holders through Hawkeye'sview_margin_for_asset, notTrader::positions().
Issues worth addressing
- Cascade robustness: in
prepare_cascade, most per-holder failures are skipped, but the oracle-report run,marginand theboundary(...)?calls use?. One misbehaving holder can abort the whole cascade (see inline). - "Every holder" vs. the 24 cap: the PR body and README say the cascade prepares every holder of the chosen side. The scan actually stops after the first 24 matching traders in pubkey order, so the set is arbitrary on busy markets. Either document the cap or rank candidates (for example by distance to liquidation).
- Search path-dependence:
CrossMarginSearch::bandandboundaryreuse one sandbox for every bisection probe. Each probe stacks another oracle report on the previous ones. If the mark or its EMA depends on report history (therestamp_readingscomment suggests it does), a probe's result depends on earlier probes. The finalrun_atre-checks the chosen point, so this affects how good the band is, not whether the result is correct. - Cost:
prepare_cascadehydrates every trader in the GlobalTraderIndex, which can be thousands of RPC fetches. It then runs Hawkeye views until it has 24 candidates.trader_positions(MCP) builds a fullSurfnetSvmfor each call. Both are acceptable but could be slow on first play. - Unrelated file:
.github/workflows/claude-review.ymlhas nothing to do with Phoenix and probably belongs in its own PR. It also puts the PR title and body straight into the prompt, and thepost_feedbackjob has write permissions. That is a prompt-injection path for posting arbitrary review comments, though only from same-repo PRs.
Minor
uncross_donereturns success when a crank makes no progress but the book is still crossed. That silently writes back a crossed book. A warning, or an error naming the market, would make this visible.
Overall the PR is in good shape. The cascade error handling is the main fix I'd make before merging.
| continue; | ||
| } | ||
| search.next_slot(); | ||
| search.run(&context.oracle_reports(mark)?)?; |
There was a problem hiding this comment.
Most per-holder failures in this loop are skipped with continue (cancels, margin view, header). This line, and the boundary(...)? calls at 570 and 578, use ? instead, so one holder whose oracle-report run or Hawkeye margin view fails mid-search aborts the whole cascade. Consider treating these errors as continue for this holder too.
| }; | ||
| candidates.push((trader, authority, held)); | ||
| } | ||
| if candidates.len() == MAX_CASCADE_CANDIDATES { |
There was a problem hiding this comment.
The scan stops at the first 24 matching holders in pubkey order. On a busy market that set is arbitrary, and it may leave out the holders closest to liquidation. That contradicts the PR body and README ("prepares every holder of the chosen side"). Either document the cap, or collect all holders and rank them before truncating, for example by margin ratio from the MarginView you already read.
| } else { | ||
| 9 * MOVE_SCALE | ||
| }; | ||
| let mut search = self.base.trial(); |
There was a problem hiding this comment.
Every probe in both first_move searches runs on this one search sandbox, so each probe's oracle reports stack on the previous probes' reports. If Phoenix's mark or EMA depends on report history (the restamp_readings comment suggests it does), whether a move counts as liquidatable depends on the order of the probes. run_at re-checks the final point, but the band can come out skewed. Using self.base.trial() for each probe, or at least for each first_move, would make the search deterministic. The same applies to boundary in prepare_cascade.
| fn uncross_done(before: &(BboView, usize), after: &(BboView, usize)) -> bool { | ||
| let ((before, before_orders), (after, after_orders)) = (before, after); | ||
| !after.crossed() | ||
| || (after.best_bid_ticks, after.best_ask_ticks, after_orders) |
There was a problem hiding this comment.
When a crank makes no progress (same bid, ask and order count) while after.crossed() is still true, this returns true, and move_in returns Ok with a book that is still crossed. Later trades or liquidations can then fill against stale orders without any signal. Consider returning an error, or at least logging a warning naming the market, when the loop exits through the no-progress branch while the book is still crossed.
| @@ -0,0 +1,141 @@ | |||
| name: Perform a code review when a pull request is created. | |||
There was a problem hiding this comment.
I am not sure if this file should enter
| Use fetchBeforeUse: true on the first Phoenix scenario on a surfnet, false after that. | ||
| address: | ||
| type: pubkey | ||
| value: 2nHGAaEw3D5dd4hVueaUNoygkQFmoeKqRQWnSPqSMFUC |
There was a problem hiding this comment.
I think that should be not hardcoded ?
| protocols::{ | ||
| phoenix_eternal::v1::state_builder::{ | ||
| PHOENIX_PERP_ASSET_MAP, build_phoenix_collateral_scenario, phoenix_markets, | ||
| phoenix_eternal::v1::{ |
There was a problem hiding this comment.
In my view that is to be generic file for the mcp, but it includes phoenix-specific logic. Do we really need that logic to be here and for what reason?
3 participants
Rebuilds the Phoenix Eternal templates so every change goes through Phoenix's own instructions instead of byte patches, which keeps the accounts that make up a market consistent with each other.
create_phoenix_collateral_scenarioMCP tool are removed.list_phoenix_trader_positions(MCP) lists a trader's positions as Hawkeye reports them, with each position's maintenance margin.keep_markets_usablemoves funding timestamps that are ahead of the local clock back to it, and oracle reading slots that are behind it up to it. A running surfnet's clock trails mainnet, and Phoenix refused funding updates and mark prices otherwise, even with the raised stale thresholds.fetchBeforeUsefor Phoenix, how the accounts are loaded, and the case the grouped load does not cover (Phoenix accounts your own code read first), with its symptoms and recovery.json_to_txtx_value_for_idl_typeispub(crate).Tested with the full core suite with
ignore_tests_ci(815 passed), the Phoenix tests against mainnet state (67), and the MCP tests (23). In Studio on a fresh surfnet, with 14 minutes between loading a trader's positions and Play: its SOL, AVAX, LTC and BNB positions were prepared and liquidated by four real keeper transactions in the logged order while its other positions stayed; earlier runs covered nine positions of one trader. A SOL long cascade was liquidated by real keeper transactions, and an ETH move of -10% landed on its target price with the book uncrossed.Pairs with LimeChain/surfpool-web-ui#26.
Fix the early crank exit and stale cascade holder filter before merging.
Fix with agent prompt
Summary
Replaces Phoenix byte patches with official instructions run on a copied VM.
Diagram
%%{init: {'theme': 'neutral'}}%% flowchart TD A[Scheduled Phoenix override] --> B[Load missing accounts] B --> C[Keep local markets usable] C --> D[Copy local VM] D --> E[Run Phoenix instructions] E --> F{Instructions succeed?} F -->|No| G[Skip override and warn] F -->|Yes| H[Return changed accounts] H --> I[Write prepared accounts to surfnet] I --> J[User sends trades or liquidations]Reviews (1) · Last reviewed commit: "feat(scenarios/phoenix): liquidate sever..." · Reviewed by Greptile