Skip to content

fix(soroban): fix cargo test — 70+ pre-existing compile and runtime failures - #16

Merged
Meshmulla merged 9 commits into
stellar-kracken:mainfrom
Meshmulla:feature/1-fix-cargo-test-suite
Jul 16, 2026
Merged

fix(soroban): fix cargo test — 70+ pre-existing compile and runtime failures#16
Meshmulla merged 9 commits into
stellar-kracken:mainfrom
Meshmulla:feature/1-fix-cargo-test-suite

Conversation

@Meshmulla

Copy link
Copy Markdown
Contributor

Summary

Third follow-up to #13 (alongside #14 and #15). With the ethnum fix (#14) and the escrow_contract crate split (#15) applied, cargo build --release --target wasm32-unknown-unknown goes green — but cargo test still failed outright. This PR fixes it, going from "doesn't compile" to 725 tests passing, 0 failing, verified on a real disposable CI run combining all three fixes together.

Note on the diff: this branch is stacked on #15 (which is stacked on #13), so GitHub is showing the cumulative diff since feature/1-split-escrow-contract-crate only exists on my fork and can't be used as a cross-repo PR base. The commits unique to this PR are the 8 most recent ones (fix(soroban): fix cargo test compile errors... through fix(soroban): fix version_migration_tests runtime failures) — everything before that is #15's content. Once #14/#15 merge, rebasing will shrink this to just those 8 commits.

None of this had ever been exercised by CI before, so it had accumulated two waves of pre-existing breakage: things that didn't even compile, and things that compiled but panicked at runtime once the compile errors were cleared out of the way.

Wave 1 — compile errors

  • Several soroban/tests/*.test.rs files imported the crate under its pre-rename name (bridge_watch_soroban / bridge_watch_contracts) instead of the current swipely_contracts.
  • emergency_fund_recovery_tests.rs and version_migration_tests.rs used crate:: to reach the library under test, which doesn't resolve from an integration test (a separate crate) — switched to swipely_contracts::.
  • insurance_pool.test.rs and governance.test.rs had no [[test]] entry in Cargo.toml, so Cargo auto-discovered them and tried to derive a binary name from the file stem (insurance_pool.test), which isn't a valid crate name because of the literal dot.
    • Digging further: both governance/insurance_pool modules are #[cfg(test)]-gated in lib.rs (to keep them out of the wasm build, avoiding the symbol-collision problem fix(soroban): extract escrow_contract into its own crate to fix wasm build #15 fixed for escrow_contract). #[cfg(test)] is only true when a crate compiles itself in test mode — never true for an external crate merely linking it as a dependency, which is exactly what a tests/*.rs file is. So these two files could never work as integration tests, regardless of naming — a structural dead end. Both contracts already have substantial in-module #[cfg(test)] mod tests covering the same scenarios, so I removed the two external files rather than fight the architecture.
  • source_trust.test.rs called Ledger::with_mut without importing the soroban_sdk::testutils::Ledger trait that provides it.
  • asset_deprecation.rs: two helpers were pub fn inside the #[contractimpl] block, making soroban-sdk treat them as contract endpoints and enforce serializable-by-value parameter types — but they take &Env/&String and are only called internally. Made them plain (non-pub) fns, matching an existing helper in the same impl block.
  • liquidity_pool.rs: two tests asserted against a .depth field that doesn't exist (real fields are pool_id/reserve_a/reserve_b/total_value_locked/depth_score/timestamp), and one used format!, unavailable in this #![no_std] crate.
  • batch_query.rs: format! again (fixed via a #[cfg(test)]-gated extern crate alloc + explicit .to_string() calls, kept test-only since the wasm release build has no allocator wired up), plus an enum variant literally named Error colliding with #[contracttype]'s generated TryFromVal impl ("ambiguous associated item") — renamed to Failure.

Wave 2 — runtime failures (57, then 16, then a handful more as later targets were finally reached)

  • Missing env.as_contract() context (the single biggest category): several free-function modules' own #[cfg(test)] mod tests — and two tests/*.rs integration files (emergency_fund_recovery_tests.rs, version_migration_tests.rs) — called contract functions directly, with no contract registered. soroban-sdk requires an active contract call frame for any env.storage() access. Registered a minimal dummy #[contract] in each setup()/setup_env() and wrapped calls in env.as_contract(&contract_id, || ...).
  • Double require_auth() per frame: soroban's host allows require_auth() for a given address only once per call frame. Two distinct bugs from this:
    • Several public contract entrypoints call another public entrypoint that also require_auth()s the same address (set_config_bulk/init_default_configset_config; submit_*_signedsubmit_*). Extracted *_internal() variants without the auth check that both callers now share.
    • Several tests called two or more auth-requiring free functions on the same address inside one env.as_contract() block. Split each mutating call into its own block — this is already a documented idiom elsewhere in the repo (see soroban/tests/operator_rotation.test.rs), just not followed consistently in the modules' own internal suites.
  • A real storage-key bug: get_all_configs() read the config-keys list from keys::CONFIG_KEYS (a plain string constant) while set_config writes that list under DataKey::ConfigKeys (a different, unrelated key) — two different storage slots, so get_all_configs() always saw an empty list. Fixed the read to match the write.
  • A real replay-protection gap: verify_signature() cached verification results keyed only by the message payload hash, checked before the nonce-replay check. Resubmitting the exact same signed payload (a textbook replay attack) hit the cache and returned true without ever reaching the nonce check. Moved the nonce check ahead of the cache lookup.
    • Adjacent, not fixed here: verify_multi_sig() calls verify_signature() once per distinct signer against the same message, so a second signer's call can still short-circuit past real signature validation via a cache hit left by an earlier signer's legitimate check. No test currently exercises multi-signer verification against a shared message, so I left this as a flagged follow-up rather than redesigning the cache blind.
  • Two real missing guards: pause_asset()/unpause_asset() were missing the assert_asset_not_locked() guard that every other asset-mutating entrypoint (submit_health, submit_price, ...) already calls — a locked asset could still be paused/unpaused. Added the guard, matching the existing pattern.
  • A real missing admin exemption: the trusted-source check in submit_health/submit_price applied to every caller including admin, with no bypass — but check_permission()'s equivalent role check already exempts admin elsewhere in the same file, and an existing test (test_admin_can_always_submit_regardless_of_trust) is explicitly written to expect that same exemption. Added it.
  • A handful of tests asserted timestamp > 0 fields without ever calling env.ledger().set_timestamp(...) in setup(), so they compared against the default t=0. Added the same set_timestamp(1_000_000) every other test file's setup already uses.
  • One test (remove_operator) always targeted the only active operator, tripping a real "cannot remove the last active operator" safety guard — added a second operator so the removal path under test can actually succeed.
  • One test's own name/assertions were checking the wrong thing given real (and correct, documented) contract behavior: consume_limit() deliberately persists breach side-effects (cooldown) even on a rejected call. Rewrote the assertion to check the usage counter directly (get_user_usage()) rather than check_limit(), which now correctly errors with CooldownActive right after a breach.

Testing/validation performed

Combined this branch with #14 (ethnum) and #15 (escrow split) on a disposable branch and pushed to real GitHub Actions repeatedly through this fix, iterating on each newly-surfaced failure as earlier ones were cleared (cargo stops scheduling new compile targets after a failure, so later targets' bugs were invisible until earlier ones were fixed). Final run: build green, test green — 725 passed, 0 failed, across every target (swipely-contracts lib + all soroban/tests/* integration targets + transfer-state-machine + escrow-contract). The disposable validation branch has been deleted — it was for verification only, not a deliverable.

Related

Building the workspace for wasm32-unknown-unknown failed with:

  error: symbol `initialize` is already defined
     --> soroban/src/escrow_contract.rs:112:1

The soroban crate's own top-level contract (in lib.rs) and
TimeLockedEscrowContract both export a wasm symbol named `initialize`.
Since they compiled into the same cdylib, the wasm export tables
collided. A comment already in lib.rs notes that governance and
insurance_pool were previously handled the same way (test-only via
#[cfg(test)]) for this exact reason, but escrow_contract had never
gotten that treatment and was still compiled unconditionally.

Fix: move escrow_contract.rs out of the soroban crate into its own
workspace member, `escrow_contract/`, matching the existing
transfer_state_machine crate layout (its own Cargo.toml, its own
wasm cdylib output). It has no dependency on any other soroban
module, so the move is a straight file relocation plus:

- soroban/src/lib.rs: drop `pub mod escrow_contract;`
- soroban/Cargo.toml: drop the escrow_contract_test [[test]] entry
- root Cargo.toml: add escrow_contract to workspace members
- Cargo.lock: add the escrow-contract workspace member entry
- README.md: document the new crate in the workspace table
- escrow_contract/src/lib.rs: add `#![no_std]` at the crate root.
  It previously inherited this from soroban/src/lib.rs as a submodule;
  as its own crate root it needs the attribute itself, otherwise it
  links libstd's panic_impl alongside soroban-sdk's own, which
  duplicates the lang item (E0152) under the wasm32 target.

Also fixed a stale import in the moved test file
(escrow_contract/tests/escrow_contract.test.rs) that referenced the
crate's pre-rename name `bridge_watch_contracts` instead of the
current `escrow_contract`.

Verified via a real GitHub Actions run (on a disposable branch,
combined with the ethnum bump from the companion PR) that
`cargo build --release --target wasm32-unknown-unknown` now succeeds
for the full workspace with these two changes together.
cargo test had never been run in CI before, so it had accumulated
several independent, pre-existing breakages:

- soroban/tests/*.test.rs: several integration test files imported
  the crate under its pre-rename name (bridge_watch_soroban /
  bridge_watch_contracts) instead of the current swipely_contracts.
- soroban/tests/emergency_fund_recovery_tests.rs and
  version_migration_tests.rs: used `crate::` to reach the library
  under test, which doesn't work from an integration test (a
  separate crate) — switched to swipely_contracts::. Also fixed a
  borrow-after-move (`env` used after being moved into
  cancel_recovery) in test_cancel_recovery.
- soroban/tests/source_trust.test.rs called Ledger::with_mut without
  importing the soroban_sdk::testutils::Ledger trait that provides it.
- Removed soroban/tests/governance.test.rs and
  soroban/tests/insurance_pool.test.rs (and their [[test]] entries).
  Both `pub mod governance;` and `pub mod insurance_pool;` in lib.rs
  are #[cfg(test)]-gated (to keep them out of the wasm release build,
  same as several other standalone contracts). #[cfg(test)] is only
  true when a crate compiles *itself* in test mode — it's never true
  for an external crate that merely links this one as a dependency,
  which is exactly what a tests/*.rs integration test is. So these
  two files could never resolve `swipely_contracts::governance` /
  `::insurance_pool` no matter what they're named; it's a structural
  dead end, not a naming bug. Both contracts already have their own
  substantial in-module `#[cfg(test)] mod tests` covering the same
  scenarios (initialize/delegate/vote/quorum/guardian-execute for
  governance; stake/claim/slash/cap/duplicate-approval for
  insurance_pool), so no coverage is lost.
- soroban/src/asset_deprecation.rs: get_deprecation_config and
  check_write_allowed were `pub fn` inside the #[contractimpl] block,
  which makes soroban-sdk treat them as contract endpoints and
  enforce serializable-by-value parameter types — but they take
  `&Env`/`&String` and are only ever called internally via Self::.
  Made them plain (non-pub) fns, matching the require_admin helper
  already in the same impl block.
- soroban/src/liquidity_pool.rs: two tests asserted against a
  `.depth` field that doesn't exist on LiquidityDepth (available
  fields are pool_id/reserve_a/reserve_b/total_value_locked/
  depth_score/timestamp) and one test built a dynamic pool id with
  `format!`, which isn't available in this #![no_std] crate. Fixed
  the assertions to check total_value_locked (recomputed the correct
  expected value from the actual pricing formula) and replaced the
  format! loop with a fixed array of pool id literals.
- soroban/src/batch_query.rs: two helpers used `format!` (also
  unavailable under no_std without alloc) to build JSON-like strings,
  interpolating soroban_sdk::String fields directly even though that
  type only implements Debug, not Display. Added a #[cfg(test)]-gated
  `extern crate alloc` (soroban/src/lib.rs — kept test-only since the
  wasm release build has no global allocator wired up) and switched
  to alloc::format! with explicit .to_string() calls on each field.
  Separately, the QueryResult enum had a variant literally named
  `Error`, which collided with the associated `Error` type from the
  TryFromVal trait that #[contracttype] generates an impl for
  ("ambiguous associated item") — renamed the variant to `Failure`
  (not referenced outside this file).

None of this touches wasm-facing production behavior: batch_query,
asset_deprecation, and liquidity_pool's affected tests are all
already #[cfg(test)]-gated or test-only code paths.
Two distinct root causes:

1. Several free-function modules (liquidity_pool, operator_rotation,
   source_blessing, threshold_window) have internal #[cfg(test)] unit
   tests that call env.storage() directly, outside of any contract
   invocation. soroban-sdk only allows storage access from within an
   active contract call frame, so every one of these panicked with
   "this function is not accessible outside of a contract, wrap the
   call with env.as_contract()". Added a minimal dummy #[contract]
   registered in each module's test setup(), and wrapped each test
   body in env.as_contract(&contract_id, || { ... }).

2. Several public contract entrypoints that already require_auth()
   internally call ANOTHER public entrypoint that also require_auth()s
   the same address within the same invocation. soroban's host tracks
   per-address authorization per call frame and rejects a second
   require_auth() for an address already authorized in that frame
   ("frame is already authorized" / Error(Auth, ExistingValue)):
     - set_config_bulk and init_default_config both looped calling
       set_config(), which itself calls require_auth(). Extracted
       set_config_internal() (no auth check) that both now call after
       authorizing once.
     - submit_health_signed / submit_price_signed /
       submit_health_batch_signed check_permission() (which
       require_auth()s) before verifying the signature, then called
       submit_health/submit_price/submit_health_batch, which
       check_permission() again. Extracted *_internal() variants
       (no auth check) for all three that both the signed and
       unsigned public entrypoints now share.

Also fixed a real storage-key bug found via test_get_all_configs_
returns_all_stored_entries and test_config_all_three_categories:
get_all_configs() read the config-keys list from `keys::CONFIG_KEYS`
(a plain string constant), but set_config_internal() writes that same
list under `DataKey::ConfigKeys` (a different, unrelated storage key)
— two different slots, so get_all_configs() always saw an empty list.
Changed the read to use DataKey::ConfigKeys, matching the write.

Also fixed rate_limiter::tests::test_rejected_consume_does_not_record_
usage, which asserted check_limit() would still report `allowed` after
a rejected consume_limit() call — but a breach deliberately triggers a
cooldown (documented behavior on consume_limit), so check_limit()
correctly errors out with CooldownActive afterward. Rewrote the
assertion to read get_user_usage() directly, which is what the test
name actually describes (usage counters must not advance on a
rejected/breached attempt) and isn't gated by the cooldown.
Two more root causes found after the first round of 57 fixes:

1. Several free-function tests called two or more auth-requiring
   functions on the same address inside a single env.as_contract()
   block (e.g. add_operator then remove_operator for the same admin).
   Each require_auth() consumes the current call frame's single-use
   authorization for that address — calling it twice within the same
   frame panics with "frame is already authorized", same as the
   contract-endpoint double-auth bug fixed earlier, just triggered via
   raw as_contract usage instead of nested public entrypoints. This is
   already a documented idiom elsewhere in the repo (see the comment
   in soroban/tests/operator_rotation.test.rs), just not followed
   consistently in the modules' own internal test suites. Split each
   mutating call into its own env.as_contract() block in
   operator_rotation, source_blessing, and threshold_window tests.

2. verify_signature() cached signature-verification results keyed
   only by the sha256 of the message payload, and checked that cache
   *before* checking nonce replay. Resubmitting the exact same signed
   payload (a textbook replay attack) hit the cache and returned true
   without ever reaching the nonce check, defeating replay protection
   for the most common replay scenario. Moved the nonce check ahead
   of the cache lookup so replay is always caught regardless of cache
   state; a cache hit now also still advances the stored nonce.

3. tests::test_propose_upgrade_sets_pending_with_timelock,
   test_emergency_upgrade_executes_after_additional_approval, and
   test_cancel_upgrade_clears_pending_proposal called assert_has_event
   (checking env.events().all()) only after one or more additional
   read-only client calls following the event-emitting call. In this
   soroban-sdk version, interleaving further client calls before
   inspecting events can leave earlier events out of the recorded
   set. Reordered each test to check its event immediately after the
   call that emits it, before any subsequent read-only calls.

Note: verify_multi_sig() calls verify_signature() once per distinct
signer against the same message payload. The same payload-only cache
key described above means a second signer's call can still short-
circuit past real signature validation via a cache hit left by an
earlier signer's legitimate check on that message. No currently
passing or failing test exercises multi-signer verification against
a shared message, so this wasn't touched here, but it's a real gap
worth a dedicated follow-up (e.g. scoping the cache by signer_id too).
remove_operator() refuses to remove an operator if doing so would
leave zero other active operators (a real safety invariant). Both
test_remove_operator and test_reactivate_operator only ever added the
one operator under test, so their remove_operator() call always hit
this guard and panicked with "cannot remove the last active operator"
instead of exercising what the test names describe. Added a second,
untouched operator to each so the removal path they're actually
testing can succeed.
- soroban/tests/asset_locking.test.rs: setup() never called
  env.ledger().set_timestamp(...), so the ledger stayed at its default
  t=0. Several assertions expected timestamp fields (locked_at,
  unlocked_at, history entry.timestamp) to be > 0, which can never be
  true at t=0. Added the same set_timestamp(1_000_000) used by every
  other test file's setup().

- soroban/src/lib.rs: pause_asset() and unpause_asset() were missing
  the assert_asset_not_locked() guard that every other asset-mutating
  entrypoint (submit_health, submit_price, register_asset's siblings)
  already calls. A locked asset could still be paused/unpaused, which
  is exactly what test_pause_asset_blocked_when_locked and
  test_unpause_asset_blocked_when_locked (both pre-existing,
  #[should_panic(expected = "asset is locked for maintenance")]) were
  written to catch. Added the guard call to both, matching the
  existing pattern.
All 14 tests called EmergencyFundRecovery::method(...) directly as
associated functions, with no contract registered, no
env.mock_all_auths(), and no env.as_contract() wrapping. Every method
touches env.storage() and calls require_auth(), so every single test
panicked with "no contract running" (HostError: Context/InternalError)
once earlier compile errors elsewhere stopped masking it.

Registered a minimal dummy #[contract] in setup_env(), enabled
env.mock_all_auths(), and wrapped each EmergencyFundRecovery:: call in
its own env.as_contract() block — each require_auth() consumes its
call frame's single-use authorization for that address, so (per the
same pattern fixed earlier in operator_rotation/source_blessing/
threshold_window) sequential auth-requiring calls on the same address
need separate frames, not one shared block.
- soroban/tests/source_trust.test.rs: setup() never set the ledger
  timestamp (same gap as asset_locking.test.rs), so registered_at
  fields stayed 0 and failed `> 0` assertions in
  test_source_info_contains_correct_data and
  test_source_registration_audit_trail. Added set_timestamp(1_000_000).

- soroban/src/lib.rs: submit_health_internal/submit_price_internal
  enforced the trusted-source check against every caller, including
  admin, once any trusted source was registered. check_permission()
  already exempts admin from the equivalent role check elsewhere in
  this file, and test_admin_can_always_submit_regardless_of_trust is
  explicitly written (with a matching comment) to expect admin to
  bypass trust enforcement the same way. Added the same admin
  exemption to the trust check at both call sites.
Same class of bug as emergency_fund_recovery_tests: 13 of 19 tests
called EnhancedMigrationHelper::method(&env, ...) directly with no
contract registered and no env.as_contract() wrapping, so every
storage-touching call panicked with "no contract running". The 6
tests that only call validate_upgrade() (pure version-comparison
logic, no Env parameter at all) were already passing and are
untouched.

Registered a minimal dummy #[contract] in setup_env(), enabled
env.mock_all_auths(), and wrapped each storage- or auth-touching call
in its own env.as_contract() block, consistent with the same fix
already applied to operator_rotation/source_blessing/threshold_window/
emergency_fund_recovery_tests.
@Meshmulla
Meshmulla merged commit 49c77cf into stellar-kracken:main Jul 16, 2026
1 check passed
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