Uh oh!
There was an error while loading. Please reload this page.
Fix panic in contract info interface when WASM has env meta but no spec - #2682
Fix panic in contract info interface when WASM has env meta but no spec#2682Galmanus wants to merge 1 commit into
Conversation
There was a problem hiding this comment.
Pull request overview
Note
Copilot was unable to run its full agentic suite in this review.
Refactors Soroban contract interface extraction to avoid a potential panic when a WASM contains env meta but no spec, and adds regression coverage for the scenario.
Changes:
- Extracted WASM spec parsing into
spec_from_wasm, adding an explicit guard for missingcontractspecv0. - Added unit tests that build minimal WASM modules with custom sections to cover missing/partial interface cases.
- Added
wasm-encoderdependency to support generating minimal WASM binaries in tests.
Reviewed changes
Copilot reviewed 2 out of 3 changed files in this pull request and generated 3 comments.
| File | Description |
|---|---|
| cmd/soroban-cli/src/commands/contract/info/interface.rs | Refactor spec parsing into helper and add regression/unit tests for missing spec sections |
| cmd/soroban-cli/Cargo.toml | Add wasm-encoder dependency used by the new test helpers |
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
| let result = spec_from_wasm(&wasm); | ||
| assert!(matches!(result, Err(Error::NoInterfacePresent()))); |
There was a problem hiding this comment.
Checked this against the enum definition — NoInterfacePresent is a zero-field tuple variant (declared NoInterfacePresent() at line 41), not a unit variant. For a zero-field tuple variant, Err(Error::NoInterfacePresent()) is the correct pattern. The two suggested alternatives don't compile here: Error::NoInterfacePresent fails with E0532 (expected unit variant, found tuple variant) and Error::NoInterfacePresent(_) fails with E0023 (pattern has 1 field, variant has 0). CI compiles and passes on the current form, so I'll keep matches!(result, Err(Error::NoInterfacePresent())).
| let result = spec_from_wasm(&wasm); | ||
| assert!(matches!(result, Err(Error::NoInterfacePresent()))); |
There was a problem hiding this comment.
Same as the assertion above: NoInterfacePresent is a zero-field tuple variant (defined NoInterfacePresent() at line 41), so Err(Error::NoInterfacePresent()) is the correct pattern. Error::NoInterfacePresent would fail with E0532 and Error::NoInterfacePresent(_) with E0023 — neither compiles. Keeping the current form; CI confirms it builds and passes.
| walkdir = "2.5.0" | ||
| mockito = "1.5.0" | ||
| serial_test = "3.0.0" | ||
| wasm-encoder = "0.235.0" |
There was a problem hiding this comment.
wasm-encoder is already under [dev-dependencies] — it's at line 145, inside the [dev-dependencies] block that starts at line 138. It's only referenced by the #[cfg(test)] module in interface.rs, so it doesn't add a runtime/build dependency for non-test builds. No change needed here.
…spec `contractenvmetav0` and `contractspecv0` are independent custom sections, so a WASM can carry env meta without a spec. The command guarded on `env_meta_base64` being present but then unwrapped `spec_base64`, panicking instead of reporting "no interface present". Extract the WASM branch into `spec_from_wasm` so it is unit-testable, replace the unwrap with a clean `NoInterfacePresent` error, and add regression tests that build minimal WASM modules with `wasm-encoder` (already used by soroban-spec-tools tests). Fixesstellar#2429
21e5e1d to
9e0da76Compare
Fixes#2429.
contractenvmetav0andcontractspecv0are independent WASM custom sections, so a WASM can carry env meta without a spec.contract info interfaceguarded onenv_meta_base64being present but then called.unwrap()onspec_base64— a completely independent field — and panicked instead of reportingno interface present in provided WASM file.Fix
run()intospec_from_wasm()so it is unit-testable.unwrap()with a cleanNoInterfacePresenterror, keeping the existing check order and semantics otherwise.Tests
Regression tests build minimal WASM modules with
wasm-encoder(added as a dev-dependency; already used bysoroban-spec-toolstests at the same version):env_meta_without_spec_returns_no_interface_error— the panic case; failed withcalled Option::unwrap() on a None valuebefore the fix.missing_env_meta_returns_no_interface_error— existing behavior preserved.