Skip to content

Graph-based spec shaking v2#1861

Open
mootz12 wants to merge 37 commits into
mainfrom
spec-shaking-marker-fix-3
Open

Graph-based spec shaking v2#1861
mootz12 wants to merge 37 commits into
mainfrom
spec-shaking-marker-fix-3

Conversation

@mootz12
Copy link
Copy Markdown
Contributor

@mootz12 mootz12 commented May 4, 2026

What

Add graph-based spec shaking v2 for Rust contracts. The SDK macros now emit a removable sidecar graph that records exact spec-entry references by hash, while contractspecv0 keeps the public contract interface. Post-build tooling treats exported functions as roots, adds marker roots for events and thrown errors, follows the sidecar graph to keep only reachable UDT specs, rewrites contractspecv0, and removes the private graph section from the final WASM.

This changes spec shaking from a marker-per-type scheme to a root-and-graph traversal: markers identify roots that cannot be inferred from the public spec, and the graph resolves the UDTs reachable from those roots without relying on ambiguous type names.

  • Add soroban-spec-markers to own spec-shaking marker, metadata, and sidecar graph wire formats.
  • Emit v2 metadata from SDK macros: event and thrown-error root markers, exact SpecTypeId values, and removable graph records keyed by spec-entry hash.
  • Filter contractspecv0 entries through function roots, marker roots, and exact graph references, then strip the private graph section during post-build rewriting.
  • Validate reachable graph coverage during post-build filtering and error when a reachable referenced spec is missing or not exported.

Why

Spec shaking needs to remove unused contract spec entries without relying on ambiguous UDT names or keeping marker bytes in the final public interface. The v2 graph gives post-build tooling exact spec-entry identities, lets functions act as roots from their public specs, and reserves markers for roots that are not visible in contractspecv0, such as published events and errors thrown through panic_with_error! or assert_with_error!.

Architecture

flowchart LR
    Macros[SDK macros] --> Markers[SpEcV1 root markers]
    Macros --> Graph[SpGrV1 sidecar graph]
    Macros --> Spec[contractspecv0 entries]
    Markers --> Filter[soroban-spec shake_contract_spec]
    Graph --> Filter
    Spec --> Filter
    Filter --> Shaken[rewritten contractspecv0]
    Filter --> Strip[drop private sidecar]
Loading

Known limitations

  • Types explicitly marked export = false remain hidden from contractspecv0; if a public boundary references one, graph validation reports the missing referenced spec instead of synthesizing a public entry.
  • WASMs that are used via contractimport! but have not been shaken will include all contracttype types defined by dependencies, including the soroban-sdk with types like soroban-sdk::auth::Context.
  • This does not solve for duplicate spec-entries. If a user defines a contracttype with the same name as another, imported contracttype, like via a WASM import, both are including in the spec. If both types are referenced by a boundary, they will both exist.

TODO

  • Confirm that contractspecv0.rssdk.graphv0 graph records won't be eliminated by LTO (testing says it won't, but find some more concrete evidence)
  • Decide if we want this to warn/error on duplicate-name spec entries during shake_contract_spec
  • Should we have stricter validation for soroban-spec version? E.g. - pass soroban-spec-version via env variable during build and use that over experimental_spec_shaking_v2 env var?
  • Consider catching events that are emitted through non-typical means in specs
  • CLI PR -> Use new spec filtering logic from SDK stellar-cli#2551

mootz12 and others added 30 commits April 15, 2026 15:32
Add a `SpecShakingMarker` bound to `Env::panic_with_error` under
`experimental_spec_shaking_v2` and call `I::spec_shaking_marker()`
before erasing the user's type via `.into()`. Share the body with the
non-feature build via a private `panic_with_error_inner` that takes the
already-converted `internal::Error`.

Add tests in `tests/spec_shaking_v2/`:
- `with_panic_error` + `UsedPanicErrorEnum` — error enum used only via
`panic_with_error!`; asserts the spec entry is retained.
- `with_assert_error` + `UsedAssertErrorEnum` — same, via
`assert_with_error!`.
- `with_panic_raw_error` — passes a raw `soroban_sdk::Error` to
`panic_with_error!`; regression check that the new bound does not break
callers using the un-typed error.
- `UnusedPubError` — a `#[contracterror]` enum referenced nowhere;
asserts it is correctly shaken out of filtered entries.

Closes #1817.

Spec shaking v2 keeps a type's `contractspecv0` entry only if its
`SpecShakingMarker::spec_shaking_marker()` survives DCE, and the only
paths that call the marker are the v2 blanket impls of
`IntoValForContractFn` (return values) and `TryFromValForContractFn`
(parameters). A contract that defined a `#[contracterror]` enum and used
it only via `panic_with_error!` or `assert_with_error!` — without ever
appearing in a `Result<_, E>` return type — had no live caller for the
marker, so the marker function was eliminated and the spec entry
stripped from the WASM, even though the enum is part of the contract's
observable error interface.
### What:
Add a GitHub Actions workflow that runs `anthropics/claude-code-action`
on non-draft pull requests to review code quality, bugs, security, and
performance, posting inline comments. Concurrency cancels in-progress
runs per PR.

### Why:
Experimenting with and comparing the outcomes and experience with using
the built-in GitHub AI reviews.
Copilot AI review requested due to automatic review settings May 4, 2026 18:52
Copy link
Copy Markdown
Contributor

Copilot AI left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull request overview

Adds graph-based spec shaking v2 to the Rust SDK stack so post-build tooling can keep only spec entries reachable from contract APIs, published events, and typed thrown errors, using exact hashed graph edges instead of name-based markers.

Changes:

  • Introduces the v2 spec-shaking pipeline: exact SpecTypeIds, removable sidecar graph records, marker scanning, and WASM rewriting/graph stripping.
  • Updates SDK/macros/auth/address behavior and tests for constructors, custom-account auth, recursive UDTs, panic/assert errors, and duplicate-name cases.
  • Refreshes generated fixtures/snapshots across the test suite, and also adds a new Claude review workflow.

Reviewed changes

Copilot reviewed 116 out of 125 changed files in this pull request and generated 2 comments.

Show a summary per file
File Description
tests/udt/test_snapshots/test/test_recursive.1.json Snapshot for recursive UDT wasm test.
tests/udt/test_snapshots/test/test_recursive_enum.1.json Snapshot for recursive enum wasm test.
tests/udt/src/lib.rs Adds recursive UDT contract cases and wasm-import tests.
tests/spec_shaking_v2/src/test.rs Expands v2 reachability/filter/strip assertions.
tests/spec_shaking_v2/src/lib.rs Adds broad v2 fixture coverage for roots and graph traversal.
tests/spec_shaking_v1/src/test.rs Updates v1 expectations for constructor/auth and duplicate names.
tests/associated_type_contracttrait/test_snapshots/test_with_wasm/test_exec.1.json Snapshot refresh for associated-type contracttrait wasm.
tests-expanded/test_workspace_lib_wasm32v1-none.rs Wasm-expanded fixture showing v2 type graph emission.
tests-expanded/test_workspace_lib_tests.rs Host-expanded fixture mirroring v2 type graph emission.
tests-expanded/test_workspace_contract_wasm32v1-none.rs Wasm-expanded fixture showing v2 function graph emission.
tests-expanded/test_workspace_contract_tests.rs Host-expanded fixture for workspace contract graph code.
tests-expanded/test_tuples_wasm32v1-none.rs Wasm-expanded tuple fixture with function graph records.
tests-expanded/test_mutability_wasm32v1-none.rs Wasm-expanded mutability fixture with function graph records.
tests-expanded/test_mutability_tests.rs Host-expanded mutability fixture for generated graph code.
tests-expanded/test_multiimpl_wasm32v1-none.rs Wasm-expanded multi-impl fixture with graph records.
tests-expanded/test_multiimpl_tests.rs Host-expanded multi-impl fixture for generated graph code.
tests-expanded/test_modular_wasm32v1-none.rs Wasm-expanded modular fixture with graph records.
tests-expanded/test_modular_tests.rs Host-expanded modular fixture for generated graph code.
tests-expanded/test_macros_wasm32v1-none.rs Wasm-expanded macro fixture with graph records.
tests-expanded/test_macros_tests.rs Host-expanded macro fixture for generated graph code.
tests-expanded/test_logging_wasm32v1-none.rs Wasm-expanded logging fixture with graph records.
tests-expanded/test_logging_tests.rs Host-expanded logging fixture for generated graph code.
tests-expanded/test_invoke_contract_wasm32v1-none.rs Wasm-expanded cross-contract invoke fixture with graph records.
tests-expanded/test_invoke_contract_tests.rs Host-expanded cross-contract invoke fixture.
tests-expanded/test_generics_wasm32v1-none.rs Wasm-expanded generics fixture with graph records.
tests-expanded/test_generics_tests.rs Host-expanded generics fixture for generated graph code.
tests-expanded/test_fuzz_wasm32v1-none.rs Wasm-expanded fuzz fixture with graph records.
tests-expanded/test_fuzz_tests.rs Host-expanded fuzz fixture for generated graph code.
tests-expanded/test_events_wasm32v1-none.rs Wasm-expanded event fixture with event roots and graph records.
tests-expanded/test_events_tests.rs Host-expanded event fixture reflecting v2 event generation.
tests-expanded/test_events_ref_wasm32v1-none.rs Wasm-expanded ref-event fixture with event roots and graph records.
tests-expanded/test_events_ref_tests.rs Host-expanded ref-event fixture reflecting v2 generation.
tests-expanded/test_errors_wasm32v1-none.rs Wasm-expanded error fixture with type IDs/graph records.
tests-expanded/test_errors_tests.rs Host-expanded error fixture reflecting v2 generation.
tests-expanded/test_empty_wasm32v1-none.rs Wasm-expanded empty-contract fixture with graph records.
tests-expanded/test_empty_tests.rs Host-expanded empty-contract fixture.
tests-expanded/test_contracttrait_trait_wasm32v1-none.rs Wasm-expanded contracttrait fixture with UDT graph metadata.
tests-expanded/test_contracttrait_path_super_wasm32v1-none.rs Wasm-expanded path-super fixture with graph records.
tests-expanded/test_contracttrait_path_super_tests.rs Host-expanded path-super fixture.
tests-expanded/test_contracttrait_path_self_wasm32v1-none.rs Wasm-expanded path-self fixture with graph records.
tests-expanded/test_contracttrait_path_self_tests.rs Host-expanded path-self fixture.
tests-expanded/test_contracttrait_path_relative_wasm32v1-none.rs Wasm-expanded path-relative fixture with graph records.
tests-expanded/test_contracttrait_path_relative_tests.rs Host-expanded path-relative fixture.
tests-expanded/test_contracttrait_path_crate_wasm32v1-none.rs Wasm-expanded path-crate fixture with graph records.
tests-expanded/test_contracttrait_path_crate_tests.rs Host-expanded path-crate fixture.
tests-expanded/test_contract_data_wasm32v1-none.rs Wasm-expanded contract-data fixture with graph records.
tests-expanded/test_contract_data_tests.rs Host-expanded contract-data fixture.
tests-expanded/test_constructor_wasm32v1-none.rs Wasm-expanded constructor fixture with graph records.
tests-expanded/test_constructor_tests.rs Host-expanded constructor fixture.
tests-expanded/test_bn254_wasm32v1-none.rs Wasm-expanded bn254 fixture with type/function graph metadata.
tests-expanded/test_bn254_tests.rs Host-expanded bn254 fixture.
tests-expanded/test_bls_wasm32v1-none.rs Wasm-expanded bls fixture with type/function graph metadata.
tests-expanded/test_bls_tests.rs Host-expanded bls fixture.
tests-expanded/test_auth_wasm32v1-none.rs Wasm-expanded auth fixture with function/type graph metadata.
tests-expanded/test_auth_tests.rs Host-expanded auth fixture.
tests-expanded/test_associated_types_wasm32v1-none.rs Wasm-expanded associated-types fixture with graph records.
tests-expanded/test_associated_types_tests.rs Host-expanded associated-types fixture.
tests-expanded/test_associated_types_contracttrait_wasm32v1-none.rs Wasm-expanded associated-types contracttrait fixture.
tests-expanded/test_associated_type_chained_wasm32v1-none.rs Wasm-expanded chained-associated-type fixture with graph records.
tests-expanded/test_associated_type_chained_tests.rs Host-expanded chained-associated-type fixture.
tests-expanded/test_add_u64_wasm32v1-none.rs Wasm-expanded add-u64 fixture with graph records.
tests-expanded/test_add_u64_tests.rs Host-expanded add-u64 fixture.
tests-expanded/test_add_u128_wasm32v1-none.rs Wasm-expanded add-u128 fixture with graph records.
tests-expanded/test_add_u128_tests.rs Host-expanded add-u128 fixture.
tests-expanded/test_add_i128_wasm32v1-none.rs Wasm-expanded add-i128 fixture with graph records.
tests-expanded/test_add_i128_tests.rs Host-expanded add-i128 fixture.
tests-expanded/test_account_wasm32v1-none.rs Wasm-expanded account fixture with auth-context graph metadata.
tests-expanded/test_account_tests.rs Host-expanded account fixture.
soroban-spec/src/lib.rs Exposes new strip module.
soroban-spec/src/shaking.rs Implements v2 marker scanning and graph-based reachability.
soroban-spec/src/strip.rs Adds WASM spec rewrite/graph stripping helpers.
soroban-spec/Cargo.toml Adds marker crate dependency for v2 support.
soroban-spec-rust/src/lib.rs Refreshes generated Rust bindings for new recursive/auth types.
soroban-spec-markers/Cargo.toml Adds new crate manifest for marker/graph wire formats.
soroban-spec-markers/src/lib.rs Defines marker/spec-id/graph record wire-format helpers.
soroban-sdk/src/try_from_val_for_contract_fn.rs Removes v2 boundary marker rooting from arg conversion path.
soroban-sdk/src/lib.rs Re-exports hidden v2 support module and updates docs.
soroban-sdk/src/into_val_for_contract_fn.rs Removes v2 boundary marker rooting from return conversion path.
soroban-sdk/src/env.rs Routes panic-with-error through typed marker hook under v2.
soroban-sdk/src/auth.rs Makes auth boundary SDK types exportable under v2.
soroban-sdk/src/address.rs Makes address Executable exportable under v2.
soroban-sdk/src/spec_shaking.rs Adds hidden runtime traits/helpers for v2 generation.
soroban-sdk/src/_features.rs Rewrites v2 feature docs around graph-based shaking.
soroban-sdk/Cargo.toml Adds marker crate dependency.
soroban-sdk-macros/src/lib.rs Threads crate_path into contract spec generation.
soroban-sdk-macros/src/derive_trait.rs Passes crate_path into generated contractspecfn.
soroban-sdk-macros/src/derive_struct.rs Emits v2 type IDs/graph records for struct UDTs.
soroban-sdk-macros/src/derive_struct_tuple.rs Emits v2 type IDs/graph records for tuple structs.
soroban-sdk-macros/src/derive_spec_fn.rs Emits function graph records from exact referenced UDTs.
soroban-sdk-macros/src/derive_event.rs Emits event graph records and inline event root markers.
soroban-sdk-macros/src/derive_error_enum_int.rs Emits v2 type IDs/graph records for contract errors.
soroban-sdk-macros/src/derive_enum.rs Emits v2 type IDs/graph records for union enums.
soroban-sdk-macros/src/derive_enum_int.rs Emits v2 type IDs/graph records for int enums.
soroban-sdk-macros/src/derive_contractimpl_trait_default_fns_not_overridden.rs Updates trait-default spec generation to pass crate path.
soroban-sdk-macros/src/shaking.rs Adds macro-side helpers for markers, type IDs, and graph records.
soroban-sdk-macros/Cargo.toml Adds marker crate dependency with alloc/hash features.
Makefile Exports v2 build-support env var in test command.
Cargo.toml Registers new soroban-spec-markers workspace member.
Cargo.lock Locks new marker crate dependency edges.
.github/workflows/claude-review.yml Adds a new Claude-based PR review workflow.

Comment thread soroban-sdk-macros/src/derive_event.rs
Comment thread .github/workflows/claude-review.yml
@claude
Copy link
Copy Markdown

claude Bot commented May 4, 2026

Claude finished @mootz12's task in 4m 6s —— View job


PR Review: Graph-based spec shaking v2

Tasks:

  • Read CLAUDE.md for repo instructions
  • Review core marker/wire format (soroban-spec-markers/src/lib.rs)
  • Review shaking logic (soroban-spec/src/shaking.rs)
  • Review WASM strip/rewrite (soroban-spec/src/strip.rs)
  • Review macro-side helpers (soroban-sdk-macros/src/shaking.rs)
  • Review derive macros (event, spec_fn, struct, enum)
  • Review SDK changes (auth, address, env, spec_shaking)
  • Compile findings and post review

This is a well-designed system with thorough test coverage. The architecture cleanly separates wire formats (soroban-spec-markers), post-build analysis (soroban-spec), and compile-time emission (soroban-sdk-macros). The const-eval graph record approach is elegant. Below are findings by severity.


High Priority

1. Event marker gap via Events::publish_event

soroban-sdk-macros/src/derive_event.rs:338-342 — The SpEcV1 marker is emitted only in the generated inherent publish() method. Two call paths bypass it:

  • env.events().publish_event(&event) — goes directly to Events::publish_event in events.rs:105, which calls contract_event without touching the inherent method
  • <MyEvent as Event>::publish(&event, env) — trait dispatch bypasses Rust's inherent-method preference

If either path is used in reachable code, spec shaking will strip the event even though it's published. The events.rs:68 default trait publish also calls publish_event directly.

For the publish_event path specifically, there's no generated inherent override that can intercept it. One approach: emit the marker in the Event::topics/Event::data implementations (which are called by publish_event), since those methods are always exercised when an event is published regardless of call path.

Fix this →

2. LTO may eliminate graph records — no runtime anchor

soroban-sdk-macros/src/shaking.rs:133-142 — Graph records use:

#[cfg_attr(target_family = "wasm", link_section = "contractspecv0.rssdk.graphv0")]
pub static __SPEC_GRAPH_FN_...: [u8; N] = encode_graph_record(...);

Unlike markers (which use read_volatile to force a runtime reference), graph records have no runtime anchor. The pub static + link_section combination should survive LTO for wasm targets, but it depends on whether the linker treats link_section statics as GC roots when no other code references them. This is the open TODO in the PR description. Until this is confirmed (ideally with a test that verifies the section survives an LTO build), there's a risk that LTO silently drops the entire sidecar graph, causing post-build shaking to fail with MissingGraphEntry errors.

3. Auth types newly visible in pre-shaken WASM

soroban-sdk/src/auth.rs:15-21 — Under experimental_spec_shaking_v2, all auth types (Context, ContractContext, CreateContractHostFnContext, ContractExecutable, InvokerContractAuthEntry, etc.) now emit entries to contractspecv0 unconditionally. Before v2 these were export = false.

For any contract that uses require_auth but does not implement __check_auth, and for which the post-build shake step has not yet run, these types now appear in the contract spec. Tools that consume the spec (client codegen, explorers) would see auth framework internals that weren't there before. The known-limitations section of the PR should explicitly note this as a breaking change for the pre-shaken spec format.


Medium Priority

4. SpecGraph::insert ref deduplication is O(n) per insert

soroban-spec/src/shaking.rs:83-87:

for ref_id in entry.refs {
    if !existing.refs.contains(&ref_id) {  // O(n) linear scan
        existing.refs.push(ref_id);
    }
}

A HashSet<SpecId> for dedup tracking would be O(1) amortized. Not a critical performance path, but for contracts with many UDT refs the current approach is quadratic in the number of merged refs.

5. add_graph_refs error attribution is order-dependent

soroban-spec/src/shaking.rs:459-476 — When iterating graph refs, missing_ref_id and non_udt_ref are set with get_or_insert on the first occurrence. Then for each uncovered expected name, the code checks missing_ref_id first, then non_udt_ref. If both are set, the error always blames MissingReferencedSpecEntry even when the actual problem is a non-UDT ref. The attribution depends on iteration order through record.refs, which is deterministic per run but could still mislead the developer.

6. Cargo.toml missing trailing newline

soroban-spec-markers/Cargo.toml is missing a trailing newline at the end of file.

7. Silent drop of duplicate contractspecv0 sections

soroban-spec/src/strip.rs:144-149:

CONTRACT_SPEC_SECTION => {
    if !wrote_spec {
        write_custom_section(&mut out, CONTRACT_SPEC_SECTION, spec_xdr)?;
        wrote_spec = true;
    }
    // else: silently dropped
}

If a WASM has multiple contractspecv0 sections (unusual but valid for custom sections), only the first is written and the rest are silently dropped. A warning or at minimum a ContractSpecDuplicate error variant would make this behaviour explicit.


Low Priority / Observations

8. Fragile sync between type_id_refs and add_type_def_udt_names

soroban-sdk-macros/src/shaking.rs:147-186 and soroban-spec/src/shaking.rs:588-610 must traverse Rust types and ScSpecTypeDef identically. The comment says "Keep this traversal in sync" and the test type_id_refs_match_strict_spec_graph_validation covers the known cases. However, any new container type added to ScSpecTypeDef in the future needs to be added to both. Consider a doc-test that explicitly lists which container kinds are traversed, so future reviewers know exactly what the contract is.

9. DataFormat should use From not manual Into

soroban-sdk-macros/src/derive_event.rs:49-57impl Into<ScSpecEventDataFormat> for DataFormat implements the wrong direction. Idiomatic Rust is impl From<DataFormat> for ScSpecEventDataFormat, which gives you Into for free. Minor style issue.

10. Const fn comment in encode_graph_record is doc-level but function is pub

soroban-spec-markers/src/lib.rs:162-169 — The extended doc comment on encode_graph_record explains why const fn is needed (macro expansion before all UDTs are in scope). This is valuable, but being a non-#[doc = ...] doc comment on a pub function it will show in rustdoc. This is actually good — keep it — just noting it's intended to be user-visible documentation.

11. spec_shaking_call naming in derive_event.rs

soroban-sdk-macros/src/derive_event.rs:204,209 — The variables spec_shaking_call and spec_shaking_graph are fine names, but the former specifically generates a marker block (not a "call" in the traditional sense). spec_shaking_marker would be more consistent with the terminology used elsewhere (e.g. generate_marker_block).


Summary

The core architecture is sound. The most important items before merge are:

  1. The event marker gap via publish_event (high — a real silent-shaking bug)
  2. Confirming LTO doesn't eliminate graph records (high — a correctness prerequisite)
  3. Documenting the auth types visibility change in known limitations (medium — a user-visible breaking change in the pre-shaken spec format)

Everything else is polish. The test coverage across soroban-spec-markers, soroban-spec, and soroban-sdk-macros is excellent, and the cross-crate sync test (type_id_refs_match_strict_spec_graph_validation) is a good safeguard for the trickiest invariant.

@mootz12
Copy link
Copy Markdown
Contributor Author

mootz12 commented May 4, 2026

Responses to #1861 (comment)

High Priority

1. Event marker gap via Events::publish_event

Events can be emitted as raw data, without specs, through these mediums. Added a TODO for this, but it's not relevant to the new design. Will consider patching here or at least in a followup if we opt to take this solutions approach.

2. LTO may eliminate graph records — no runtime anchor

Tracked in a TODO

3. Auth types newly visible in pre-shaken WASM

This was an intentional choice. WASMs currently don't have the ability to differentiate a contracttype defined by the soroban-sdk, one defined in code, or one defined by another imported WASM. Thus, this PR makes the choice to treat them all the same and allow spec shaking to keep what is needed.

Medium Priority

4. SpecGraph::insert ref deduplication is O(n) per insert

We can consider this if it's an issue, but is not necessary per my testing.

5. add_graph_refs error attribution is order-dependent

Fixed, prefer ReferencedSpecEntryNotUdt errors as they are more specific

6. Cargo.toml missing trailing newline

Fixed

7. Silent drop of duplicate contractspecv0 sections

Won't fix - 2 of these should never exist and this PR opts to ignore intentionally broken WASMs.

Low Priority / Observations

8. Fragile sync between type_id_refs and add_type_def_udt_names

Improved the existing validation test to reduce drift risk.

9. DataFormat should use From not manual Into

Fixed

10. Const fn comment in encode_graph_record is doc-level but function is pub

Nothing to fix

11. spec_shaking_call naming in derive_event.rs

Fixed

@claude
Copy link
Copy Markdown

claude Bot commented May 4, 2026

Claude finished @mootz12's task in 2m 25s —— View job


PR Review: Graph-based spec shaking v2 — Follow-up

Tasks:


All four claimed fixes are confirmed correct, plus two unrequested improvements. Here's the full accounting.


Confirmed Fixes

#5 — Error attribution order (soroban-spec/src/shaking.rs:482-498)

The non_udt_ref branch is now checked before missing_ref_id. This is correct: a ReferencedSpecEntryNotUdt error is more specific (we know what the entry is) than MissingReferencedSpecEntry (we only know the ID). Good fix.

#6 — Cargo.toml trailing newline (soroban-spec-markers/Cargo.toml)

Confirmed: file now ends with a newline.

#9DataFormat impl direction (soroban-sdk-macros/src/derive_event.rs:49-56)

Correctly changed to impl From<DataFormat> for ScSpecEventDataFormat. ✓

#11spec_shaking_callspec_shaking_marker (soroban-sdk-macros/src/derive_event.rs:204,339)

Renamed consistently. ✓


Unrequested Improvements (Good Additions)

Panic → compile error in generate_graph_record (soroban-sdk-macros/src/shaking.rs:124-130)

The assert! on ref_count > u16::MAX was replaced with syn::Error::new(...).to_compile_error(). This is strictly better: it produces a proper rustc diagnostic with a span pointing at the offending type instead of panicking during macro expansion. The companion test at line 236 confirms the error message is correct.

encode_graph_record assert order (soroban-spec-markers/src/lib.rs:174-175)

The N <= u16::MAX check now precedes LEN == graph_record_len(N). This matters on 32-bit targets where N * SPEC_ID_LEN in graph_record_len could otherwise overflow usize before the bounds check fires. Correct ordering.


Test Coverage Improvement (#8)

The type_id_refs_match_strict_spec_graph_validation test now covers eight cases: udt, reference, option, result, vec, map, tuple, and nested. Each case exercises both the macro-side type_id_refs traversal and the spec-side filter validation, so drift between the two is caught immediately. Significant improvement.


Remaining Open Items

The three items from the previous review that the author deferred are still open:

  1. Event marker gap via publish_event — acknowledged as a TODO. Code using env.events().publish_event(&event) or the trait Event::publish default still bypasses the marker. Not a blocker if the use case is considered out of scope for v2, but worth tracking.

  2. LTO and graph record survival — still an open TODO in the PR description. The pub static + link_section combination should survive for wasm targets in practice, but a concrete test (e.g., verifying the section is present in an LTO-built WASM fixture) would close this definitively.

  3. Auth types in pre-shaken spec — the _features.rs docs were improved with the export = false / SpecShakingMarker note, which is helpful. The auth-types-visible-before-shaking behaviour is intentional per the author.


Minor Observation

The claude-review.yml workflow file is no longer in the PR diff (removed via the merge with origin/main), resolving the copilot concern about it being included in this feature PR.


Everything else looks clean. The four claimed fixes are correct, the two bonus improvements are sound, and the test expansion is substantial. The outstanding items are either known-acceptable tradeoffs or tracked TODOs.

@leighmcculloch
Copy link
Copy Markdown
Member

leighmcculloch commented May 5, 2026

Some light thoughts after a first read:

Very nice. 😎


  • Confirm that contractspecv0.rssdk.graphv0 graph records won't be eliminated by LTO

I would not expect it to be eliminated based on how we already use the other custom sections.


  • WASMs that are used via contractimport! but have not been shaken will include all contracttype types defined by dependencies, including the soroban-sdk with types like soroban-sdk::auth::Context

I'd like to understand more about why this is. Is this a limitation of the existing spec shaking v2, or only the spec shaking v2 in this PR?


Does this implementation avoid recursive types from stack overflow?


In this design where the graph is separate from the spec and has a narrow use case I understand the motivation to discard it. However, I think the relationships and the type id's more concretely linking type dependencies is helpful and if that information was instead stored in the spec itself, it would make the spec more exact/accurate.


I think we should produce XDR valid values for the graph / spec-with-graph. That shouldn't be hard for us to do either by hand or by introducing a const form of xdr encoding into the rs-stellar-xdr crate.

@mootz12
Copy link
Copy Markdown
Contributor Author

mootz12 commented May 5, 2026

I'd like to understand more about why this is. Is this a limitation of the existing spec shaking v2, or only the spec shaking v2 in this PR?

Since we are walking specs exactly, we now error whenever a spec is missing for a type that is defined at a public endpoint.

This is problematic for contracts that write their own __check_auth, or use types like Context in their own functions.

Given these types could change as they are not defined in XDR so don't have any backwards compatibility guarantees, it makes sense that they are included in the spec when used to avoid drift.

if that information was instead stored in the spec itself, it would make the spec more exact/accurate.

I agree. But it's not as obvious what the new XDR shape should be. Currently, the graph uses hashes of the spec entry that is structure / name based, but if we make the branches rely on hashes over names, now we can't compute hashes of recursive structures.

I think we should produce XDR valid values for the graph / spec-with-graph.

I also agree. Just defined it locally for now to see if this direction is right, and if we wanted to pursue just updating the spec-xdr directly instead.

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.

3 participants