fix(escrow): typed EscrowNotFound error instead of .expect() panics - #542
Open
Vyacheslav-Tomashevskiy wants to merge 1 commit into
Open
Conversation
get_escrow(), release(), and refund() all used
.expect("Escrow not found") when looking up a nonexistent escrow_id,
which panics with an untyped, unstructured message that callers can't
match on programmatically.
Added a soroban_sdk::contracterror Error enum with an EscrowNotFound
variant and changed the three functions to return
Result<T, Error>/Result<(), Error>. The generated client keeps the
existing panicking foo() methods for the happy path plus adds
try_foo() methods that return the typed error, so callers that want
to handle a missing escrow gracefully now can.
Scope: only the not-found case from issue Northgate-Systems#332. The other panic!()
calls in this module (bad amount, wrong status, not-yet-expired) are
left untouched -- covering every failure mode is the broader, separate
scope of issue Northgate-Systems#333.
Tests: replaced the two should_panic(expected = "Escrow not found")
tests with typed-error assertions via try_release/try_refund/
try_get_escrow, and added a test confirming the non-try get_escrow()
still returns the state directly on the success path. 15/15 tests
pass (13 pre-existing + 2 new). cargo clippy --all-targets -- -D
warnings is clean. cargo fmt --check shows the same 7 pre-existing
diffs that are already on main (unrelated formatting, not touched by
this change) -- confirmed by running fmt --check on a clean checkout
of main before making any changes.
cargo build --target wasm32-unknown-unknown --release still fails on
this branch the same way it fails on a clean main checkout (Cargo.toml
pulls in soroban-sdk's "testutils" feature as a normal dependency,
which has a hard compile_error! on the wasm target) -- pre-existing,
unrelated to this fix, already flagged to the maintainer in the Northgate-Systems#341
escrow-ids PR.
Closes Northgate-Systems#332
|
Someone is attempting to deploy a commit to the codex723's projects Team on Vercel. A member of the Team first needs to authorize it. |
6 tasks
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
get_escrow(),release(), andrefund()all used.expect("Escrow not found")when looking up a nonexistentescrow_id, which panics with an untyped, unstructured message. This adds asoroban_sdk::contracterrorErrorenum with anEscrowNotFoundvariant and changes the three functions to returnResult<T, Error>.Scope note: this only covers the not-found case from #332. The other
panic!()calls in this module (bad amount, wrong status, not-yet-expired) are intentionally left untouched — covering every failure mode with typed errors is the broader scope of #333, and mixing the two would make this PR harder to review.What changed
contracts/escrow/src/lib.rs: addedErrorenum (#[contracterror]), changedget_escrow/release/refundsignatures toResult<T, Error>, replaced.expect("Escrow not found")with.ok_or(Error::EscrowNotFound)/?.contracts/escrow/src/test.rs: replaced the two#[should_panic(expected = "Escrow not found")]tests with typed-error assertions via the generatedtry_release/try_refund/try_get_escrowclient methods, and added a test confirming the non-try_get_escrow()still returns the state directly on the success path.contracts/escrow/README.md: updated the interface table + a short note on the new error type.The Soroban SDK's
#[contractimpl]macro auto-generates both a panickingfoo()client method (unchanged behavior on the happy path) and atry_foo()method returning the typedResultfor any function returningResult<T, Error>, so this is not a breaking change for existing callers that only used the non-try_methods and never inspected the panic message text.Testing
cargo test: 15/15 pass (13 pre-existing + 2 new:test_get_escrow_nonexistent_returns_typed_error,test_get_escrow_found_still_returns_state_directly; the two not-found tests for release/refund were converted rather than added, so net +1 test file count but the twoshould_panicstring-matching tests became explicit typed-error matches).cargo clippy --all-targets -- -D warnings: clean, no warnings.cargo fmt --check: shows the same 7 pre-existing diffs that are already on a cleanmaincheckout (verified by runningcargo fmt --checkonmainbefore making any changes) — none of them touch code this PR modifies, so I left them alone rather than reformatting unrelated lines.cargo build --target wasm32-unknown-unknown --release: fails identically on this branch and on a cleanmaincheckout (soroban-sdk'stestutilsfeature is pulled in as a normal dependency inCargo.toml, which has a hardcompile_error!on thewasm32target). Pre-existing, unrelated to this change — already flagged to the maintainer in the Support multiple concurrent escrows per sender/recipient pair #342 (escrow IDs) PR thread.Checklist (from the issue)
contracts/escrow/src/lib.rscontracts/escrow/src/test.rscovering success and failure pathscargo testpasses locallycargo clippy -- -D warningsandcargo fmt --checkpass (fmt: no new diffs introduced, see above)README.mdupdated (interface table + error-handling note)Closes #332