fix(escrow): harden internal.rs and complete multi-asset/refund flows - #789
Merged
JSE19 merged 4 commits intoAug 27, 2026
Merged
Conversation
The tally_votes function used saturating_add for vote counting, which silently caps at u32::MAX when overflow would occur. Replaced with checked_add that returns ArithmeticError on overflow, matching the rest of the codebase's financial math pattern. closes JSE-ORG#716
transition_state was annotated #[allow(dead_code)] and never called by any production code; the real state guards are enforced inline. Removed the dead function, its #[allow(dead_code)], and the test_escrow_states module that only exercised it. closes JSE-ORG#717
Multi-asset support (per-escrow token selection) is already in place for create_escrow, EscrowData.token, and fund_escrow. Adds a test confirming the escrow's chosen token is actually enforced: funding with a buyer who only holds a different token reverts and leaves the escrow untouched. closes JSE-ORG#729
request_refund/approve_refund already existed for the Funded -> RefundRequested -> Refunded flow. Completes the third acceptance criterion: the seller can now mark_shipped from RefundRequested, which overrides the outstanding refund request and transitions to Shipped without returning funds. Adds tests covering the override path and confirming Pending is still rejected. closes JSE-ORG#730
|
@Asta-wizard Great news! 🎉 Based on an automated assessment of this PR, the linked Wave issue(s) no longer count against your application limits. You can now already apply to more issues while waiting for a review of this PR. Keep up the great work! 🚀 |
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
2 participants
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
Four escrow contract issues in one batch (Stellar Wave):
tally_votesoverflow safety: switched fromsaturating_addtochecked_addreturningArithmeticError, matching the rest of the codebase's financial math.transition_statefunction (#[allow(dead_code)]) and thetest_escrow_statesmodule that only exercised it; the real state guards were already enforced inline.EscrowData.token,create_escrowtoken param,fund_escrowhonoring it); added a test proving funding with the wrong token reverts.request_refund/approve_refund); completed the missing third criterion: a seller can nowmark_shippedfromRefundRequested, overriding the refund request, plus tests for that path and thePendingrejection.Motivation
Each issue describes a correctness gap or dead code in the escrow lifecycle; acceptance criteria were verified against the current code before implementing.
Changes
internal.rs:tally_votesnow useschecked_addand propagatesContractError::ArithmeticErroron overflow; removed deadtransition_state.instructions.rs:mark_shippedaccepts bothFundedandRefundRequested, so a seller can override an outstanding refund request.lib.rs: caller updated fortally_voteschange; test module registrations pruned/added accordingly.test_multi_asset.rs(new): wrong-token funding must revert (Add multi-asset support with per-escrow token choice #729).test_refund_override.rs(new):RefundRequested -> Shippedoverride andPendingrejection (Implement buyer-initiated refund before shipment #730).test_escrow_states.rs(deleted): only tested the removed dead function (internal.rs — transition_state is dead code with #[allow(dead_code)] #717).Test Coverage
make fmt-check,make clippy(-D warnings),make test(433 passed / 0 failed),make build-wasm— all green.fund_with_wrong_token_fails(Add multi-asset support with per-escrow token choice #729);mark_shipped_overrides_refund_request,mark_shipped_rejects_pending_even_after_refund_request_path(Implement buyer-initiated refund before shipment #730).Notes for Reviewers
contracts/escrow/test_snapshots/were not touched: they are stale relative tomainand regenerated automatically by the Soroban test harness on everycargo testrun (unrelated to this change).create_escrow,request_refund,approve_refund,mark_shipped) is unchanged; only an internal helper's signature changed.Closes #716
Closes #717
Closes #729
Closes #730