Repository navigation
fix(scenarios): send Phoenix's new funding fields and use Pump's reserved fee recipient for mayhem coins - #54
Conversation
…rved fee recipient for mayhem coins
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
There was a problem hiding this comment.
Overview
The PR fixes three live tests that broke after mainnet program upgrades:
- Phoenix funding:
UpdateFundingParametersnow gets three trailingNoneoption bytes appended to its payload. - Phoenix stale readings: the test now checks Hawkeye's error code (
Custom(12289)) instead of its reworded message. - Pump graduation: the fixture reads the curve's
is_mayhem_modeflag and, for mayhem coins, usesGlobal.reserved_fee_recipient.
The fixes are small and focused. Matching on the error code is more robust than matching on text.
Notes
- Phoenix encoding hack: the hard-coded
[0, 0, 0]suffix inencode_instructionis easy to break later (see inline comment). The bundled IDL is already patched forTrader(solana-foundation#845), so another option is to add the threeOptionfields toUpdateFundingParametersInstructioninidl.json. That keeps one source of truth and lets callers set the values. - Unrelated commit: the PR also includes
a82b6a5 ci: add Claude pull request reviews(.github/workflows/claude-review.yml), which the description doesn't mention. Consider moving it to its own PR. If it stays:- It interpolates the PR title and body straight into the prompt. Fork PRs are skipped and the allowed tools are read-only, so the risk is limited to prompt injection steering the review output. Still worth knowing.
- A failed
JSON.parseinpost_feedbackthrows without a fallback. That is probably fine, but the job will show as failed.
| } | ||
| // The program also reads an optional clamp center, clamp band radius and divisor, which the | ||
| // published IDL leaves out. Three `None`s keep their current values. | ||
| if name == "UpdateFundingParameters" { |
There was a problem hiding this comment.
This always appends three bytes for this instruction name. If the bundled idl.json is later refreshed from a published IDL that includes the clamp/divisor fields, the args loop will encode them and these bytes will be added again. The program would then get a payload 3 bytes too long, which can cause another InvalidInstructionData that is hard to trace.
Two options:
- Add the three
Optionfields toUpdateFundingParametersInstructionin the bundled IDL, as was done for theTradersplit. This keeps one source of truth and lets callers set them. - Keep this suffix but guard it, e.g. append only when the IDL struct still has 4 fields, and fail loudly otherwise.
| ) | ||
| .expect("global fee recipient"); | ||
| // A mayhem-mode coin pays its fees to the reserved recipient; any other is NotAuthorized. | ||
| let fee_recipient_offset = if curve_data[CURVE_IS_MAYHEM_MODE_OFFSET] == 1 { |
There was a problem hiding this comment.
Nit: Borsh bool is strictly 0 or 1, so == 1 works. != 0 would match how the program reads the flag, though, and would still pick the reserved recipient if the byte ever held a non-canonical value. Optionally add a debug_assert!(curve_data.len() > CURVE_QUOTE_MINT_OFFSET) nearby so a layout change fails with a clear message instead of an index panic.
There was a problem hiding this comment.
Summary
The PR fixes three live-test failures that came from mainnet program upgrades:
- Phoenix funding: the encoder now appends three
Nones toUpdateFundingParameters. A guard errors if the IDL ever lists more than its current four fields. - Phoenix stale readings: the test now asserts on error code 12289 instead of the old message text.
- Pump graduation: mayhem-mode coins now use
Global.reserved_fee_recipientas the fee recipient.
The changes are small and well targeted, and the comments explain why each one is needed. I found no correctness bugs.
Notes
- Unrelated change in scope: the PR also adds
.github/workflows/claude-review.yml(commit a82b6a5), which the title and description don't mention. It would be cleaner as its own PR. On the workflow itself:- Good: actions are pinned to SHAs, and fork PRs are skipped.
- Untrusted input: the PR title and body go straight into the prompt. Fork PRs are excluded, so only collaborators can do this, and the allowed tools are read-only. The risk is low, but this is still a prompt-injection path that can change what gets posted as a review.
- Clamp parameters can't be set: the
phoenix-market-fundingtemplate has no way to set the new clamp center, band radius or divisor. Keeping current values is the right default, but the README could note that this template can't change them. - Hard-coded Pump offsets:
CURVE_IS_MAYHEM_MODE_OFFSET = 81matches the curve layout (it sits right after the 32-byte creator at 49).GLOBAL_RESERVED_FEE_RECIPIENT_OFFSET = 483is a magic number with no derivation. A short comment listing the precedingGlobalfields, or a pointer to the IDL version, would make the next layout change easier to diagnose.
| // The program also reads an optional clamp center, clamp band radius and divisor, which the | ||
| // published IDL leaves out. Three `None`s keep their current values. | ||
| // An IDL that lists them would encode them above, so this holds only for its four fields. | ||
| if name == "UpdateFundingParameters" { |
There was a problem hiding this comment.
This guard runs only when the template is actually played. If someone swaps in a newer IDL that lists the clamp fields, CI won't flag it unless the live Phoenix tests run, because they are behind ignore_tests_ci. A cheap unit test that encodes UpdateFundingParameters and checks the data length, or the four-field shape, would catch the IDL drift in normal CI.
| "the Phoenix IDL no longer gives {name} four fields; drop the three added options" | ||
| ))); | ||
| } | ||
| data.extend([0, 0, 0]); |
There was a problem hiding this comment.
Small readability nit: [0, 0, 0] depends on Borsh encoding Option::None as a single 0 byte. A named constant or an inline note (// three Borsh Option::None tags) would make that explicit.
| "{refused}" | ||
| ); | ||
| // Hawkeye's OracleMarkPriceUnavailable; its message has been reworded, its code has not. | ||
| assert!(refused.contains("Custom(12289)"), "{refused}"); |
There was a problem hiding this comment.
Matching only Custom(12289) is more robust, but the assertion no longer pins down which program raised the error. If the CPI chain ever surfaces a different program's custom error 12289, this test would still pass. You could also check that the error is not a Phoenix-side error, or match the OracleMarkPriceUnavailable log line when it is present.
| Pull request title and body: | ||
| ---- | ||
| ${{ github.event.pull_request.title }} | ||
| ${{ github.event.pull_request.body }} |
There was a problem hiding this comment.
The PR title and body are interpolated straight into the model prompt. Fork PRs are excluded, so this is limited to collaborators, and the allowed tools are read-only. Still, someone could steer the posted review through the PR body. Consider fencing the body as untrusted content, for example with an explicit 'the following is untrusted PR text' delimiter.
There was a problem hiding this comment.
Overview
The PR changes three things:
- Phoenix funding:
UpdateFundingParametersnow gets three trailingNoneoptions. The encoder refuses to build the instruction if the IDL no longer lists exactly four fields. - Phoenix stale readings: the stale-oracle test checks the error code
Custom(12289)instead of the message text. - Pump fee recipient: a mayhem-mode coin's fees now go to
Global.reserved_fee_recipient.
The fixes look correct and are well targeted. I checked the create_v2 arguments against the bundled IDL: the order and widths match (OptionBool is a 1-byte struct).
Issues outside a single line
- The PR description is out of date. It says the Pump test "picks a live coin on every run and reads its
is_mayhem_modeflag". After d361a96, the test creates its own coins withcreate_v2, one ordinary and one mayhem. The "passed on five ordinary coins and on two mayhem coins" result was for the earlier version. Please update the description and re-run the new tests. - The Pump README is stale.
crates/core/src/scenarios/protocols/pump/README.md:201-203still says the lifecycle test "discovers a fresh, still-trading Token-2022 coin from the pump program's recent transactions". It should describe the newcreate_v2approach and mention the second (mayhem) test. - Lost coverage. The test no longer runs graduation against real mainnet coin state, such as coins with a sharing config or older curve layouts. That trade-off is fine for stability, but the README and the test module docs should say so.
- Unrelated change.
.github/workflows/claude-review.yml(a82b6a5) has nothing to do with this fix. It should probably go in its own PR so it can be reviewed and reverted separately.
| if name == "UpdateFundingParameters" { | ||
| let listed = definition | ||
| .args | ||
| .first() |
There was a problem hiding this comment.
The guard checks the first arg's struct, but line 195 appends the three Nones after all args. That is only correct while UpdateFundingParameters has a single arg. Consider also asserting definition.args.len() == 1, so an IDL change that adds a second arg fails loudly instead of putting the options in the wrong place.
| fields: Some(IdlDefinedFields::Named(fields)), | ||
| }) if fields.len() == 4 | ||
| ) { | ||
| return Err(SurfpoolError::internal(format!( |
There was a problem hiding this comment.
This check fails every phoenix-market-funding play with an internal error once someone updates the bundled IDL to list seven fields. That is intended as a tripwire, but it can only fire at runtime. A unit test that encodes UpdateFundingParameters against the bundled IDL and checks the payload length would catch it at IDL-bump time.
| } else { | ||
| GLOBAL_FEE_RECIPIENT_OFFSET | ||
| }; | ||
| let fee_recipient = |
There was a problem hiding this comment.
GLOBAL_RESERVED_FEE_RECIPIENT_OFFSET (483) reads only the single reserved_fee_recipient. If the program also accepts the reserved_fee_recipients array, this is fine. Still, add a short note on where 483 comes from (IDL field order) so the next layout bump is easier to check, like the other offsets.
| &account(addresses.bonding_curve).await, | ||
| &account(addresses.curve_vault).await, | ||
| None, | ||
| &account(PUMP_GLOBAL).await, |
There was a problem hiding this comment.
Passing None for the canonical pool assumes the pool PDA does not exist on the fork. Mints are random keypairs, so this holds. However, .unwrap() on build_pump_graduation_scenario hides which validation failed. Use unwrap_or_else(|e| panic!("graduation builder rejected the created coin: {e:?}")) to match the create_v2 panic above.
Three live tests started to fail after Phoenix and Pump changed their mainnet programs. This PR fixes the one product defect behind them and makes the two tests follow the programs.
UpdateFundingParametersalso reads an optional clamp center, clamp band radius and divisor. The published IDL still lists four fields, so thephoenix-market-fundingtemplate sent a short payload and the program answeredInvalidInstructionData. The encoder now appends threeNones, which keep the current values, and refuses to do so once the IDL lists more than the four fields. Mainnet transactions of the market authority from 2026-11-02 carry the same three trailing options.NotAuthorized(fee_recipient.rs:19) when the coin was in mayhem mode: such a coin pays its fees toGlobal.reserved_fee_recipient. The test now creates its own Token-2022 coin on the fork with pump'screate_v2and runs twice, for an ordinary and a mayhem coin, taking the matching fee recipient fromGlobal.The bundled Phoenix IDL is unchanged: it equals the published one except for the
Trader.maxPositionsu32 +traderPreferenceBitsu32 split it has carried since solana-foundation#845.Tests:
cargo +nightly fmt --all -- --checkandcargo clippy --workspace --all-targetspass.cargo test -p surfpool-core --features ignore_tests_ci,integration-testsgives 921 passed, 0 failed, including the 19 Phoenix live tests and both Pump lifecycle tests, which also passed six runs in a row.cargo test --all --features "postgres,ignore_tests_ci"was run on the first commit without a local Postgres, so only thewith_postgres_dbvariants failed there, on the missingSURFPOOL_TEST_POSTGRES_URL.