Skip to content

feat(types,core): typed errors for scenario follies - #765

Open
cds-amal wants to merge 9 commits into
solana-foundation:mainfrom
cds-rs:spike/stefan-b-technique
Open

feat(types,core): typed errors for scenario follies#765
cds-amal wants to merge 9 commits into
solana-foundation:mainfrom
cds-rs:spike/stefan-b-technique

Conversation

@cds-amal

@cds-amal cds-amal commented Aug 18, 2026

Copy link
Copy Markdown
Contributor

This PR was inspired by Stefan Baumgartner Rust talk. It was a fun exercise. Sharing it here in case anyone wants to get it over the line.

Disclaimer: most tests were written by AI.

Error fidelity: before and after

Registering a scenario whose PDA override references a property the values map
does not contain:

Before

{ "jsonrpc": "2.0", "id": 1, "result": { "context": { "slot": 1234 }, "value": null } }

followed, one slot later, by the un-actionable warn above. The override never
applies.

After

{
  "jsonrpc": "2.0", "id": 1,
  "error": {
    "code": -32600,
    "message": "Cannot register scenario 'liquidation drill': override '2f6e0c0d': PDA for program CAMMCzo5YL8w4VFF8KVHrK22GGUsp5VTaW7grrKgrWqK, seed 1: property 'poolId' not found in values"
  }
}

Nothing is scheduled; the caller learns the failing override, the seed position, and the cause in one message. Multiple bad overrides are all reported, joined with "; ", because validation collects every failure before rejecting (one round trip to fix everything, instead of one round trip per mistake).

A sampler of the causes that were previously indistinguishable:

Failure Message
Garbage pubkey address invalid account address: 'not-a-pubkey' is not a valid pubkey
Config index too large PDA for program CAMM..., seed 2: property 'configIndex' value 70000 does not fit in u16
Bad Pyth feed id PDA for program pyth..., seed 1: 'ef0d8b' is not a 32-byte hex string
Wrong JSON type PDA for program CAMM..., seed 1: property 'poolId' is bool, expected string or u64
Nested derivation, two levels deep PDA for program whirl..., seed 0: seed 1 of derived PDA for program whirl...: 'oops' is not a valid pubkey

`PdaSeed::to_bytes` and `AccountAddress::resolve` returned `Option`,
collapsing seven distinct failures into `None`: invalid public keys,
missing value maps, unknown properties, incorrect JSON types,
out-of-range `u16` values, malformed hex, and nested derivation
failures.

Changes:

* Introduce `SeedError` and `ScenarioError` using `thiserror`, and
  convert `PdaSeed::to_bytes`, `AccountAddress::resolve`, and
  `resolve_simple` to return `Result`.
* Box the source of `SeedError::DerivedSeed` so nested `DerivedPda`
  failures preserve context such as the seed index and program at every
  level.
* Replace the duplicated `filter_map`/length-check pattern in `resolve`
  and `DerivedPda` with `enumerate` and
  `collect::<Result<Vec<_>, _>>()`.
* Move the value-map lookup shared by `PropertyRef`, `U16BeRef`, and
  `Bytes32Ref` into a `property_value` helper.

Successful resolution is unchanged; only failures become more
diagnostic.
`register_scenario` previously accepted every scenario and returned
`Ok`, even if an enabled override referenced an address that could not
be resolved. Those failures surfaced only during materialization as a
server-side warning with no underlying cause.

Changes:

- Resolve every enabled override's address during registration and
  reject the scenario if any resolution fails.
- Report each failing override by id together with its specific
  resolution error.
- Skip disabled overrides during validation, matching
  materialization's behavior.
- Preserve materialization's skip-and-continue policy, but include the
  underlying resolution error in its warning.
- Add `From<ScenarioError>` for `SurfpoolError` and an
  `invalid_scenario` constructor for aggregated registration failures.

Successful scenarios behave like before; invalid overrides are now
rejected before any work is scheduled, with actionable diagnostics.
…rrides

`surfnet_registerScenario` now rejects scenarios whose enabled
overrides have unresolvable addresses. Update the documentation to
describe the validation and the resulting error.

Changes:

- Document registration-time validation in the RPC rustdoc.
- Update the scenarios README to describe validation and the shape of
  registration failures.
- Document the same behavior in the MCP `create_scenario` tool
  description.
- Add registration validation to the MCP tool's strict-rules list so
  LLM callers know the error is resolved by correcting the scenario and
  retrying.

The docs now matches the registration behavior introduced by the
validation changes.
Template addresses were converted from YAML with an infallible `From`,
allowing malformed public keys and PDA definitions to survive until
address resolution. Those failures were then reported without a clear
connection to the template that introduced them.

Changes:

- Replace the infallible `From` conversion with `TryFrom<YamlAccountAddress>`.
- Validate all information available at load time: provided public keys,
  PDA program ids, and literal public-key seeds, including nested
  `DerivedPda` values via `validate_literals`.
- Leave reference seeds unvalidated until resolution, since they depend
  on instance values.
- Preserve the existing empty-string placeholder for omitted public
  keys, which SPL Token templates use to indicate per-instance
  overrides.
- Add `ScenarioError::Template` so conversion failures identify the
  template they originated from.
- Share the wrapping logic across the three
  `Yaml`-to-`OverrideTemplate` conversions with a
  `template_address` helper.
- Panic if an embedded template fails validation, matching the
  registry's existing policy for embedded assets.

Successful templates behave as before; malformed templates now fail when
loaded instead of during address resolution.
Scheduled override application expressed every failure as an inline
warning and continued processing. Five distinct causes were reported
only as log messages, making them difficult to test and impossible to
handle as typed failures.

Changes:

- Introduce `OverrideError` in `surfpool-types` to represent address
  resolution failures, missing accounts, undersized accounts, missing
  IDLs, and forge failures.
- Move override application into an `apply_override` helper returning
  `Result<(), OverrideError>`, leaving the existing skip-and-continue
  policy to its caller.
- Define `Ok(())` to cover both a successfully applied override and the
  documented no-op case where every value is a PDA seed reference.
- Check account size before looking up an IDL so undersized accounts
  consistently report `AccountTooSmall`.
- Clarify `AccountNotFound` by documenting that overrides patch
  existing accounts rather than creating them, with `fetchBeforeUse`
  as the remedy.
- Wrap core-side forge failures (IDL lookup, Borsh decode, and Borsh
  encode) at the crate boundary while preserving the existing
  discriminator mismatch behavior.

Successful overrides behave as before; skipped overrides now carry
typed, testable failure causes.
The existing tests verified typed errors structurally, but never
exercised their `Display` implementations. A format-string change could
therefore alter every user-facing error message without failing a test.

Changes:

* Add a test covering every variant of `SeedError`,
  `ScenarioError`, and `OverrideError`.
* Assert the exact rendered `Display` message for each variant.
* Print each message before asserting so `cargo test -- --nocapture`
  serves as a catalog of the error text users see.

The test now guards both the structure of the error types and the
user-facing messages they produce.
The u16 string path gained a WrongType { expected: "u16" } construction
when the decimal-string parse met the typed-error conversion during the
rebase; pin the exact error and its rendered message, and state the
range/type boundary: an overflowing decimal string reports the type,
because the parse rejects it before any range logic runs.
@cds-amal
cds-amal marked this pull request as ready for review August 18, 2026 15:42
@greptile-apps

greptile-apps Bot commented Aug 18, 2026

Copy link
Copy Markdown
Contributor

Greptile Summary

This PR replaces opaque scenario address-resolution failures with typed, contextual errors and validates enabled override addresses before scheduling.

  • Adds structured seed, address, and override error types.
  • Returns aggregated JSON-RPC errors for invalid scenario overrides.
  • Refactors scheduled override application to report typed failures.
  • Validates embedded scenario templates and updates documentation and tests.

Confidence Score: 5/5

The PR appears safe to merge, with no concrete changed-code defect identified.

The new typed-error paths consistently propagate address-resolution failures through template loading, scenario registration, and scheduled override handling, and repository call sites have been updated for the fallible APIs.

Important Files Changed

Filename Overview
crates/types/src/scenarios.rs Introduces typed seed/address/application errors, fallible PDA resolution and template conversion, plus comprehensive unit coverage.
crates/core/src/surfnet/svm.rs Validates enabled override addresses before scheduling and extracts typed override application while retaining existing materialization semantics.
crates/core/src/error.rs Maps scenario failures into aggregated invalid-request JSON-RPC errors.
crates/core/src/scenarios/registry.rs Handles fallible template conversion during built-in registry initialization and updates resolution tests.
crates/core/src/rpc/surfnet_cheatcodes.rs Documents registration-time validation behavior without changing the RPC method shape.
crates/mcp/src/surfpool/mod.rs Updates MCP guidance so clients can correct address-resolution failures and retry.
crates/core/src/scenarios/README.md Documents up-front enabled-override address validation and aggregated failure reporting.

Reviews (1): Last reviewed commit: "cargo: fmt" | Re-trigger Greptile

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant