Fix Result type generation for user-defined error enums named "Error"

[mootz12]

What

When a contract defines its own error enum named Error and uses it in a Result<T, Error> return type, the generated client code now emits Result<T, Error> (referencing the user-defined enum) instead of Result<T, soroban_sdk::Error>.

The fix lives entirely in client generation. The on-the-wire spec format is unchanged: a contract function returning Result<_, Error> still serializes the error type as ScSpecTypeDef::Error, exactly as every prior SDK release has done. During Rust client generation, soroban-spec-rust now checks whether the spec also contains a UdtErrorEnumV0 named Error, and if so rewrites every ScSpecTypeDef::Error reference to Udt { name: "Error" } so that the generated trait references the user-defined type.

If the spec contains no error enum named Error, behavior is unchanged: ScSpecTypeDef::Error continues to map to soroban_sdk::Error, which covers contracts that intentionally use the SDK error type at their boundary.

Why

Previously, contracts whose error enum was named Error (a natural choice — it's what every other Rust crate would name it) silently produced a generated client that returned Result<_, soroban_sdk::Error> instead of Result<_, Error>, hiding the contract's error variants behind the opaque SDK error type and breaking client-side error matching.

This iteration of the fix moves the resolution from spec emission to client interpretation, per review feedback (#1709 (comment)). Two benefits over the original approach of changing what #[contractimpl] emits:

1. No spec wire-format change. The bytes that go into a deployed wasm are identical to what main produces today, so no contract's spec hash drifts.
2. Already-deployed contracts benefit immediately. Any mainnet contract that defines an Error error enum will, the next time someone regenerates a Rust client against its spec with the new soroban-spec-rust, produce a correct Result<_, Error> signature — no redeploy required.

Example — before the fix:

// Contract defines:
pub enum Error { Overflow = 1 }
pub fn safe_add(a: u64, b: u64) -> Result<u64, Error>

// Generated client had:
fn try_safe_add(...) -> Result<..., Result<soroban_sdk::Error, ...>>

Example — after the fix:

// Generated client now correctly has:
fn try_safe_add(...) -> Result<..., Result<Error, ...>>

Closes #1710

Known limitations

If a contract both defines an error enum named Error and uses soroban_sdk::Error directly at a function boundary, every Error reference in the spec resolves to the user-defined UDT — there is no way to mix the two in one spec. This matches user intent: a contract that names its own error enum Error will not also be using the SDK error type by that name.

Credit

This PR was forked from #1709 from @willemneal to allow maintainers to handle merge conflicts.

[claude]

Claude Code Review

Working on review...

• Read CLAUDE.md and understand repo conventions
• Read changed files (focus on soroban-spec-rust/src/lib.rs)
• Analyze test changes and snapshots
• Identify potential bugs, edge cases, and quality issues
• Post final review

View job run

[claude]

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

---

Claude Code Review

Approach: focused review on soroban-spec-rust/src/lib.rs (the only non-snapshot, non-test source change) plus a sanity-check of the regenerated trait outputs.

• Read repo conventions
• Read soroban-spec-rust/src/lib.rs changes
• Analyze test changes and snapshots
• Identify potential bugs, edge cases, and quality issues
• Post final review

---

Summary

The fix is well-scoped and well-documented. The chosen approach — rewriting ScSpecTypeDef::Error → Udt { name: "Error" } at client-generation time (instead of at #[contractimpl] spec emission) — is the right call: it preserves the on-wire spec format (no spec-hash drift for already-deployed contracts) and lets every existing contract that already names its error enum Error benefit on the next regen. Test coverage hits all four meaningful paths: trait output for both the override and fallback cases, plus a direct assertion that the on-wire ScSpecTypeDef::Error is unchanged.

Generated code in tests-expanded/test_import_contract_tests.rs:12 confirms the trait now reads fn safe_add(env, a, b) -> Result<u64, Error> referencing the user UDT, while the embedded WASM bytes at line 9 still encode the built-in ScSpecTypeDef::Error (4-byte tag \0\0\0\x06) — exactly the wire-compat property the PR description claims.

Below are observations ordered by significance. None are blocking.

---

Observations

1. to_utf8_string_lossy() makes the override trigger fuzzy on non-UTF-8 input — soroban-spec-rust/src/lib.rs:187

matches!(e, ScSpecEntry::UdtErrorEnumV0(err) if err.name.to_utf8_string_lossy() == "Error")

If a UDT error enum's name bytes are not valid UTF-8, to_utf8_string_lossy will replace bad bytes with U+FFFD, which could in pathological cases yield a string that compares equal to "Error" (e.g. UDT names produced by a non-Rust spec emitter). In practice spec names are ASCII Rust identifiers, so this is unreachable today — but the nearby unit test (test_add_u64_spec_entries) deliberately uses the strict to_utf8_string().unwrap() form.

Suggest aligning on the strict form so both reads of the same field use the same decoding rule:

matches!(
    e,
    ScSpecEntry::UdtErrorEnumV0(err)
        if err.name.to_utf8_string().as_deref() == Ok("Error")
)

Fix this →

2. Rewrite recurses through compound types but isn't tested in nested positions — soroban-spec-rust/src/lib.rs:204-228

rewrite_ty correctly descends into Option, Result, Vec, Map, and Tuple, and the entry-walker visits function inputs/outputs, struct fields, union tuple cases, and event params. This is the right shape — but the snapshot tests only exercise ScSpecTypeDef::Error in the top-level Result.error_type of a function output. A targeted unit test that places Error in, e.g., a struct field or a Vec<Error> would lock in the recursion behavior so a later refactor can't silently regress it. Not blocking; the logic is straightforward enough to read.

3. Naming-collision edge case for the rare "struct named Error" — non-issue, just worth confirming

The override only triggers when a UdtErrorEnumV0 named Error exists. If a contract instead exports a UdtStructV0 named Error (no error enum), ScSpecTypeDef::Error continues to map to soroban_sdk::Error, and the user's pub struct Error is generated alongside — name shadowing in the same module would be a Rust compile error in the generated code. This is the same situation as on main, so the PR neither introduces nor fixes it. Worth briefly noting in the "Known limitations" section of the PR description so future readers don't think the fix covers it.

4. "Error".try_into().unwrap() — soroban-spec-rust/src/lib.rs:208

Safe: 5 ASCII bytes is well under any StringM bound. No change needed.

5. Doc comments and Cow pattern

The doc comments on apply_error_udt_override and rewrite_error_to_udt (lines 168–202) clearly explain why this lives in the client generator rather than the macro emitter, which is exactly the kind of context that's hard to reconstruct from git blame alone. The Cow::Borrowed fast path avoids cloning the entire spec slice when no rewrite is needed — nice touch for the common case where most contracts won't define Error.

6. Integration test signal — tests/import_contract/src/lib.rs:26

The fact that Err(Ok(addcontract::Error::Overflow)) => ... even compiles is a strong end-to-end signal: pre-fix, addcontract::Error was soroban_sdk::Error (which has no Overflow variant), so this match arm would not have type-checked. Useful as a regression guard going forward.

---

Verdict

LGTM modulo the minor to_utf8_string_lossy consistency nit (#1). The wire-format-preserving design is the right call and addresses the review feedback referenced in the PR description.
• Branch: local_fix/error_generation

[Copilot]

Pull request overview

This PR fixes Rust client generation for contracts that define a user error enum named Error and use it as the Err side of a Result<T, Error>: generated clients now reference the user-defined Error UDT instead of soroban_sdk::Error. The fix is implemented in soroban-spec-rust by rewriting ScSpecTypeDef::Error to Udt { name: "Error" } during client generation when the spec includes a UdtErrorEnumV0 named Error, preserving the on-the-wire spec format while improving client type fidelity (including for already-deployed contracts).

Changes:

• Add a spec pre-pass in soroban-spec-rust that conditionally rewrites ScSpecTypeDef::Error references to the Error UDT when an Error error-enum UDT is present.
• Add/extend test contracts and snapshots to cover both Error and non-Error user-defined error enums (MyError) across direct and imported-client usage.
• Add generator-level unit tests validating both (a) the generated Rust signatures and (b) that the wasm spec itself still contains ScSpecTypeDef::Error for Error.

Reviewed changes

Copilot reviewed 8 out of 8 changed files in this pull request and generated 1 comment.

Show a summary per file

File	Description
soroban-spec-rust/src/lib.rs	Implements the conditional spec rewrite pass and adds unit tests to validate correct Rust output and unchanged spec wire format.
tests/add_u64/src/lib.rs	Adds safe_add/safe_add_two entrypoints and defines Error/MyError error enums to exercise the generation behavior.
tests/add_u64/test_snapshots/test/test_safe_add.1.json	Adds snapshot output for the new safe_add test coverage.
tests/import_contract/src/lib.rs	Adds an importing contract that calls into the add_u64 contract and validates error mapping through generated clients.
tests-expanded/test_add_u64_wasm32v1-none.rs	Updates committed expanded output reflecting the new error enums and Result signatures in the add_u64 test contract.
tests-expanded/test_add_u64_tests.rs	Updates committed expanded output reflecting the new add_u64 tests and generated client signatures.
tests-expanded/test_import_contract_wasm32v1-none.rs	Updates committed expanded output reflecting the new imported-client calls and error enum in the import_contract test.
tests-expanded/test_import_contract_tests.rs	Updates committed expanded output reflecting the new import_contract tests and generated client signatures.

[mootz12]

1. to_utf8_string_lossy() makes the override trigger fuzzy on non-UTF-8 input — soroban-spec-rust/src/lib.rs:187

I don't think this needs to be fixed, the check won't pass (corretly) with non-UTF-8 input.

2. Rewrite recurses through compound types but isn't tested in nested positions — soroban-spec-rust/src/lib.rs:204-228

Added a test case here.

3. Naming-collision edge case for the rare "struct named Error" — non-issue, just worth confirming

contracterror types can only be enums. The current name collision issue is out of scope.

[claude]

Claude finished @mootz12's task in 1m 33s —— View job

---

Claude Code Review (follow-up)

• Verify new test for nested compound types (point Add LICENSE #2 from prior review)
• Investigate Copilot's use core::u64; deprecation claim
• Re-evaluate point populate #1 (to_utf8_string_lossy) given mootz12's response
• Post follow-up review

---

Responses

Point #1 (to_utf8_string_lossy) — agreed, no fix needed

mootz12 is right. to_utf8_string_lossy() only differs from the strict variant when the input contains invalid UTF-8 bytes — and in that case it inserts U+FFFD replacement characters. Since "Error" is pure ASCII (no replacement chars and no multi-byte sequences), the only way for to_utf8_string_lossy() == "Error" to hold is for the underlying bytes to be exactly [0x45, 0x72, 0x72, 0x6f, 0x72], which is also what the strict variant would accept. My earlier suggestion was overcautious — the lossy form is safe here. Withdrawing the nit.

Point #2 (nested rewrite test) — addressed at soroban-spec-rust/src/lib.rs:543

test_error_udt_override_rewrites_nested_vec now exercises ScSpecTypeDef::Error inside a Vec element type, asserting the generated trait emits soroban_sdk::Vec<Error> (the user UDT) rather than soroban_sdk::Vec<soroban_sdk::Error>. This locks in the recursion behavior for at least one nested-container case, which is sufficient as a regression guard given that the other arms (Option, Result.ok_type, Map, Tuple) share the same straightforward structural-recursion pattern. Good addition.

Point #3 (struct named Error collision) — agreed, out of scope

#[contracterror] is enum-only at the macro level, so the "user-defined Error struct" scenario isn't reachable in practice. Fair to leave out of scope.

Copilot review on use core::u64; — addressed at tests/add_u64/src/lib.rs:33

The deprecated core::u64 module import has been removed; u64::MAX is referenced directly via the primitive type at tests/add_u64/src/lib.rs:55. Verified the file no longer contains the import.

---

Final verdict

LGTM. The follow-up commit cleanly resolves the only concrete fix request (the deprecated module import that would have failed CI under -Dwarnings) and adds reasonable nested-recursion coverage. The PR is ready from my end.
• Branch: local_fix/error_generation

[claude]

Claude finished @mootz12's task in 49s —— View job

---

Claude Code Review (sync follow-up)

• Identify what changed since last review
• Diff vs prior reviewed state
• Spot-check regenerated snapshot
• Post follow-up

---

The new commit 5d2350c chore: make expand-tests is a routine snapshot regeneration following the previous commit that removed use core::u64; from tests/add_u64/src/lib.rs. Verified:

• Snapshot diff is only in tests-expanded/test_add_u64_tests.rs: drops the use core::u64; line and shifts the #[rustc_test_marker] source-line metadata for test_add (41 → 39) and test_safe_add (52 → 50).
• Cross-checked against tests/add_u64/src/lib.rs: fn test_add is now at line 39 and fn test_safe_add is at line 50, matching the new snapshot exactly.
• No other files changed; no logic or generator behavior touched. The Result<u64, Error> / Result<u64, MyError> signatures and the wire-format-preserving rewrite logic in soroban-spec-rust/src/lib.rs are unchanged from the prior approved state.

Verdict

LGTM — snapshot-only regeneration commit, consistent with the source change it follows. No new review concerns. PR remains in the approved-from-my-end state from the previous round.
• Branch: local_fix/error_generation