Repository navigation
feat(scenarios): add Velocity support - state templates - #50
92Infinitus92 wants to merge 16 commits into
Conversation
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
There was a problem hiding this comment.
Summary
This PR adds Velocity, the Drift v2 fork, as a natively supported scenario protocol. It bundles the IDL verbatim and adds five templates: oracle price, perp market risk, exchange risk, fees and spot market risk. It also adds documentation, a large integration suite that runs the deployed program against live mainnet accounts, a shared diff_indices test helper (moved out of the Kamino and Pump tests), and updated registry counts.
The templates line up with the IDL:
max_open_interestis a top-levelu128onPerpMarket, so passing it as a decimal string is correct.min_borrow_rateandmax_token_depositsareu8andu64onSpotMarket.PythLazerOraclehasprice,conf,posted_slotandpublish_time.
The docs are thorough, especially the section on how prices go stale on a fork. Overall this looks good. My notes are mostly about scope and maintenance.
Notes
- Out-of-scope changes.
.github/workflows/claude-review.yml(commit319335f) and the MCPCallSurfnetRpcParams.paramsdescription change have nothing to do with Velocity. Consider splitting them into their own PRs. The MCP text also documents the internal serde shape ofJsonValue({"String":..},{"Number":..}). That ties the MCP schema to an implementation detail; makingparamsaccept plain JSON would be a better fix. - Brittle pin.
load_mainnetassertsDEPLOYED_SLOT == 451_568_782, and the docs say Velocity "upgrades often". Every redeploy will fail all 8 Velocity tests until someone bumps the constant. They only run underintegration-tests, but if that feature runs in CI, this will break unrelated PRs. Consider gating these tests behind an env var or#[ignore]. - Name mismatch. The protocol lives under
velocity/v1withVELOCITY_V1_*constants, but the YAML saysversion: v2.169.0and the README says "built on Drift v2". That's harmless, but someone will eventually trip over it.
| value: "2etx5NvPNxeMZ7EfHE6GjJfW2imRYEUANehNS1WB4CVW" | ||
| llm_context: | | ||
| The Velocity State is one account: 2etx5NvPNxeMZ7EfHE6GjJfW2imRYEUANehNS1WB4CVW. Set | ||
| fetchBeforeUse: true and put it in the override's account as {"pubkey":"<state>"}. |
There was a problem hiding this comment.
velocity-fees and velocity-exchange-risk both target the State and both tell the model to set fetchBeforeUse: true. The materializer only skips the upstream refetch for an account already settled in the same slot (settled_this_slot in svm.rs). So if a later scenario or a later slot applies velocity-fees, it refetches the mainnet State. That silently puts mainnet's deposit pause back, resets slots_before_stale_for_margin to 120 and undoes the earlier guard-rail changes. Say this in both llm_contexts: put both State templates in the same slot, or drop fetchBeforeUse once the State has been forked.
| exchange_status pauses actions on every market. It is a bit mask, 0 runs everything: 1 | ||
| deposits, 2 withdrawals, 4 AMM, 8 fills, 16 liquidations, 32 funding, 64 settle PnL, 128 AMM | ||
| immediate fills. A paused action fails with ExchangePaused (6024). The State comes with | ||
| mainnet's pauses; exchange_status: 0 lifts them. |
There was a problem hiding this comment.
Since deposits on mainnet have been paused since 2026-10-30, every Velocity scenario that deposits needs velocity-exchange-risk with exchange_status: 0. The velocity-oracle-price and velocity-perp-market-risk llm_contexts don't mention this. An LLM building a 'SOL drops 20%, then liquidate' scenario around a fresh deposit will hit ExchangePaused (6024). Add a one-line pointer to those templates as well.
| <[Account; 2]>::try_from(fetch(&[PROGRAMDATA, STATE]).await).unwrap(); | ||
| let deployed_slot = u64_at(&programdata.data, 4..12); | ||
| assert_eq!( | ||
| deployed_slot, DEPLOYED_SLOT, |
There was a problem hiding this comment.
This hard assert makes every Velocity test fail on each program upgrade, and the module doc says upgrades are frequent. If integration-tests runs in CI, unrelated PRs will go red. Consider making the pin opt-in (for example an env var, or #[ignore] with a dedicated job), or failing only the IDL round-trip test and letting the behavioural tests skip.
| pub method: String, | ||
| #[schemars(description = "The parameters to pass to the RPC method")] | ||
| #[schemars( | ||
| description = "The parameters to pass to the RPC method. Wrap every value at every depth by its type: a string as {\"String\":\"...\"}, a whole number as {\"Number\":5}, a boolean as {\"Bool\":true}, a list as {\"Array\":[...]} and an object as {\"Object\":{\"key\":<wrapped value>}}; object keys stay plain. Example for getProgramAccounts with a filter: [{\"String\":\"<program id>\"},{\"Object\":{\"encoding\":{\"String\":\"base64\"},\"filters\":{\"Array\":[{\"Object\":{\"memcmp\":{\"Object\":{\"offset\":{\"Number\":0},\"bytes\":{\"String\":\"<base58>\"}}}}}]}}}]" |
There was a problem hiding this comment.
This change is unrelated to Velocity. It also documents the serde enum encoding of txtx JsonValue in the public MCP schema. If the goal is to let models pass plain JSON params, a custom deserializer or switching to serde_json::Value would be sturdier than asking every caller to hand-wrap each value.
There was a problem hiding this comment.
Summary
This PR adds Velocity (Drift v2 fork) support: the IDL that the program publishes on chain, bundled unchanged; five override templates (oracle price, perp market risk, exchange risk/pauses, fees and spot market risk); registry wiring and count updates; a long README; and mainnet-backed integration tests that run the deployed program. It also moves diff_indices into the shared test helpers, a small cleanup.
I checked the template paths against the bundled IDL. PythLazerOracle.{price,conf,posted_slot,publish_time}, State.promo_fee_tier, perp_fee_structure.fee_tiers (array of 10), oracle_guard_rails.validity.*, max_open_interest (u128, so a string) and max_token_deposits (u64, so a number) all exist and match the docs. Two State templates in the same slot both using fetchBeforeUse: true is safe, because materialize_overrides skips accounts in settled_this_slot. The docs are thorough and the templates look right.
Issues
- Changes outside the feature:
.github/workflows/claude-review.yml(commit319335f) and theCallSurfnetRpcParams.paramsschema description incrates/mcphave nothing to do with Velocity. Consider moving them into separate PRs so they can be reviewed and reverted on their own. - Tests will keep breaking: the integration tests assert the ProgramData deploy slot equals
DEPLOYED_SLOT, and the module doc says Velocity "upgrades often". Every upstream redeploy will turn--features integration-testsred until someone moves the pin. If CI runs these tests, consider skipping or warning on a mismatch instead of panicking, or gating them behind a separate opt-in. - Large batch fetch:
load_mainnetfetches every perp market, spot market and vault in onegetMultipleAccounts. Public RPCs cap that call at 100 keys, so it will fail onceperp + 2*spot > 100. Chunking the call would fix this; keep the market and its vault in the same request so their snapshots still agree. - Minor: the maker rebate example differs between the README (
1/100000) and the YAML (3). Not wrong, but pick one example.
| properties: | ||
| - path: price | ||
| label: Price | ||
| description: "Price x 10^8 (every live oracle has exponent -8). Example: 8500000000 for $85" |
There was a problem hiding this comment.
Hardcoding "every live oracle has exponent -8" is fragile: one new market with a different exponent and an LLM-built scenario mis-scales the price by orders of magnitude. The llm_context already has the model read the oracle with jsonParsed, so tell it to scale by the oracle's own exponent (price = USD × 10^-exponent) rather than assume 10^8. Same for the README's Number formats table.
| const STATE: Pubkey = Pubkey::from_str_const("2etx5NvPNxeMZ7EfHE6GjJfW2imRYEUANehNS1WB4CVW"); | ||
| const PROGRAMDATA: Pubkey = Pubkey::from_str_const("HkRf36jvUB32dnHjHqpMnPcKJhUP2TUKJTH7RDTifJm7"); | ||
| /// Release program-velocity-v2.170.0. | ||
| const DEPLOYED_SLOT: u64 = 464_750_765; |
There was a problem hiding this comment.
Pinning the deploy slot makes these tests fail on every Velocity upgrade, and the module doc says those are frequent. That's acceptable if integration tests are run manually. If CI runs them, consider skipping with a warning when the slot differs, or gating this module behind its own env var, so an upstream deploy doesn't block unrelated PRs.
| Pull request title and body: | ||
| ---- | ||
| ${{ github.event.pull_request.title }} | ||
| ${{ github.event.pull_request.body }} |
There was a problem hiding this comment.
This CI workflow has nothing to do with Velocity support; please move it into its own PR. Also, the PR title and body go into the prompt unescaped. The tool allowlist is read-only and fork PRs are skipped, so the risk is limited, but any same-repo author can steer the review output that post_feedback posts with pull-requests: write.
|
|
||
| Set fetchBeforeUse: true. Put the oracle in the override's account as {"pubkey":"<oracle>"}, | ||
| never in values. A later override of the same account, in another scenario or slot, uses | ||
| fetchBeforeUse false, since a reload drops earlier local changes to it. |
There was a problem hiding this comment.
Nit: "in another scenario or slot" is accurate, since the same slot is protected by settled_this_slot. Still, adding "(the same slot is fine)" would save an LLM from wrongly setting fetchBeforeUse: false when it combines velocity-exchange-risk and velocity-fees on the State in one slot. That mistake would skip the upstream fetch if the State isn't loaded yet.
There was a problem hiding this comment.
Overview
This PR adds five Velocity (Drift fork) state templates: oracle price, perp market risk, exchange risk, fees and spot market risk. It bundles the published 2.170.0 IDL and registers the protocol, with registry counts updated from 81 to 86. It moves diff_indices into the shared test helpers and adds a large integration suite pinned to the deploy slot. The templates and llm_context are thorough and carefully worded. The tests that check field offsets against the live program are a good safeguard against IDL drift.
Issues
- The oracle exponent fix wasn't applied everywhere. Commit b807bfe changed the
priceproperty description and the README to read the exponent from the oracle. The oracle template'sllm_context(overrides.yaml:49) still saysprice is the USD price x 10^8; leave exponent alone. That is the text the LLM actually follows, so it should match. - Changes outside the feature. The PR adds
.github/workflows/claude-review.ymland rewrites the MCPCallSurfnetRpcParams.paramsschema description (crates/mcp/src/surfpool/mod.rs). Neither is about Velocity, and the MCP change affects every RPC call made through MCP. Consider splitting them into their own PRs, or at least listing them in the PR description. - Two templates write the same State account.
velocity-exchange-riskandvelocity-feesboth target the State. Thellm_contextexplains how to combine them withfetchBeforeUse. An ordering bug there would silently drop an unpause, so a test that applies both in one scenario would help. - Tests will break on every redeploy. They fail whenever Velocity redeploys (
DEPLOYED_SLOTpin), and the PR says the program "upgrades often." That's intended, but if these run in CI on a schedule, expect regular red builds.
Overall the PR looks good once the leftover 10^8 wording is fixed.
|
|
||
| Write price, posted_slot and publish_time together. Read the oracle and the Clock in one | ||
| getMultipleAccounts call with encoding jsonParsed: [<oracle>, | ||
| "SysvarC1ock11111111111111111111111111111111"]. price is the USD price x 10^8; leave exponent |
There was a problem hiding this comment.
This still hardcodes price is the USD price x 10^8. b807bfe changed the property description and the README to read the exponent from the oracle, but not this text, and the LLM follows llm_context. Suggest: price is the USD price x 10^-exponent, with exponent read from the oracle in the same call; leave exponent alone.
There was a problem hiding this comment.
Summary
This PR adds Velocity, the Drift v2 fork, as a native scenario protocol. It includes the bundled 2.170.0 IDL and five state templates: oracle price, perp market risk, exchange risk, fees and spot market risk. It updates the registry and its counts, moves diff_indices into the shared test helpers, and adds integration tests against the program at its pinned deploy slot.
The templates are careful. The llm_context blocks explain how to find markets, how prices go stale, the error codes and the guard rails, and the integration tests check each documented behaviour against the deployed program. Overall this looks good to merge after the scope and brittleness points below are settled.
Issues
-
Unrelated changes are bundled in.
.github/workflows/claude-review.ymladds a CI code-review workflow that has nothing to do with Velocity.- The change to the
CallSurfnetRpcParams.paramsdescription incrates/mcp/src/surfpool/mod.rsalters MCP behaviour for every protocol.
Both are reasonable on their own, but they should go in separate PRs so each can be reviewed and reverted independently.
-
The tests are pinned to one deploy.
load_mainnetasserts thatDEPLOYED_SLOT == 464_750_765, and the module doc says Velocity "upgrades often." Every redeploy will therefore fail all the Velocity tests until someone updates the pin and the IDL. If these tests run in any scheduled or PR CI withintegration-testsenabled, expect recurring red builds that have nothing to do with the change under test. Consider checking for the pin mismatch once and skipping or ignoring the tests with a clear message, rather than failing every one. -
The Velocity test file repeats IDL byte offsets by hand.
crates/core/src/tests/velocity/mod.rshard-codes about 50 offsets taken from the IDL.the_bundled_idl_matches_the_pinned_release_on_mainnetcovers the round trip, but it would be cheaper to maintain if the offsets were derived from the bundled IDL or asserted against it, so an IDL bump doesn't leave stale constants behind. -
Minor: the version label doesn't match the directory. The templates live under
velocity/v1/but declareversion: v2.170.0.
| in the same slot can both keep true. | ||
|
|
||
| An account pays the perp fees of the higher of its 30-day volume tier and promo_fee_tier. | ||
| The program uses tiers 0 to 3 only, so promo_fee_tier: 3 puts every account on tier 3. |
There was a problem hiding this comment.
In upstream Drift, determine_user_fee_tier can put an account on tiers 4 and 5 (high stake or volume). If Velocity keeps that logic, promo_fee_tier: 3 will not move those accounts onto tier 3, so the claim that "every trader pays them" would be wrong for top-volume accounts. Please confirm the 0–3 limit against the 2.170.0 source, or make the promo_fee_tier description and this context less absolute.
| @@ -0,0 +1,330 @@ | |||
| protocol: Velocity | |||
| version: v2.170.0 | |||
There was a problem hiding this comment.
Nit: the directory is velocity/v1 and the other protocols use short labels like v1 or v2. Pinning the release string here means it has to change on every IDL bump, and the directory name and the label will drift apart. Consider version: v1, and record the release number in the README or the IDL metadata.
| let [programdata, state] = | ||
| <[Account; 2]>::try_from(fetch(&[PROGRAMDATA, STATE]).await).unwrap(); | ||
| let deployed_slot = u64_at(&programdata.data, 4..12); | ||
| assert_eq!( |
There was a problem hiding this comment.
This hard assert fails every Velocity test after any redeploy, and the module docs say Velocity upgrades often. Consider making only the IDL test fail here, and having the behavioural tests skip with a clear message, so unrelated CI runs don't go red until someone moves the pin.
| .map(|i| by_index(b"spot_market_vault", i)) | ||
| .collect(); | ||
| let markets = [perp_markets.clone(), spot_markets.clone(), vaults.clone()].concat(); | ||
| let market_accounts = fetch(&markets).await; |
There was a problem hiding this comment.
fetch sends every perp market, spot market and vault in one getMultipleAccounts call. Most RPCs cap that call at 100 keys, so once Velocity lists enough markets (perp + 2 × spot > 100) this breaks with an opaque RPC error. The oracle and mint fetch at line 355 has the same limit. Consider chunking the requests, or at least asserting on the count up front.
There was a problem hiding this comment.
Velocity state templates
This PR adds five Velocity templates: oracle price, perp market risk, exchange risk, fees and spot market risk. It also bundles the 2.170.0 IDL and adds an IDL-backed keep-alive template (velocity-oracle-publication) that reposts a moved oracle every slot. Tests run the deployed program, pinned to its deploy slot, against live accounts. diff_indices moves into the shared test helpers.
The templates are carefully written. The llm_context explains the pause switches, the staleness windows, the MM-oracle precedence and the price bands, and the README matches the YAML. The registry counts and tests are updated. Velocity is the first protocol to back a keep-alive template with an IDL, and the new registry test covers that path.
Issues
- Unrelated changes are bundled in.
.github/workflows/claude-review.yml(a new CI workflow that needs a secret and posts reviews) and the expandedparamsdescription incrates/mcp/src/surfpool/mod.rshave nothing to do with Velocity. Please split them into their own PRs so they can be reviewed and reverted on their own. - The PR description is out of date. It says nothing reposts Velocity oracles and that keep-alive (solana-foundation#906) will come in a follow-up. Commit
fb68145already adds keep-alive here, and the README andllm_contextsay "Surfpool keeps it posted". Please update the description, including the "A scenario that moves a price posts it at the current slot" wording. - Maintenance risk. The integration tests hardcode many byte offsets and the deploy slot. The module docs say a Velocity upgrade breaks them until the pin is moved. That's fine for opt-in
integration-tests, but someone should own that process.
| - path: conf | ||
| label: Confidence | ||
| description: Uncertainty band in the same scale as price; scale it with price so their ratio holds | ||
| - path: posted_slot |
There was a problem hiding this comment.
posted_slot and publish_time are offered as editable properties, but keep-alive overwrites both every slot from the slot the override plays. The llm_context (line 54) also says "Write price and conf only". A value a user sets in Studio would be silently replaced. Either drop these two from properties, or say in their descriptions that keep-alive overwrites anything written here.
| VELOCITY_V1_OVERRIDES_CONTENT, | ||
| "velocity", | ||
| ); | ||
| let idl = serde_json::from_str(VELOCITY_V1_IDL_CONTENT).expect("velocity idl"); |
There was a problem hiding this comment.
This parses the roughly 22k-line IDL a second time on every TemplateRegistry::new(), because load_protocol_overrides already deserializes VELOCITY_V1_IDL_CONTENT. Consider parsing it once and passing it to both calls, or having load_protocol_overrides return or accept the parsed Idl. It's minor, but TemplateRegistry::new() is called in many tests. Also, expect("velocity idl") doesn't match the panic!("unable to load ...: {e}") style used elsewhere.
| - path: promo_fee_tier | ||
| label: Promo Fee Tier | ||
| description: "Lowest fee tier any account is charged at. Accounts pay the fees of the higher of their 30-day volume tier and this one; 3 puts every account on tier 3" | ||
| - path: perp_fee_structure.fee_tiers.3.fee_numerator |
There was a problem hiding this comment.
Only tier 3 can be edited, and the template depends on promo_fee_tier: 3 to put everyone on it. If an account's volume tier is ever above 3, or the program changes how many tiers it uses, the template quietly stops applying to some accounts. Consider stating in description that this template sets every account's fees through the promo tier, so the UI makes that clear without the llm_context.
| pub method: String, | ||
| #[schemars(description = "The parameters to pass to the RPC method")] | ||
| #[schemars( | ||
| description = "The parameters to pass to the RPC method. Wrap every value at every depth by its type: a string as {\"String\":\"...\"}, a whole number as {\"Number\":5}, a boolean as {\"Bool\":true}, a list as {\"Array\":[...]} and an object as {\"Object\":{\"key\":<wrapped value>}}; object keys stay plain. Example for getProgramAccounts with a filter: [{\"String\":\"<program id>\"},{\"Object\":{\"encoding\":{\"String\":\"base64\"},\"filters\":{\"Array\":[{\"Object\":{\"memcmp\":{\"Object\":{\"offset\":{\"Number\":0},\"bytes\":{\"String\":\"<base58>\"}}}}}]}}}]" |
There was a problem hiding this comment.
This MCP schema description change is unrelated to Velocity support. Please move it to a separate PR.
| @@ -0,0 +1,141 @@ | |||
| name: Perform a code review when a pull request is created. | |||
There was a problem hiding this comment.
This new CI workflow is out of scope for a Velocity templates PR. It needs ANTHROPIC_API_KEY and has a job with pull-requests: write. Please submit it in a separate PR so maintainers can review it on its own.
There was a problem hiding this comment.
Velocity state templates (PR #50)
This PR adds the Velocity (Drift v2 fork) IDL at 2.170.0. It also adds five override templates (oracle price, perp market risk, spot market risk, fees, exchange risk) and an owned keep-alive template that reposts every loaded Velocity PythLazerOracle from the slot velocity-oracle-price plays. Registry counts and tests are updated, and there is a large integration suite against the deployed program. The YAML and llm_context are thorough: bit masks, scales and error codes are documented, and the fetchBeforeUse and reload pitfalls are spelled out.
Overall
The PR looks solid. Most of my notes are about the docs matching what the code does, plus one scope or perf question.
- The PR description is out of date. It says keep-alive is a follow-up once solana-foundation#906 lands. But solana-foundation#906 is merged into this branch, and
3aae662already wireskeep_alivewith anownedaddress. Please update the description and the "prices on a fork" section so reviewers and Studio know prices are now reposted automatically. - Keep-alive scope is wide. One
velocity-oracle-priceoverride reposts every Velocity oracle the surfnet has loaded, every slot, from then on.- Cost: this is N account writes per slot. Worth checking with a surfnet that has loaded many markets.
- Hidden staleness: it silently hides staleness for every other market. Any staleness scenario now needs
keepAlive: falseon each oracle it wants stale. That is documented inllm_context, but it is easy to miss in the UI. - Possible alternative: scope keep-alive to the overridden oracle plus the quote/USDT oracle by default. If the wide scope is intended, keep it.
crates/mcpadds a longparamsschema description that has nothing to do with Velocity. It changes MCP behaviour for every protocol, so it would fit better in its own PR.- Test gap: the
keepAlive: falseexclusion from the owned sweep is exercised once (line ~1272). An assertion that an excluded oracle'sposted_slotstays put across several slots after other oracles are reposted would lock that contract down.
| VELOCITY_V1_OVERRIDES_CONTENT, | ||
| "velocity", | ||
| ); | ||
| let idl = serde_json::from_str(VELOCITY_V1_IDL_CONTENT).expect("velocity idl"); |
There was a problem hiding this comment.
This parses the ~22.5k-line Velocity IDL a second time; load_protocol_overrides already parsed it. Startup cost isn't huge, but TemplateRegistry::new() runs often in tests. Consider parsing once and calling load_collection(Some(idl.clone()), …) and load_keep_alive(Some(idl), …) directly. Also, expect("velocity idl") doesn't match the panic!("unable to load {} idl: {}") wording used by the other loaders.
| - path: conf | ||
| label: Confidence | ||
| description: Uncertainty band in the same scale as price; scale it with price so their ratio holds | ||
| - path: posted_slot |
There was a problem hiding this comment.
posted_slot and publish_time are offered as editable properties, but with keep-alive on (the default) both are overwritten on the very next slot, so a user-supplied value has no lasting effect. Either drop them from this template or say in the description that they only stick when keepAlive: false. Otherwise Studio users will set them and wonder why nothing changes.
| description: Pyth Lazer timestamp in microseconds. Keep-alive writes the Clock time every slot | ||
| address: | ||
| type: pubkey | ||
| keep_alive: |
There was a problem hiding this comment.
address: { type: owned } makes one oracle move repost every loaded Velocity oracle each slot, indefinitely. That costs N writes per slot, and it also stops any other market from going stale unless each one is explicitly overridden with keepAlive: false. Is this wide scope intended? If yes, please mention the side effect in the template's description as well (not only in llm_context), since UI users won't see llm_context.
| - **Pump v1** - Bonding curve launchpad with curve reserve and global config override templates | ||
| - **Phoenix Eternal** - Perpetuals venue with trader collateral, mark price, maintenance margin, market fee, withdraw limit, trader capability, stop-loss trigger and delegated permission templates. See [protocols/phoenix-eternal/README.md](./protocols/phoenix-eternal/README.md) | ||
| - **PumpSwap v1** - Constant-product AMM with pool state and global config override templates, including canonical pool derivation for migrated pump.fun coins | ||
| - **Velocity** - Perpetuals exchange built on the Drift v2 program, with oracle price, perp market risk, exchange status, trading fee and spot market risk templates; an oracle a scenario moves is kept alive. See [protocols/velocity/v1/README.md](./protocols/velocity/v1/README.md) |
There was a problem hiding this comment.
"an oracle a scenario moves is kept alive" understates the behaviour. The keep-alive import uses an owned address, so every loaded Velocity oracle is reposted, not just the moved one.
| )] | ||
| pub method: String, | ||
| #[schemars(description = "The parameters to pass to the RPC method")] | ||
| #[schemars( |
There was a problem hiding this comment.
This changes the MCP tool schema for every protocol and has nothing to do with Velocity templates. I'd split it into its own PR. Also, a hand-written example of the internal JsonValue wrapping inside a description will drift silently if that enum changes. Could the wrapping be generated from or tested against the actual type?
There was a problem hiding this comment.
Summary
This PR adds Velocity (the Drift v2 fork) support. It includes the 2.170.0 IDL, five override templates (oracle price, perp market risk, spot market risk, exchange risk and fees), a keep-alive template that reposts oracles, registry wiring and a thorough mainnet-pinned integration suite. It also moves diff_indices into the shared test helpers and expands the MCP params description.
Overall it is high quality. The bit masks for the perp, spot and exchange pause flags match Drift's PerpOperation, SpotOperation and ExchangeStatus. The tests check the IDL round-trip, the discriminator layout and the behaviour of each template against the deployed program.
Issues
- PR description is out of date. It says a fork posts a moved price once, at the current slot, and that keep-alive (solana-foundation#906) will come in a follow-up. The branch already merges solana-foundation#906 and imports
velocity-oracle-publication, which reposts every loaded Velocity oracle every slot, and the README andllm_contextdescribe that behaviour. The description should be updated so reviewers and Studio (surfpool-web-ui#37) work from the right model. - Unrelated change. The MCP
CallSurfnetRpcParams.paramsdescription change has nothing to do with Velocity. It would be easier to review in its own PR, or should at least be mentioned in this PR's description. - Keep-alive scope. The owned-account keep-alive rewrites
posted_slotandpublish_timeon every loadedPythLazerOracleevery slot once any oracle override plays. This is documented, but it quietly changes which price fills read for markets the user never touched: once the clock passes a market'smm_oracle_sequence_id, its fills read the oracle instead of the MM price. Consider adding a short note in the templatedescriptiontoo, not only inllm_contextand the README, because the Studio UI may only show the description. - Fee tier assumption. The
promo_fee_tier: 3guidance depends on the claim that "the program uses tiers 0 to 3 only". Upstream Drift computes tiers 0 to 5. If Velocity kept that, high-volume accounts would keep their own tier and not pay the overridden fees. If a test already checks this, mention it next to the claim; if not, it is worth adding one.
| VELOCITY_V1_OVERRIDES_CONTENT, | ||
| "velocity", | ||
| ); | ||
| let idl = serde_json::from_str(VELOCITY_V1_IDL_CONTENT).expect("velocity idl"); |
There was a problem hiding this comment.
This parses the ~22.5k-line IDL a second time on every TemplateRegistry::new(), after load_protocol_overrides already parsed it. It also uses .expect, where load_protocol_overrides handles a parse error with a match. Consider parsing once and passing the result to both calls, or having load_protocol_overrides return the parsed Idl.
| description: Pyth Lazer timestamp in microseconds. Keep-alive writes the Clock time every slot | ||
| address: | ||
| type: pubkey | ||
| keep_alive: |
There was a problem hiding this comment.
The owned keep-alive covers every Velocity PythLazerOracle, not just the one this override writes. One price override therefore starts reposting every oracle the surfnet has loaded, and that also changes which price fills read (oracle vs MM) on markets the user never touched. llm_context and the README explain this, but the template description does not, and that may be all the UI shows. Consider adding one line about it there.
| description: "Bit flags: 1 deposits, 2 withdrawals, 4 AMM, 8 fills, 16 liquidations, 32 funding, 64 settle PnL, 128 AMM immediate fills. 0 means everything runs" | ||
| - path: oracle_guard_rails.validity.slots_before_stale_for_margin | ||
| label: Margin Staleness Window | ||
| description: "Margin checks refuse a price older than this many 400 ms units, counted from its posted_slot at two slots per unit, and three times that for USDT. Live: 120 (240 slots). Raise it to keep a price nothing reposts usable longer. Example: 1000000" |
There was a problem hiding this comment.
The "two slots per unit" conversion depends on the State's slot_duration_ms (the tests read SLOT_DURATION_MS). If the program derives the factor from that field, consider saying so here (e.g. "units of 400 ms, converted using the State's slot_duration_ms"), so the 240-slot figure isn't read as a fixed constant.
There was a problem hiding this comment.
Summary
This PR adds Velocity (the Drift v2 fork) as a built-in scenario protocol. It includes:
- the 2.170.0 IDL, copied as published
- five override templates: oracle price, perp market risk, exchange risk, fees and spot market risk
- a keep-alive file that reposts every
PythLazerOraclethe program owns oncevelocity-oracle-priceplays - a large integration suite that runs the deployed program, pinned to its deploy slot
The YAML lines up with the keep-alive machinery already in core: owned addresses work, and so do the $slot and $unix_us placeholders. The registry test checks that the oracle discriminator is unique, so keep-alive can only write oracles. The README and llm_context are thorough and accurate about staleness windows, MM-oracle precedence and the pause switches.
Overall this looks good. Issues:
- The PR description is out of date. It says nothing reposts Velocity oracles and that keeping them posted is a follow-up to solana-foundation#906. The branch already merges solana-foundation#906 and makes
velocity-oracle-pricerepost every loaded oracle each slot, and the README says so. The description should match before merge. - An unrelated change is bundled in. The
crates/mcp/src/surfpool/mod.rschange rewrites theparamsdescription for every MCP RPC call. It's useful, but it has nothing to do with Velocity and would be easier to review as its own PR. The wrapped-value example is also long enough to bloat every tool schema. - The IDL is large and parsed twice. It is about 500 KB, and
TemplateRegistry::new()now parses it twice; see the inline comment.
| VELOCITY_V1_OVERRIDES_CONTENT, | ||
| "velocity", | ||
| ); | ||
| let idl = serde_json::from_str(VELOCITY_V1_IDL_CONTENT).expect("velocity idl"); |
There was a problem hiding this comment.
This parses the ~500 KB Velocity IDL a second time; load_protocol_overrides has already parsed it just above. TemplateRegistry::new() runs on every server start, in MCP and HTTP setup, and in dozens of tests, so the cost adds up. Parse it once and pass a clone to both calls, e.g. let idl: Idl = serde_json::from_str(..)?; self.load_collection(Some(idl.clone()), ..); self.load_keep_alive(Some(idl), ..). The expect message should also match the unable to load {protocol} idl style used elsewhere.
| } | ||
|
|
||
| /// A field of the user's position in `market`, 0 when it holds none. | ||
| fn position_field(user: &[u8], market: u16, field: Range<usize>) -> i64 { |
There was a problem hiding this comment.
An empty PerpPosition slot has market_index == 0, which is the same as SOL_PERP. For market 0, find returns the first empty slot if it comes before the real SOL position, for example after an earlier position in slot 0 is closed. The function then reads 0. This works today only because of the order the tests open positions in. Also require the slot to be in use (non-zero base, quote or open orders), or check every slot with a matching index.
|
|
||
| fn set_slot(&self, slot: u64) { | ||
| let mut clock = self.clock(); | ||
| clock.unix_timestamp += (slot.saturating_sub(clock.slot) / 5) as i64; |
There was a problem hiding this comment.
/ 5 hardcodes the 200 ms slot (5 slots/s) and assumes slot >= clock.slot. When the slot moves backward, saturating_sub leaves the timestamp unchanged while the slot goes down. If that's intended, a short comment would help, or derive the rate from the State's SLOT_DURATION_MS, which the file already reads.
…the others stay fresh
There was a problem hiding this comment.
I found no blocking bugs in this PR. I did not run the tests/velocity integration suite or the unit tests.
What the PR adds
- Five state templates for Velocity, the Drift fork: oracle price, perp market risk, exchange risk, fees and spot market risk.
- A keep-alive template,
velocity-oracle-publication. When an oracle override runs withkeepAlive: true, Surfpool reposts everyPythLazerOracleaccount the program owns, once per slot. - Bundled 2.170.0 IDL (about 22.5k lines), plus a large
tests/velocityintegration suite that runs the deployed program at its pinned deploy slot. - Small cleanups:
diff_indicesmoves into a shared test helper and is reused by the kamino and pump tests.- The MCP
paramsdescription now explains the wrappedJsonValueencoding.
Assessment
The YAML matches the keep-alive schema. owner: in the YAML deserializes to owners: Vec<String> through YamlOwners. The template counts and the registry test are consistent. The llm_context text is detailed and states the program's limits and error codes.
Points to address
- The PR description is out of date. It says the keep-alive work (solana-foundation#906) is a future follow-up and that a moved price is posted once at the current slot. The branch now merges solana-foundation#906 and ships owned keep-alive, which reposts every loaded Velocity oracle each slot.
- One oracle override changes the staleness behaviour of every oracle. This is documented, but it is a big side effect. The workaround for keeping one oracle stale needs two overrides, so it is easy to get wrong. A short note in the Velocity README's opening lines would help.
- This is the first protocol to load a keep-alive file.
check_keep_alive_importsruns insideTemplateRegistry::new()and panics there. So any change to how the Velocity IDL'sPythLazerOraclediscriminator is handled would break every registry construction, not just Velocity scenarios. The new registry test helps here; keep it.
| VELOCITY_V1_OVERRIDES_CONTENT, | ||
| "velocity", | ||
| ); | ||
| let idl = serde_json::from_str(VELOCITY_V1_IDL_CONTENT).expect("velocity idl"); |
There was a problem hiding this comment.
This parses the roughly 22k-line Velocity IDL a second time. load_protocol_overrides has already deserialized it a few lines above, and this runs on every TemplateRegistry::new(), which the test suite calls dozens of times. Consider parsing it once and passing it to both loaders. Also, expect here panics, while load_protocol_overrides handles a bad IDL its own way. The two paths should fail the same way.
| pub method: String, | ||
| #[schemars(description = "The parameters to pass to the RPC method")] | ||
| #[schemars( | ||
| description = "The parameters to pass to the RPC method. Wrap every value at every depth by its type: a string as {\"String\":\"...\"}, a whole number as {\"Number\":5}, a boolean as {\"Bool\":true}, a list as {\"Array\":[...]} and an object as {\"Object\":{\"key\":<wrapped value>}}; object keys stay plain. Example for getProgramAccounts with a filter: [{\"String\":\"<program id>\"},{\"Object\":{\"encoding\":{\"String\":\"base64\"},\"filters\":{\"Array\":[{\"Object\":{\"memcmp\":{\"Object\":{\"offset\":{\"Number\":0},\"bytes\":{\"String\":\"<base58>\"}}}}}]}}}]" |
There was a problem hiding this comment.
This guidance is useful but leaves some cases out:
JsonValue::Numberis ani64, so floats andu64values abovei64::MAXcannot be sent asNumber. Say that here.- The
Nullvariant is not mentioned. It is a unit variant, sent as the plain string"Null", which a model would not guess. - The
PublicKey,MintAddressandProgramIdvariants are not mentioned either. Either list them or say thatStringis fine for pubkeys.
| description: "Bit flags: 1 deposits, 2 withdrawals, 4 AMM, 8 fills, 16 liquidations, 32 funding, 64 settle PnL, 128 AMM immediate fills. 0 means everything runs" | ||
| - path: oracle_guard_rails.validity.slots_before_stale_for_margin | ||
| label: Margin Staleness Window | ||
| description: "Margin checks refuse a price older than this many 400 ms units past its posted_slot, converted with the State's slot_duration_ms (200 ms live, so two slots per unit), and three times that for USDT. Live: 120 (240 slots). Raise it to keep a price nothing reposts usable longer. Example: 1000000" |
There was a problem hiding this comment.
This description says the live value is 120 (240 slots), which is right. Two related things look off, though:
- When a
velocity-oracle-priceoverride runs withkeepAlive: true, every loaded oracle is reposted each slot, so this window only matters for oracles written withkeepAlive: false, or before any oracle override has run. Thellm_contextsays so, but this UI description does not. Studio users may raise it thinking they need to. - The example
1000000is the admin-path maximum. A smaller, more typical example would be safer.
Adds Velocity (vELoC1audYbSYVRXn1vPaV8Axoa9oU6BYmNGZZBDZ1P, the Drift fork) state templates, so a scenario can move a market's oracle price and change the market, spot and exchange settings that decide margin, liquidations and fees.
velocity-oracle-pricemoves the price a perp market reads;velocity-perp-market-risk,velocity-spot-market-risk,velocity-feesandvelocity-exchange-riskchange one market or the exchange.exchange_status: 0withvelocity-exchange-risklifts them locally.Prices on a fork: the program ages a price by slots since it was posted. Margin checks refuse a price older than the State's
slots_before_stale_for_margin(240 slots, three times that for USDT), whichvelocity-exchange-riskcan raise; AMM fills and settles need a price within 20 slots; liquidations ignore the age.velocity-oracle-priceimports a keep-alive template over everyPythLazerOraclethe program owns (theownedkind from #53, which this PR is stacked on; merge #53 first). Once an override withkeepAlive: trueplays, every Velocity oracle the surfnet has loaded is reposted at the current slot and Clock time every slot. To keep one oracle stale while the others stay fresh, thellm_contextgives the recipe: one override withkeepAlive: falseand one withkeepAlive: true. No keeper repegs the AMM on a fork, so a fill against the AMM sendsupdate_ammsfirst; the README covers it.Tests:
tests/velocityruns the deployed program (pinned to its deploy slot) against live accounts: IDL round trips, price moves liquidating positions within the guard rails, margin stress, fee tiers, open interest caps and reduce-only, spot risk, the margin window and the AMM window, and keep-alive through later slots and a time travel. fmt, clippy and the core suite pass.Shared with the other open protocol PRs, byte for byte: the MCP
paramsdescription.Studio: LimeChain/surfpool-web-ui#37