feat: implement memo-based withdrawals in escrow contract #142
Conversation
- Add support for memo-based staged withdrawals - Implement tests for memo-based withdrawal functionality - Fix test environment setup and authorization handling - Update escrow contract to handle memo-based releases - Clean up code warnings and deprecated method usage Resolves Trustless-Work#109
WalkthroughIntroduces a new Soroban escrow contract crate with full implementation: contract entrypoints, core managers (escrow, dispute, milestone), validators, math/fee utilities, storage types, events, and extensive tests with JSON snapshots. Adds memo-based withdrawal flow, dispute management/resolution, milestone state/approval handling, balance queries, and TTL extension. Changes
Sequence Diagram(s)sequenceDiagram
autonumber
actor Caller as Caller
participant Escrow as EscrowContract
participant EscMgr as EscrowManager
participant Token as TokenClient
rect rgb(240,248,255)
note over Caller,Escrow: Release with optional memo
Caller->>Escrow: release_funds(release_signer, trustline)
Escrow->>EscMgr: get_escrow + validate_release_conditions
EscMgr->>Token: balance(contract)
EscMgr->>EscMgr: calculate_standard_fees(total, platform_fee_bps)
alt receiver_memo is empty
EscMgr->>Token: transfer(contract -> receiver, receiver_amount)
else receiver_memo present
EscMgr->>EscMgr: set pending_receiver_amount, mark fees_released
end
EscMgr->>Token: transfer(contract -> platform, platform_fee)
EscMgr->>Token: transfer(contract -> trustless_work, tw_fee)
EscMgr-->>Escrow: updated escrow
Escrow-->>Caller: Result
end
rect rgb(245,255,245)
note over Caller,Escrow: Withdraw with memo (second step)
Caller->>Escrow: withdraw_with_memo()
Escrow->>EscMgr: get_escrow + validate_withdrawal_conditions
EscMgr->>Token: transfer(contract -> receiver, pending_amount)
EscMgr->>EscMgr: clear pending_receiver_amount, mark released
EscMgr-->>Escrow: updated escrow
Escrow-->>Caller: Result
end
sequenceDiagram
autonumber
actor Party as Authorized Party
participant Escrow as EscrowContract
participant Dispute as DisputeManager
participant EscMgr as EscrowManager
participant Token as TokenClient
note over Party,Escrow: Open dispute
Party->>Escrow: dispute_escrow(signer)
Escrow->>Dispute: validate + mark disputed
Dispute-->>Party: Escrow state
note over Party,Escrow: Resolve dispute
Party->>Escrow: resolve_dispute(dispute_resolver, trustline, distributions)
Escrow->>Dispute: load escrow + validate conditions
Dispute->>Token: balance(contract)
Dispute->>Token: transfer(contract -> TW, tw_fee)
Dispute->>Token: transfer(contract -> platform, platform_fee)
loop for each participant share
Dispute->>Token: transfer(contract -> participant, net_share)
end
Dispute->>Dispute: update flags (resolved, disputed=false)
Dispute-->>Party: Escrow state
Estimated code review effort🎯 4 (Complex) | ⏱️ ~75 minutes Possibly related issues
Possibly related PRs
Poem
Pre-merge checks and finishing touches❌ Failed checks (2 warnings)
✅ Passed checks (3 passed)
✨ Finishing touches
🧪 Generate unit tests
Tip 👮 Agentic pre-merge checks are now available in preview!Pro plan users can now enable pre-merge checks in their settings to enforce checklists before merging PRs.
Please see the documentation for more information. Example: reviews:
pre_merge_checks:
custom_checks:
- name: "Undocumented Breaking Changes"
mode: "warning"
instructions: |
Pass/fail criteria: All breaking changes to public APIs, CLI flags, environment variables, configuration keys, database schemas, or HTTP/GraphQL endpoints must be documented in the "Breaking Change" section of the PR description and in CHANGELOG.md. Exclude purely internal or private changes (e.g., code not exported from package entry points or explicitly marked as internal).Please share your feedback with us on this Discord post. Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
|
@armandocodecr review and let me know it says "Merging is blocked |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 11
🧹 Nitpick comments (45)
Timebased Escrows/contracts/escrow/test_snapshots/tests/test/memo_withdrawal_test/test/test_withdraw_with_memo.1.json (2)
559-565: Unexplained 3,000 remaining at escrow after withdrawalAfter withdraw_with_memo:
- Escrow balance shows 3,000 (Line 563).
- Receiver balance shows 996,500 (Line 703).
- withdrawal_state.pending_receiver_amount is 0 and fees_released = true (Lines 385-408).
If the two‑step flow releases only fees first and then the full remaining to the receiver, escrow should typically end with 0 unless some amount is intentionally retained (e.g., affiliate payouts not yet sent, dust/rounding bucket). Please confirm the intended sink of 3,000 or adjust the snapshot to drive escrow to 0. Otherwise this introduces accounting ambiguity in downstream assertions and analytics.
Also applies to: 699-705, 385-408
864-867: Consider asserting events for withdraw_with_memoevents is empty. Given the PR exposes WithdrawMemo events, capturing them in the snapshot helps prevent regressions and documents the on‑chain audit trail. Recommend adding the event(s) to the snapshot or explicit test assertions.
Timebased Escrows/contracts/escrow/test_snapshots/tests/test/memo_withdrawal_test/test/test_release_funds_with_memo.1.json (1)
389-407: Add event assertions for the “release with memo” stagingSnapshot shows fees_released = true and pending set, but events is empty (Lines 794-842). Recommend capturing a ReleaseFunds/WithdrawMemoStaged event to lock the API contract and ease debugging.
Also applies to: 794-842
Timebased Escrows/contracts/escrow/test_snapshots/tests/test/test_fund_escrow_signer_insufficient_funds_error.1.json (1)
1-55: Ensure the negative-path is actually exercised and assertedThis snapshot mints 1,000,000 to the signer, while the escrow amount is 100,000,000 (Lines 196-201), consistent with “insufficient funds.” Please verify:
- The test invokes fund_escrow in this scenario (not visible in this snapshot) and
- It asserts the specific error (e.g., ContractError::InsufficientFunds or sdk error code), not just a generic failure.
If not already done, add an assertion on the precise error to avoid false positives.
Also applies to: 160-429, 431-499
Timebased Escrows/contracts/escrow/test_snapshots/tests/test/test_fund_escrow_successful_deposit.1.json (1)
1319-1320: Optional: Capture FundEsc eventsAs with other snapshots, events is empty. If the contract emits FundEsc (or similar), consider snapshotting it to lock the event schema.
Timebased Escrows/contracts/escrow/src/tests/mod.rs (1)
1-2: Gate tests to avoid bloating the release WasmThese modules are fine. Ensure lib.rs includes them under cfg(test) so they’re excluded from the contract Wasm:
// in src/lib.rs #[cfg(test)] mod tests;If already gated elsewhere, ignore.
Timebased Escrows/contracts/escrow/src/tests/test/memo_withdrawal_test.rs (4)
59-59: Fix inconsistent indentationThe indentation is inconsistent with the rest of the file.
-let (contract_id, token_admin, escrow, _token_client, token_admin_client) = create_escrow_with_memo(&env); + let (contract_id, token_admin, escrow, _token_client, token_admin_client) = create_escrow_with_memo(&env);
87-87: Fix inconsistent indentationThe indentation is inconsistent with the rest of the file.
-let (contract_id, token_admin, escrow, _token_client, token_admin_client) = create_escrow_with_memo(&env); + let (contract_id, token_admin, escrow, _token_client, token_admin_client) = create_escrow_with_memo(&env);
118-118: Fix inconsistent indentationThe indentation is inconsistent with the rest of the file.
-let (contract_id, _token_admin, escrow, _token_client, token_admin_client) = create_escrow_with_memo(&env); + let (contract_id, _token_admin, escrow, _token_client, token_admin_client) = create_escrow_with_memo(&env);
136-136: Fix inconsistent indentationThe indentation is inconsistent with the rest of the file.
-let (contract_id, token_admin, mut escrow, _token_client, token_admin_client) = create_escrow_with_memo(&env); + let (contract_id, token_admin, mut escrow, _token_client, token_admin_client) = create_escrow_with_memo(&env);Timebased Escrows/contracts/escrow/src/modules/math/basic.rs (1)
15-17: Consider unifying subtraction error to “Overflow” for signed intsFor signed i128,
checked_subfails on either direction overflow; labeling it “Underflow” can be misleading to callers. Either map toContractError::Overflowor introduce a singleArithmeticOverflowused by both add/sub. Optional: document current semantics if you keepUnderflow.Timebased Escrows/contracts/escrow/test_snapshots/tests/test/test_dispute_escrow_authorized_and_unauthorized.1.json (1)
425-425: TTL sanity checkPersistent entry TTL is 4095 while
min_persistent_entry_ttlis 4096 in this snapshot. If your simulator enforces the min at write time, this is fine; otherwise consider bumping TTL to ≥4096 to avoid edge failures when tools update.Timebased Escrows/contracts/escrow/src/core/validators/milestone.rs (2)
8-22: Add a validator to ensure new_status is non‑empty for status changes
validate_milestone_status_change_conditionsdoesn’t (and can’t) checknew_status. Recommend a small helper (or extend this one’s signature) to reject empty statuses at the status‑change callsite for consistency with the approval path.
30-33: Typo in error variant name (“Milstone”)If practical, consider introducing a correctly spelled variant (deprecated old one) to improve API polish. Keep backward compatibility if external users match on this error.
Timebased Escrows/contracts/escrow/src/core/milestone.rs (2)
30-35: Reject empty new_status earlyApproving a milestone later forbids empty status, but status changes allow it. Guard here for consistency.
- if let Some(evidence) = new_evidence { + if new_status.is_empty() { + return Err(ContractError::EmptyMilestoneStatus); + } + + if let Some(evidence) = new_evidence { milestone_to_update.evidence = evidence; } milestone_to_update.status = new_status;
54-63: Minor: perform validations before fetching the milestoneYou can compute
idxonce (as above) and then run validations that don’t depend on mutable state; this keeps mutation after all checks. Not critical.Timebased Escrows/contracts/escrow/src/core/dispute.rs (1)
32-38: Minor: early continue is fine; consider rejecting zero/negative amounts up‑front.You already error on non‑positive amounts while summing. Keeping that strictness is good; alternatively, reject the entire map if any entry ≤ 0 to avoid silent skipping later logic.
Timebased Escrows/contracts/escrow/test_snapshots/tests/test/test_release_funds_successful_flow.1.json (1)
1-1174: Prefer targeted assertions over massive golden snapshots.This JSON is very large and brittle. Where feasible, assert a few critical invariants (balances, flags, roles, fees) instead of the entire ledger object to minimize test churn and review noise.
Timebased Escrows/contracts/escrow/src/core/validators/dispute.rs (1)
38-72: Optional: reorder checks for clearer intent.You can reject
signer == dispute_resolverbefore buildingis_authorizedto cut one branch and keep the specific error. Current logic is correct; this is purely readability.Timebased Escrows/contracts/escrow/src/events/handler.rs (2)
1-2: Consider importing Address for cleaner code.Since
soroban_sdk::Addressis used multiple times throughout the file, consider importing it alongsideStringfor consistency and readability.-use soroban_sdk::{contractevent, String}; +use soroban_sdk::{contractevent, Address, String};Then use
Addressinstead ofsoroban_sdk::Addressthroughout the file.
17-21: Consider renaming for clarity.The struct name
DisEscdoesn't clearly indicate this is for release operations. Consider renaming toReleaseEscor similar to match the topic "tw_release" and improve code readability.-pub struct DisEsc { +pub struct ReleaseEsc { pub release_signer: soroban_sdk::Address, }Timebased Escrows/contracts/escrow/test_snapshots/tests/test/test_release_funds_same_receiver_as_provider.1.json (1)
382-405: Release path (no memo) is encoded correctlyflags.released=true and withdrawal_state remains zeroed, aligning with single-step release. Consider adding an event snapshot entry (if your harness supports it) to assert fees and receiver amounts for stronger regression protection.
Timebased Escrows/contracts/escrow/src/core/escrow.rs (4)
28-41: Funding controls: prevent over/under-funding and redundant auth
- Currently any positive amount is accepted if expected_escrow matches stored_escrow. This allows multiple top-ups and totals ≠ escrow.amount. Add a check that current_balance + amount == stored_escrow.amount (or track total_funded) to prevent accidental misfunding.
- Token transfer will enforce authorization from signer; local signer.require_auth() is redundant and costs budget. Safe to drop unless you rely on it for an extra policy.
Example check:
let bal = token_client.balance(&e.current_contract_address()); if bal + amount != stored_escrow.amount { return Err(ContractError::AmountMismatch); }Requires a new error variant (AmountMismatch).
154-176: Tighten withdraw_with_memo semantics and remove unused arg
- _contract_id is unused. Remove it to avoid confusion.
- Optionally require the caller to present the expected memo to reduce accidental invocation drift and assert intent, even if the chain cannot read L1 transaction memos.
Minimal clean-up:
- pub fn withdraw_with_memo( - e: &Env, - _contract_id: &Address, - ) -> Result<(), ContractError> { + pub fn withdraw_with_memo(e: &Env) -> Result<(), ContractError> {Stronger intent check (optional):
- pub fn withdraw_with_memo(e: &Env) -> Result<(), ContractError> { + pub fn withdraw_with_memo(e: &Env, expected_memo: soroban_sdk::String) -> Result<(), ContractError> { let mut escrow = Self::get_escrow(e)?; validate_withdrawal_conditions(&escrow)?; + if escrow.receiver_memo != expected_memo { + return Err(ContractError::InvalidMemo); + }Would also need to update contract.rs withdraw_with_memo entrypoint and add ContractError::InvalidMemo.
112-138: Balance queries: consider early return for empty set and per-address error isolationFor addresses.len()==0, return early to save budget. Also consider handling failures per address (e.g., missing Escrow on a foreign contract) to return partial results instead of reverting the whole call.
140-145: Cross-contract call surfaceinvoke_contract expects target to expose get_escrow with no args. If that surface changes, this path will panic. Optional: wrap in a Result-returning helper that maps failures to ContractError::EscrowNotFound for nicer diagnostics.
Timebased Escrows/contracts/escrow/src/tests/test.rs (4)
35-37: Typo in test name.Rename to test_initialize_escrow for clarity and grepability.
-#[test] -fn test_initialize_excrow() { +#[test] +fn test_initialize_escrow() {
434-435: Redundant mint to approver (not used).You later mint to the contract (Lines 496–499) before release. Drop this mint to reduce noise and keep balances deterministic.
- usdc_token.1.mint(&approver_address, &(amount as i128));
741-758: Test name suggests “fallback” but flow uses a valid separate receiver.Either rename the test or add an actual fallback scenario (e.g., simulate invalid/unresolvable receiver so funds default to service_provider).
Option A (rename):
-#[test] -fn test_release_funds_invalid_receiver_fallback() { +#[test] +fn test_release_funds_different_receiver() {Option B (add fallback case): I can draft a companion test that sets an invalid receiver and asserts funds route to service_provider. Want me to add it?
862-951: Missing negative test: withdraw_with_memo must be blocked when disputed.Add a test that: initializes → funds → releases with a non-empty receiver_memo → opens a dispute → attempts withdraw_with_memo → expects Err(EscrowOpenedForDisputeResolution).
I can provide a ready-to-run test function wired to your helpers. Want me to push it?
Timebased Escrows/contracts/escrow/src/error.rs (4)
14-18: Fix “Milstone” typos in variant names.Prefer consistent “Milestone” spelling to avoid long-term API confusion. This is a new file in this PR, so renaming now is low-risk.
- OnlyServiceProviderChangeMilstoneStatus = 8, - InvalidMileStoneIndex = 10, - OnlyApproverChangeMilstoneFlag = 11, + OnlyServiceProviderChangeMilestoneStatus = 8, + InvalidMilestoneIndex = 10, + OnlyApproverChangeMilestoneFlag = 11,Remember to update all references and Display arms accordingly.
4-6: Derive Eq for stronger equality semantics.Enum already derives PartialEq; deriving Eq is idiomatic and harmless.
-#[derive(Debug, Copy, Clone, PartialEq)] +#[derive(Debug, Copy, Clone, PartialEq, Eq)]
35-46: Non-sequential discriminants are okay but can confuse diagnostics.If there’s no on-chain compatibility constraint yet, consider normalizing to a sequential, grouped layout (withdrawal/dispute/milestone groups) for maintainability. Keep unique codes stable once shipped.
126-131: Polish wording in messages.Use “less than or equal to 0” for consistency.
- "None of the amounts to be transferred should be less or equal than 0." + "None of the amounts to be transferred should be less than or equal to 0." - "The funds of the approver or receiver must not be less or equal than 0." + "The funds of the approver or receiver must not be less than or equal to 0."Also applies to: 156-161
Timebased Escrows/contracts/escrow/src/core/validators/escrow.rs (2)
102-109: Avoid magic numbers: extract MAX_MILESTONES.Improves readability and single-source-of-truth.
-use soroban_sdk::{Address, Env}; +use soroban_sdk::{Address, Env}; + +const MAX_MILESTONES: usize = 10; ... - if new_escrow.milestones.len() > 10 { + if new_escrow.milestones.len() > MAX_MILESTONES { return Err(ContractError::TooManyMilestones); } ... - if escrow_properties.milestones.len() > 10 { + if escrow_properties.milestones.len() > MAX_MILESTONES { return Err(ContractError::TooManyMilestones); }Also applies to: 143-148
150-165: Funding equality check is strict; confirm intended UX.Requiring full struct equality with expected_escrow can make benign metadata edits (e.g., title) block funding. If that’s desired, fine; otherwise, compare only invariant fields (engagement_id, trustline, roles, amount, platform_fee).
I can propose a targeted comparator if you confirm which fields must be immutable at funding time.
Timebased Escrows/contracts/escrow/src/storage/types.rs (5)
11-11: Clarify fee units and consider renaming for intent.platform_fee as u32 is fine, but the unit isn’t clear (percent vs basis points). Prefer basis points and an explicit name to prevent misinterpretation.
Example:
- pub platform_fee: u32, + /// Platform fee in basis points (1% = 100 bps) + pub platform_fee_bps: u32,
15-16: Model memo presence with Option instead of sentinel empty String.Empty strings as “no memo” are error‑prone and complicate validation. Using Option makes invariants explicit and simplifies checks.
- pub receiver_memo: String, + pub receiver_memo: Option<String>,Follow‑ups:
- Update validators to require Some(non‑empty) for the staged flow.
- Adjust event emission to unwrap safely or default to "" only for logging.
21-26: Use strong types for milestone status and optional evidence.Free‑form strings for status make state transitions hard to validate. Evidence may be absent.
-#[derive(Clone, Debug, PartialEq, Eq)] +#[derive(Clone, Debug, PartialEq, Eq)] pub struct Milestone { - pub description: String, - pub status: String, - pub evidence: String, + pub description: String, + pub status: MilestoneStatus, + pub evidence: Option<String>, pub approved: bool, } + +#[contracttype] +#[derive(Clone, Debug, PartialEq, Eq)] +pub enum MilestoneStatus { + Pending, + InProgress, + Completed, + Rejected, +}
62-66: State invariants for withdrawals should prevent negatives and signal completion.pending_receiver_amount should never be negative; fees_released should imply pending >= 0; after successful memo withdrawal, pending must be 0 and escrow.flags.released = true.
- Ensure validators enforce pending_receiver_amount >= 0 on every write.
- Confirm EscrowManager::withdraw_with_memo sets pending to 0 and flips flags.released = true.
- Consider deriving Default and documenting invariants:
-#[derive(Clone, Debug, PartialEq, Eq)] +#[derive(Clone, Debug, PartialEq, Eq, Default)] pub struct WithdrawalState { pub fees_released: bool, pub pending_receiver_amount: i128, }
55-59: Derive Eq/PartialEq for AddressBalance if used in tests/snapshots.This eases assertions and snapshot comparisons.
-#[derive(Clone)] +#[derive(Clone, PartialEq, Eq)] pub struct AddressBalance { ... }Timebased Escrows/contracts/escrow/src/contract.rs (4)
74-84: Emit amount from post‑state or return value to prevent drift.You snapshot escrow before the withdrawal, then emit event using that snapshot’s pending amount. If manager’s computation differs (e.g., due to rounding/fees or partial withdrawals in future), the event could be inaccurate.
Simplest: re-fetch after the call and emit the delta (pre.pending - post.pending).
- let escrow = EscrowManager::get_escrow(e)?; - EscrowManager::withdraw_with_memo(e, &e.current_contract_address())?; - WithdrawMemo { - amount: escrow.withdrawal_state.pending_receiver_amount, - receiver: escrow.roles.receiver, - memo: escrow.receiver_memo, - }.publish(e); + let pre = EscrowManager::get_escrow(e)?; + EscrowManager::withdraw_with_memo(e, &e.current_contract_address())?; + let post = EscrowManager::get_escrow(e)?; + let amount = pre.withdrawal_state.pending_receiver_amount + .saturating_sub(post.withdrawal_state.pending_receiver_amount); + WithdrawMemo { + amount, + receiver: post.roles.receiver, + memo: post.receiver_memo.unwrap_or_default(), + }.publish(e);
64-72: Event naming nit: DisEsc reads like “dispute.”Minor readability: release_funds emits DisEsc, which can be confused with disputes. Consider a more explicit event name (e.g., ReleaseInitiated).
156-172: Prefer &Env over Env by value for consistency and to avoid needless clones.Elsewhere methods take &Env; these two take Env and then pass &e. Align signatures.
- pub fn change_milestone_status( - e: Env, + pub fn change_milestone_status( + e: &Env, ... - ) -> Result<(), ContractError> { - let escrow = MilestoneManager::change_milestone_status( - &e, + ) -> Result<(), ContractError> { + let escrow = MilestoneManager::change_milestone_status( + e, ... )?; - MilestoneStatusChanged { escrow }.publish(&e); + MilestoneStatusChanged { escrow }.publish(e); Ok(()) }Apply similarly to approve_milestone.
Also applies to: 174-183
126-151: TTL extension: consider upper bounds and role gating in validators.You require platform auth, good. Add a max ledgers guard to prevent accidental huge TTL extension; optionally centralize this policy in validators.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (35)
Timebased Escrows/contracts/escrow/Cargo.toml(1 hunks)Timebased Escrows/contracts/escrow/src/contract.rs(1 hunks)Timebased Escrows/contracts/escrow/src/core/dispute.rs(1 hunks)Timebased Escrows/contracts/escrow/src/core/escrow.rs(1 hunks)Timebased Escrows/contracts/escrow/src/core/milestone.rs(1 hunks)Timebased Escrows/contracts/escrow/src/core/validators/dispute.rs(1 hunks)Timebased Escrows/contracts/escrow/src/core/validators/escrow.rs(1 hunks)Timebased Escrows/contracts/escrow/src/core/validators/milestone.rs(1 hunks)Timebased Escrows/contracts/escrow/src/error.rs(1 hunks)Timebased Escrows/contracts/escrow/src/events/handler.rs(1 hunks)Timebased Escrows/contracts/escrow/src/lib.rs(1 hunks)Timebased Escrows/contracts/escrow/src/modules/fee/calculator.rs(1 hunks)Timebased Escrows/contracts/escrow/src/modules/math/basic.rs(1 hunks)Timebased Escrows/contracts/escrow/src/modules/math/safe.rs(1 hunks)Timebased Escrows/contracts/escrow/src/storage/types.rs(1 hunks)Timebased Escrows/contracts/escrow/src/tests/mod.rs(1 hunks)Timebased Escrows/contracts/escrow/src/tests/test.rs(1 hunks)Timebased Escrows/contracts/escrow/src/tests/test/memo_withdrawal_test.rs(1 hunks)Timebased Escrows/contracts/escrow/test_snapshots/tests/test/memo_withdrawal_test/test/test_release_funds_with_memo.1.json(1 hunks)Timebased Escrows/contracts/escrow/test_snapshots/tests/test/memo_withdrawal_test/test/test_withdraw_with_empty_memo.1.json(1 hunks)Timebased Escrows/contracts/escrow/test_snapshots/tests/test/memo_withdrawal_test/test/test_withdraw_with_memo.1.json(1 hunks)Timebased Escrows/contracts/escrow/test_snapshots/tests/test/memo_withdrawal_test/test/test_withdraw_with_memo_before_release.1.json(1 hunks)Timebased Escrows/contracts/escrow/test_snapshots/tests/test/test_change_milestone_status_and_approved.1.json(1 hunks)Timebased Escrows/contracts/escrow/test_snapshots/tests/test/test_dispute_escrow_authorized_and_unauthorized.1.json(1 hunks)Timebased Escrows/contracts/escrow/test_snapshots/tests/test/test_dispute_management.1.json(1 hunks)Timebased Escrows/contracts/escrow/test_snapshots/tests/test/test_dispute_resolution_process.1.json(1 hunks)Timebased Escrows/contracts/escrow/test_snapshots/tests/test/test_fund_escrow_signer_insufficient_funds_error.1.json(1 hunks)Timebased Escrows/contracts/escrow/test_snapshots/tests/test/test_fund_escrow_successful_deposit.1.json(1 hunks)Timebased Escrows/contracts/escrow/test_snapshots/tests/test/test_get_multiple_escrow_balances_platform_authorized.1.json(1 hunks)Timebased Escrows/contracts/escrow/test_snapshots/tests/test/test_initialize_excrow.1.json(1 hunks)Timebased Escrows/contracts/escrow/test_snapshots/tests/test/test_release_funds_invalid_receiver_fallback.1.json(1 hunks)Timebased Escrows/contracts/escrow/test_snapshots/tests/test/test_release_funds_milestones_incomplete.1.json(1 hunks)Timebased Escrows/contracts/escrow/test_snapshots/tests/test/test_release_funds_same_receiver_as_provider.1.json(1 hunks)Timebased Escrows/contracts/escrow/test_snapshots/tests/test/test_release_funds_successful_flow.1.json(1 hunks)Timebased Escrows/contracts/escrow/test_snapshots/tests/test/test_update_escrow.1.json(1 hunks)
🧰 Additional context used
🧬 Code graph analysis (6)
Timebased Escrows/contracts/escrow/src/tests/test/memo_withdrawal_test.rs (3)
Timebased Escrows/contracts/escrow/src/tests/test.rs (1)
create_usdc_token(16-22)Timebased Escrows/contracts/escrow/src/core/escrow.rs (5)
e(144-144)initialize_escrow(20-26)release_funds(43-88)get_escrow(147-152)withdraw_with_memo(154-177)Timebased Escrows/contracts/escrow/src/contract.rs (5)
initialize_escrow(44-51)approve_milestone(174-183)release_funds(64-72)get_escrow(104-106)withdraw_with_memo(74-84)
Timebased Escrows/contracts/escrow/src/core/milestone.rs (3)
Timebased Escrows/contracts/escrow/src/core/validators/milestone.rs (2)
validate_milestone_flag_change_conditions(25-47)validate_milestone_status_change_conditions(9-22)Timebased Escrows/contracts/escrow/src/contract.rs (2)
change_milestone_status(156-172)get_escrow(104-106)Timebased Escrows/contracts/escrow/src/core/escrow.rs (2)
e(144-144)get_escrow(147-152)
Timebased Escrows/contracts/escrow/src/contract.rs (3)
Timebased Escrows/contracts/escrow/src/core/escrow.rs (8)
initialize_escrow(20-26)e(144-144)fund_escrow(28-41)release_funds(43-88)get_escrow(147-152)change_escrow_properties(89-110)get_escrow_by_contract_id(140-145)get_multiple_escrow_balances(112-138)Timebased Escrows/contracts/escrow/src/core/milestone.rs (2)
change_milestone_status(13-44)change_milestone_approved_flag(46-74)Timebased Escrows/contracts/escrow/src/core/dispute.rs (2)
resolve_dispute(19-82)dispute_escrow(84-93)
Timebased Escrows/contracts/escrow/src/tests/test.rs (1)
Timebased Escrows/contracts/escrow/src/core/escrow.rs (1)
e(144-144)
Timebased Escrows/contracts/escrow/src/modules/fee/calculator.rs (2)
Timebased Escrows/contracts/escrow/src/modules/math/safe.rs (2)
safe_mul_div(6-6)safe_mul_div(10-16)Timebased Escrows/contracts/escrow/src/modules/math/basic.rs (2)
safe_sub(7-7)safe_sub(15-17)
Timebased Escrows/contracts/escrow/src/core/escrow.rs (3)
Timebased Escrows/contracts/escrow/src/core/validators/escrow.rs (5)
validate_escrow_property_change_conditions(62-111)validate_fund_escrow_conditions(151-165)validate_initialize_escrow_conditions(114-148)validate_release_conditions(30-59)validate_withdrawal_conditions(9-27)Timebased Escrows/contracts/escrow/src/contract.rs (7)
initialize_escrow(44-51)fund_escrow(53-62)get_escrow(104-106)release_funds(64-72)get_multiple_escrow_balances(115-120)get_escrow_by_contract_id(108-113)withdraw_with_memo(74-84)Timebased Escrows/contracts/escrow/src/modules/fee/calculator.rs (2)
calculate_standard_fees(20-23)calculate_standard_fees(30-50)
🔇 Additional comments (25)
Timebased Escrows/contracts/escrow/src/modules/math/safe.rs (1)
10-15: Safe multiply/divide helper looks solid. The checked multiply/divide chain cleanly maps overflow and division failures into the contract’s error surface—nice and concise.Timebased Escrows/contracts/escrow/test_snapshots/tests/test/memo_withdrawal_test/test/test_release_funds_with_memo.1.json (1)
86-89: TTL sanity checkmin_persistent_entry_ttl is 4096 (Lines 86-89) yet some persistent entries show 4095 (Lines 419-420). This might be harmless in the snapshot harness, but please confirm it aligns with Soroban TTL rules to avoid brittle tests.
Also applies to: 419-420
Timebased Escrows/contracts/escrow/test_snapshots/tests/test/test_fund_escrow_successful_deposit.1.json (1)
55-324: Nice end‑to‑end deposit coverage; consider adding an explicit invariant checkThe sequence funds twice with transfer sub‑invocations; final balances reflect 0 at signer and 100,000,000 at escrow. Looks good. Consider adding a test assertion that sum(Balance.amount) across participants equals the minted total for the asset in this scenario, to catch future regressions in transfer paths.
Also applies to: 326-599, 1046-1112, 1115-1183
Timebased Escrows/contracts/escrow/test_snapshots/tests/test/test_dispute_escrow_authorized_and_unauthorized.1.json (2)
392-414: Snapshot structure LGTM; includes new withdrawal_state fields
fees_releasedandpending_receiver_amountpresence matches staged withdrawals state. No blockers.If feasible, add one assertion in the test that inspects these fields after dispute flows to guard against regressions.
Also applies to: 668-685
781-787: Confirm asset code representation “aaa\0” matches test harness expectationsSome harnesses expect null‑padded asset codes; others expect raw string “aaa”. Please confirm this exact encoding is required here to avoid fragile diffs across tool versions.
Timebased Escrows/contracts/escrow/src/core/dispute.rs (1)
77-81: State flags look consistent; confirm ifreleasedneeds reset.After resolution you set
resolved=trueanddisputed=false. Ifreleasedinteracts with post‑dispute flows, ensure it matches your state machine expectations.Timebased Escrows/contracts/escrow/test_snapshots/tests/test/test_release_funds_successful_flow.1.json (1)
1171-1174: No events captured; ensure event emission is tested elsewhere.Given the contract emits events for releases, consider adding a small event snapshot/assert to guard regressions.
Timebased Escrows/contracts/escrow/src/core/validators/dispute.rs (1)
8-36: LGTM: resolution preconditions are precise.The equality check (
total == current_balance) plus positivity guarantees clean invariants for the dispute flow.Timebased Escrows/contracts/escrow/test_snapshots/tests/test/test_dispute_resolution_process.1.json (1)
1-1107: Test snapshot file looks appropriate for dispute resolution testing.This JSON test snapshot captures the expected ledger state after a dispute resolution process, including proper flag updates (resolved=true) and balance distributions. The structure is consistent with Soroban contract test patterns.
Timebased Escrows/contracts/escrow/test_snapshots/tests/test/test_get_multiple_escrow_balances_platform_authorized.1.json (1)
1-1033: Test snapshot appropriately validates multiple escrow balance functionality.This JSON snapshot captures the expected state for testing multiple escrow balance retrieval with platform authorization. The data structure is consistent and includes proper authorization sequences and ledger entries for multiple escrow instances.
Timebased Escrows/contracts/escrow/src/events/handler.rs (4)
4-8: LGTM!The
InitEscevent structure is well-defined with appropriate contractevent annotation and topic naming.
10-15: LGTM!The
FundEscevent captures the essential information for funding operations with appropriate field types.
23-28: LGTM!The
ChgEscevent structure appropriately captures platform and engagement updates.
30-71: LGTM!All remaining event structures are well-defined and appropriate for their respective use cases. The
WithdrawMemoevent particularly aligns well with the PR's memo-based withdrawal objectives, capturing the essential information (amount, receiver, memo) for this functionality.Timebased Escrows/contracts/escrow/test_snapshots/tests/test/test_update_escrow.1.json (1)
1-935: Test snapshot appropriately validates escrow update functionality.This JSON snapshot captures the expected ledger state after an escrow update operation, including updated amounts, descriptions, milestones, and platform fees. The structure is consistent with Soroban contract test patterns.
Timebased Escrows/contracts/escrow/src/modules/fee/calculator.rs (5)
1-7: LGTM!The imports are well-organized and include all necessary dependencies for fee calculation operations.
9-10: LGTM!The fee constants are appropriately set with 30 basis points (0.30%) for Trustless Work fees and the standard 10,000 basis points denominator.
12-17: LGTM!The
StandardFeeResultstruct is well-designed with appropriate field types and derives. The public fields provide easy access to the calculated fee components.
19-24: LGTM!The
FeeCalculatorTraithas a clean, focused interface with appropriate error handling and parameter types.
29-51: LGTM! Fee calculation logic is mathematically sound.The implementation correctly calculates fees using the standard basis points formula. 30 basis points equals 0.30%, and the calculation properly computes percentage-based fees on transaction amounts. The sequence is appropriate:
- Calculate Trustless Work fee (30 bps = 0.30%)
- Calculate platform fee (variable bps)
- Subtract both fees from total to get receiver amount
The use of checked arithmetic (
SafeMathandBasicMath) properly handles overflow/underflow scenarios and maintains precision.Timebased Escrows/contracts/escrow/test_snapshots/tests/test/test_change_milestone_status_and_approved.1.json (1)
313-353: Milestone approved state looks consistent with status transitionApproved=true with status="completed" and updated evidence is coherent. No issues spotted.
Timebased Escrows/contracts/escrow/test_snapshots/tests/test/memo_withdrawal_test/test/test_withdraw_with_memo_before_release.1.json (1)
392-642: Snapshot intention vs. contentTest name implies “withdraw before release” should fail, but this snapshot records only initialization with failed_call=false. Please confirm whether this file is the precondition snapshot; if not, capture the failing invocation with failed_call=true and pertinent error (FeesNotReleasedYet).
Timebased Escrows/contracts/escrow/test_snapshots/tests/test/test_dispute_management.1.json (1)
270-306: Dispute flag gatingflags.disputed=true with milestones unapproved matches validator expectations that block release. Snapshot looks coherent.
Timebased Escrows/contracts/escrow/src/tests/test.rs (1)
496-539: Post-release contract balance assertions are good.These checks guard against partial state leaks after distribution.
If staged (memo) release paths are added elsewhere, ensure similar “contract balance” assertions exist for the two-step flow (fees-only release then final withdrawal).
Also applies to: 707-739, 848-860
Timebased Escrows/contracts/escrow/src/contract.rs (1)
27-36: Who is allowed to call deploy? Reconfirm intended auth boundary.Current logic allows anyone to deploy if they authorize as the provided deployer. Is that intentional, or should this be restricted to the platform/admin?
If restriction is intended, verify against escrow.roles.platform_address or an admin DataKey, and fail otherwise. Otherwise, add a brief doc comment clarifying that deploy is a generic utility.
| pub fn withdraw_with_memo(e: &Env) -> Result<(), ContractError> { | ||
| let escrow = EscrowManager::get_escrow(e)?; | ||
| EscrowManager::withdraw_with_memo(e, &e.current_contract_address())?; | ||
| WithdrawMemo { | ||
| amount: escrow.withdrawal_state.pending_receiver_amount, | ||
| receiver: escrow.roles.receiver, | ||
| memo: escrow.receiver_memo, | ||
| } | ||
| .publish(e); | ||
| Ok(()) | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion | 🟠 Major
🧩 Analysis chain
Memo verification path unclear; function appears permissionless.
-
Where is the check that the transaction’s memo matches the stored receiver memo? The contract method doesn’t accept a memo parameter and I don’t see a call that reads an L1 memo from Env.
-
If on-chain memo access isn’t supported, the contract cannot verify memos; enforcement must live in the client/backend. In that case, update docs/tests to reflect “memo required by client, not verified on-chain.”
-
Authorization: anyone can call withdraw_with_memo here. Confirm EscrowManager::withdraw_with_memo enforces:
- fees_released == true,
- pending_receiver_amount > 0,
- receiver_memo.is_some() && not empty,
- idempotency (subsequent calls revert),
- sets flags.released = true and pending to 0 after transfer.
If not already enforced, add these checks in the manager/validators.
🌐 Web query:
Can a Soroban contract access the Stellar transaction memo (e.g., via soroban_sdk::Env) to verify it on-chain?
If yes, what API is used and in which Soroban protocol/sdk version?
💡 Result:
Short answer: No — a Soroban contract cannot directly read the Stellar transaction memo from env (there is no Env API that returns the transaction memo). The memo can be used in Soroban authorization credentials (CAP‑0064 adds a txMemo field for SorobanAddressCredentialsV2 and the host validates it), but that memo-checking happens in the host/authorization layer, not by the contract code via soroban_sdk::Env. [1][2][3]
Sources
- Soroban Env / SDK docs (Env API shows ledger/auth context access but no tx memo accessor). [1]
- CAP‑0064 (adds txMemo to Soroban credentials; host validates tx memo as part of authorization). [2]
- Relayer / docs note: memos are not supported on Soroban contract operations (attempting to include a memo with invoke_contract/create_contract/upload_wasm results in an error). [3]
References
- soroban_sdk Env documentation (API listing). [https://docs.rs/soroban-sdk/latest/soroban_sdk/struct.Env.html]
- CAP‑0064 in stellar-protocol (txMemo in SorobanAddressCredentialsV2). [https://github.com/stellar/stellar-protocol/blob/master/core/cap-0064.md]
- Relayer / docs statement that memos are not supported for Soroban contract operations. [https://docs.openzeppelin.com/relayer/1.1.x/stellar]
---
🏁 Script executed:
```shell
#!/bin/bash
set -eux
# locate contract.rs
find . -type f -name 'contract.rs'
# locate EscrowManager definitions
rg -nP 'struct\s+EscrowManager\b' -g '*.rs'
rg -nP 'impl\s+EscrowManager\b' -g '*.rs'
# locate withdraw_with_memo definition
rg -nP 'fn\s+withdraw_with_memo\b' -g '*.rs'
Length of output: 1055
🏁 Script executed:
#!/bin/bash
# Extract EscrowManager::withdraw_with_memo implementation for inspection
sed -n '140,260p' "Timebased Escrows/contracts/escrow/src/core/escrow.rs"Length of output: 1215
🏁 Script executed:
#!/bin/bash
# Locate and show context around validate_withdrawal_conditions definition
rg -nP -C5 'fn\s+validate_withdrawal_conditions' -g '*.rs'Length of output: 1057
🏁 Script executed:
#!/bin/bash
sed -n '1,200p' "Timebased Escrows/contracts/escrow/src/core/validators/escrow.rs"Length of output: 4321
Cannot verify tx memo on-chain; enforce off-chain and restrict withdrawal to receiver
Soroban Env has no API for reading the Stellar transaction memo—memo validation must happen in your client/backend and update docs/tests to reflect “memo required off-chain, not verified on-chain.” The existing validate_withdrawal_conditions already enforces fees_released, pending_receiver_amount > 0, non-empty stored memo, idempotency, and state reset. To ensure only the designated receiver can trigger withdrawal, add an Env::caller() == escrow.roles.receiver check in withdraw_with_memo.
| pub fn update_escrow( | ||
| e: &Env, | ||
| plataform_address: Address, | ||
| escrow_properties: Escrow, | ||
| ) -> Result<Escrow, ContractError> { | ||
| let updated_escrow = EscrowManager::change_escrow_properties( | ||
| e, | ||
| &plataform_address, | ||
| escrow_properties.clone(), | ||
| )?; | ||
| ChgEsc { | ||
| platform: plataform_address, | ||
| engagement_id: escrow_properties.engagement_id.clone(), | ||
| } | ||
| .publish(e); | ||
| Ok(updated_escrow) | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion | 🟠 Major
🧩 Analysis chain
Typo: plataform_address → platform_address (public API).
Public API misspelling propagates across clients/tests.
- pub fn update_escrow(
- e: &Env,
- plataform_address: Address,
- escrow_properties: Escrow,
- ) -> Result<Escrow, ContractError> {
- let updated_escrow = EscrowManager::change_escrow_properties(
- e,
- &plataform_address,
- escrow_properties.clone(),
- )?;
- ChgEsc {
- platform: plataform_address,
+ pub fn update_escrow(
+ e: &Env,
+ platform_address: Address,
+ escrow_properties: Escrow,
+ ) -> Result<Escrow, ContractError> {
+ let updated_escrow = EscrowManager::change_escrow_properties(
+ e,
+ &platform_address,
+ escrow_properties.clone(),
+ )?;
+ ChgEsc {
+ platform: platform_address,
engagement_id: escrow_properties.engagement_id.clone(),
}
.publish(e);
Ok(updated_escrow)
}Run to find other occurrences:
🏁 Script executed:
#!/bin/bash
rg -nP 'plataform_address|Plataform' -C2Length of output: 6034
Rename all occurrences of plataform_address to platform_address across public APIs and core modules
- Misspelling appears in:
Timebased Escrows/contracts/escrow/src/contract.rs(update_escrow)Timebased Escrows/contracts/engagement/src/contract.rs(change_escrow_properties)Timebased Escrows/contracts/engagement/src/core/escrow.rs(parameter, comparisons, auth)- Identical occurrences in
Price-Oracle-Contract/contracts/engagement/...
- This is a breaking change to your public API—update all function signatures, call sites, events, client code and tests in both repositories.
- Use an automated rename/refactor tool or IDE to ensure consistency and avoid missing any references.
🤖 Prompt for AI Agents
In Timebased Escrows/contracts/escrow/src/contract.rs around lines 86 to 102,
the identifier `plataform_address` is misspelled and must be renamed to
`platform_address` across public APIs and core modules; update the function
signature, parameter name, local references, event fields, and any call sites to
use `platform_address`, then propagate the same rename to Timebased
Escrows/contracts/engagement/src/contract.rs, Timebased
Escrows/contracts/engagement/src/core/escrow.rs and the identical locations in
Price-Oracle-Contract (including function signatures, comparisons, auth checks,
event structs, clients and tests); perform the change with an automated
rename/refactor tool or IDE to ensure all references (including public API docs,
exported symbols, and tests) are updated consistently, run compilation and
tests, and adjust any external client code to match the new API name.
| use crate::modules::{ | ||
| fee::{FeeCalculator, FeeCalculatorTrait}, | ||
| math::{BasicArithmetic, BasicMath}, | ||
| }; |
There was a problem hiding this comment.
Fix rounding/overdraft and overflow in fee pro‑rata distribution.
- Problem: fee_share uses integer division; sum(fee_shares) ≤ total_fees. Because fees are transferred first, sum(net_amounts) can exceed the remaining balance, causing a transfer to fail (insufficient balance) or leave dust if order changes.
- Problem:
amount * total_feescan overflow i128 for large balances. - Fix: Distribute from the net pot (receiver_amount) using SafeMath::safe_mul_div, track assigned sum, and send the deterministic remainder to the last beneficiary. This guarantees fees + net == total without overflow.
Apply:
@@
-use crate::modules::{
- fee::{FeeCalculator, FeeCalculatorTrait},
- math::{BasicArithmetic, BasicMath},
-};
+use crate::modules::{
+ fee::{FeeCalculator, FeeCalculatorTrait},
+ math::{BasicArithmetic, BasicMath, SafeMath},
+};
@@
- let fee_result = FeeCalculator::calculate_standard_fees(total, escrow.platform_fee)?;
- let total_fees =
- BasicMath::safe_add(fee_result.trustless_work_fee, fee_result.platform_fee)?;
+ let fee_result = FeeCalculator::calculate_standard_fees(total, escrow.platform_fee)?;
+ let total_fees =
+ BasicMath::safe_add(fee_result.trustless_work_fee, fee_result.platform_fee)?;
+ let net_pot = fee_result.receiver_amount;
@@
- for (addr, amount) in distributions.iter() {
- if amount <= 0 {
- continue;
- }
- let fee_share = (amount * (total_fees as i128)) / total;
- let net_amount = amount - fee_share;
- if net_amount > 0 {
- token_client.transfer(&contract_address, &addr, &net_amount);
- }
- }
+ // Pro‑rata net distribution with deterministic remainder allocation
+ let mut assigned: i128 = 0;
+ let mut last_addr: Option<Address> = None;
+ for (addr, amount) in distributions.iter() {
+ if amount <= 0 {
+ continue;
+ }
+ last_addr = Some(addr.clone());
+ let share = SafeMath::safe_mul_div(amount, net_pot, total)?;
+ if share > 0 {
+ token_client.transfer(&contract_address, &addr, &share);
+ assigned = BasicMath::safe_add(assigned, share)?;
+ }
+ }
+ let remainder = BasicMath::safe_sub(net_pot, assigned)?;
+ if remainder > 0 {
+ if let Some(addr) = last_addr {
+ token_client.transfer(&contract_address, &addr, &remainder);
+ }
+ }Rationale anchored by existing primitives (SafeMath::safe_mul_div, BasicMath). Based on provided snippets.
Also applies to: 47-50, 66-75
🤖 Prompt for AI Agents
In Timebased Escrows/contracts/escrow/src/core/dispute.rs around lines 6-9 (and
also apply same change to ranges 47-50 and 66-75), fix the fee pro‑rata
distribution by computing fee shares from the net pot (receiver_amount) rather
than gross amount and by using SafeMath::safe_mul_div to avoid i128 overflow;
compute each beneficiary's fee_share = SafeMath::safe_mul_div(receiver_amount,
beneficiary_weight, total_weight), accumulate assigned_sum and for the final
beneficiary set fee_share = receiver_amount - assigned_sum to absorb any
rounding remainder so that sum(fee_shares) + sum(net_amounts) == total, and
replace any direct amount * total_fees multiplication with safe_mul_div calls
and bounds-checked arithmetic to prevent overdraft and overflow.
| if fee_result.trustless_work_fee > 0 { | ||
| token_client.transfer( | ||
| &contract_address, | ||
| &trustless_work_address, | ||
| &fee_result.trustless_work_fee, | ||
| ); | ||
| } |
There was a problem hiding this comment.
🧩 Analysis chain
Validate the trustless_work fee recipient; do not take it from the caller.
trustless_work_address is supplied by the invoker and not validated against a canonical/configured address. A (compromised) dispute_resolver could redirect TW fees. Persist this address at init (e.g., in instance storage/roles) and enforce equality here, or hardcode via contract config.
Follow‑up: If you already store TW address elsewhere, please wire it here and reject mismatches.
Would you like me to draft a storage key (e.g., DataKey::TrustlessWorkAddress) change and migration?
🏁 Script executed:
#!/bin/bash
# Locate and display dispute.rs context
FILE=$(fd --hidden 'dispute.rs' -t f | head -n1)
sed -n '1,100p' "$FILE"
# Search all occurrences of trustless_work_address
rg -n 'trustless_work_address' -C3 .Length of output: 46561
🏁 Script executed:
rg -n "enum DataKey" -C3 "Timebased Escrows/contracts/escrow/src/storage/types.rs"Length of output: 98
🏁 Script executed:
#!/bin/bash
rg -n "pub struct Escrow" -C3 "Timebased Escrows/contracts/escrow/src/core/escrow.rs"Length of output: 206
Enforce a configured trustless_work_address rather than using the caller’s value.
Define a storage key (e.g. DataKey::TrustlessWorkAddress), persist the canonical TW address at init or via a setter, and in resolve_dispute reject calls where the passed trustless_work_address does not equal the stored value.
| let mut escrow = Self::get_escrow(e)?; | ||
| validate_release_conditions(&escrow, release_signer)?; | ||
|
|
||
| let contract_address = e.current_contract_address(); | ||
| let token_client = TokenClient::new(e, &escrow.trustline.address); | ||
|
|
||
| if token_client.balance(&contract_address) < escrow.amount { | ||
| return Err(ContractError::EscrowBalanceNotEnoughToSendEarnings); | ||
| } | ||
|
|
There was a problem hiding this comment.
Add explicit guard to prevent re-running staged releases
After a staged release (memo set + fees_released=true), calling release_funds again will currently fail only due to balance check; if the contract is re-funded to ≥ amount, it would double‑pay fees. Add a state guard to block this path.
let mut escrow = Self::get_escrow(e)?;
- validate_release_conditions(&escrow, release_signer)?;
+ validate_release_conditions(&escrow, release_signer)?;
+ // Prevent double fee release when memo flow has already been staged
+ if !escrow.receiver_memo.is_empty() && escrow.withdrawal_state.fees_released {
+ // Consider introducing a dedicated error like `FeesAlreadyReleased`
+ return Err(ContractError::FeesAlreadyReleased);
+ }Note: requires adding ContractError::FeesAlreadyReleased in error.rs and mapping in validators.
🤖 Prompt for AI Agents
In Timebased Escrows/contracts/escrow/src/core/escrow.rs around lines 50 to 59,
add an explicit guard that checks the escrow state (e.g., escrow.fees_released)
before proceeding with release_funds so staged releases cannot be re-run and
accidentally double‑pay if the contract is later refilled; if fees_released is
true, return a new ContractError::FeesAlreadyReleased. Also add the
FeesAlreadyReleased variant to error.rs and update any validator mappings to
translate this new error into the appropriate validation result/status.
| let mut milestone_to_update = existing_escrow | ||
| .milestones | ||
| .get(milestone_index as u32) | ||
| .ok_or(ContractError::InvalidMileStoneIndex)?; | ||
|
|
||
| if let Some(evidence) = new_evidence { | ||
| milestone_to_update.evidence = evidence; | ||
| } | ||
|
|
||
| milestone_to_update.status = new_status; | ||
|
|
||
| existing_escrow | ||
| .milestones | ||
| .set(milestone_index as u32, milestone_to_update); |
There was a problem hiding this comment.
Fix potential index wrap leading to wrong milestone updates
Casting i128 to u32 with as wraps values; a huge positive index (≥2^32) maps to a valid small u32, letting callers mutate the wrong milestone instead of erroring.
Apply this diff to validate the index:
- let mut milestone_to_update = existing_escrow
- .milestones
- .get(milestone_index as u32)
- .ok_or(ContractError::InvalidMileStoneIndex)?;
+ let idx = u32::try_from(milestone_index)
+ .map_err(|_| ContractError::InvalidMileStoneIndex)?;
+ let mut milestone_to_update = existing_escrow
+ .milestones
+ .get(idx)
+ .ok_or(ContractError::InvalidMileStoneIndex)?;📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| let mut milestone_to_update = existing_escrow | |
| .milestones | |
| .get(milestone_index as u32) | |
| .ok_or(ContractError::InvalidMileStoneIndex)?; | |
| if let Some(evidence) = new_evidence { | |
| milestone_to_update.evidence = evidence; | |
| } | |
| milestone_to_update.status = new_status; | |
| existing_escrow | |
| .milestones | |
| .set(milestone_index as u32, milestone_to_update); | |
| // validate that milestone_index fits in u32, error if not | |
| let idx = u32::try_from(milestone_index) | |
| .map_err(|_| ContractError::InvalidMileStoneIndex)?; | |
| let mut milestone_to_update = existing_escrow | |
| .milestones | |
| .get(idx) | |
| .ok_or(ContractError::InvalidMileStoneIndex)?; | |
| if let Some(evidence) = new_evidence { | |
| milestone_to_update.evidence = evidence; | |
| } | |
| milestone_to_update.status = new_status; | |
| existing_escrow | |
| .milestones | |
| .set(milestone_index as u32, milestone_to_update); |
| #[inline] | ||
| pub(crate) fn validate_withdrawal_conditions(escrow: &Escrow) -> Result<(), ContractError> { | ||
| if escrow.flags.released { | ||
| return Err(ContractError::EscrowAlreadyReleased); | ||
| } | ||
|
|
||
| if !escrow.withdrawal_state.fees_released { | ||
| return Err(ContractError::FeesNotReleasedYet); | ||
| } | ||
|
|
||
| if escrow.withdrawal_state.pending_receiver_amount == 0 { | ||
| return Err(ContractError::NoPendingWithdrawal); | ||
| } | ||
|
|
||
| if escrow.receiver_memo.is_empty() { | ||
| return Err(ContractError::NoMemoProvided); | ||
| } | ||
|
|
||
| Ok(()) | ||
| } |
There was a problem hiding this comment.
Missing dispute/resolution guards in withdrawal validator.
Allowing withdraw_with_memo during dispute or after resolution is a logic/safety gap. Block withdrawals in these states.
pub(crate) fn validate_withdrawal_conditions(escrow: &Escrow) -> Result<(), ContractError> {
if escrow.flags.released {
return Err(ContractError::EscrowAlreadyReleased);
}
+ // Disallow withdrawals while disputed or after resolution.
+ if escrow.flags.disputed {
+ return Err(ContractError::EscrowOpenedForDisputeResolution);
+ }
+ if escrow.flags.resolved {
+ return Err(ContractError::EscrowAlreadyResolved);
+ }
+
if !escrow.withdrawal_state.fees_released {
return Err(ContractError::FeesNotReleasedYet);
}
if escrow.withdrawal_state.pending_receiver_amount == 0 {
return Err(ContractError::NoPendingWithdrawal);
}
if escrow.receiver_memo.is_empty() {
return Err(ContractError::NoMemoProvided);
}
Ok(())
}Add a companion negative test as suggested in tests.
📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| #[inline] | |
| pub(crate) fn validate_withdrawal_conditions(escrow: &Escrow) -> Result<(), ContractError> { | |
| if escrow.flags.released { | |
| return Err(ContractError::EscrowAlreadyReleased); | |
| } | |
| if !escrow.withdrawal_state.fees_released { | |
| return Err(ContractError::FeesNotReleasedYet); | |
| } | |
| if escrow.withdrawal_state.pending_receiver_amount == 0 { | |
| return Err(ContractError::NoPendingWithdrawal); | |
| } | |
| if escrow.receiver_memo.is_empty() { | |
| return Err(ContractError::NoMemoProvided); | |
| } | |
| Ok(()) | |
| } | |
| #[inline] | |
| pub(crate) fn validate_withdrawal_conditions(escrow: &Escrow) -> Result<(), ContractError> { | |
| if escrow.flags.released { | |
| return Err(ContractError::EscrowAlreadyReleased); | |
| } | |
| // Disallow withdrawals while disputed or after resolution. | |
| if escrow.flags.disputed { | |
| return Err(ContractError::EscrowOpenedForDisputeResolution); | |
| } | |
| if escrow.flags.resolved { | |
| return Err(ContractError::EscrowAlreadyResolved); | |
| } | |
| if !escrow.withdrawal_state.fees_released { | |
| return Err(ContractError::FeesNotReleasedYet); | |
| } | |
| if escrow.withdrawal_state.pending_receiver_amount == 0 { | |
| return Err(ContractError::NoPendingWithdrawal); | |
| } | |
| if escrow.receiver_memo.is_empty() { | |
| return Err(ContractError::NoMemoProvided); | |
| } | |
| Ok(()) | |
| } |
🤖 Prompt for AI Agents
In Timebased Escrows/contracts/escrow/src/core/validators/escrow.rs around lines
8 to 27, the withdrawal validator currently misses guards for dispute and
resolution states; add checks that return appropriate ContractError when
escrow.flags.in_dispute is true (block withdrawals during dispute) and when
escrow.flags.resolved is true (block withdrawals after resolution), ensuring
these checks run before allowing a withdrawal; also add a companion negative
unit test that attempts withdraw_with_memo during dispute and after resolution
and asserts the validator rejects both cases.
| #[inline] | ||
| pub fn validate_release_conditions( | ||
| escrow: &Escrow, | ||
| release_signer: &Address, | ||
| ) -> Result<(), ContractError> { | ||
| if escrow.flags.released { | ||
| return Err(ContractError::EscrowAlreadyReleased); | ||
| } | ||
|
|
||
| if escrow.flags.resolved { | ||
| return Err(ContractError::EscrowAlreadyResolved); | ||
| } | ||
|
|
||
| if release_signer != &escrow.roles.release_signer { | ||
| return Err(ContractError::OnlyReleaseSignerCanReleaseEarnings); | ||
| } | ||
|
|
||
| if escrow.milestones.is_empty() { | ||
| return Err(ContractError::NoMilestoneDefined); | ||
| } | ||
|
|
||
| if !escrow.milestones.iter().all(|milestone| milestone.approved) { | ||
| return Err(ContractError::EscrowNotCompleted); | ||
| } | ||
|
|
||
| if escrow.flags.disputed { | ||
| return Err(ContractError::EscrowOpenedForDisputeResolution); | ||
| } | ||
|
|
||
| Ok(()) | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion | 🟠 Major
Prevent double “release” in staged (memo) flow.
Once fees are released (fees_released = true), a second call to release_funds should be rejected to avoid re-running commissions logic.
pub fn validate_release_conditions(
escrow: &Escrow,
release_signer: &Address,
) -> Result<(), ContractError> {
if escrow.flags.released {
return Err(ContractError::EscrowAlreadyReleased);
}
if escrow.flags.resolved {
return Err(ContractError::EscrowAlreadyResolved);
}
+ if escrow.withdrawal_state.fees_released {
+ // Staged release already performed; final payout must use withdraw_with_memo.
+ return Err(ContractError::EscrowAlreadyReleased);
+ }
+
if release_signer != &escrow.roles.release_signer {
return Err(ContractError::OnlyReleaseSignerCanReleaseEarnings);
}If you prefer a clearer error, introduce a FeesAlreadyReleased error; otherwise reuse EscrowAlreadyReleased.
📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| #[inline] | |
| pub fn validate_release_conditions( | |
| escrow: &Escrow, | |
| release_signer: &Address, | |
| ) -> Result<(), ContractError> { | |
| if escrow.flags.released { | |
| return Err(ContractError::EscrowAlreadyReleased); | |
| } | |
| if escrow.flags.resolved { | |
| return Err(ContractError::EscrowAlreadyResolved); | |
| } | |
| if release_signer != &escrow.roles.release_signer { | |
| return Err(ContractError::OnlyReleaseSignerCanReleaseEarnings); | |
| } | |
| if escrow.milestones.is_empty() { | |
| return Err(ContractError::NoMilestoneDefined); | |
| } | |
| if !escrow.milestones.iter().all(|milestone| milestone.approved) { | |
| return Err(ContractError::EscrowNotCompleted); | |
| } | |
| if escrow.flags.disputed { | |
| return Err(ContractError::EscrowOpenedForDisputeResolution); | |
| } | |
| Ok(()) | |
| } | |
| #[inline] | |
| pub fn validate_release_conditions( | |
| escrow: &Escrow, | |
| release_signer: &Address, | |
| ) -> Result<(), ContractError> { | |
| if escrow.flags.released { | |
| return Err(ContractError::EscrowAlreadyReleased); | |
| } | |
| if escrow.flags.resolved { | |
| return Err(ContractError::EscrowAlreadyResolved); | |
| } | |
| // Prevent double “release” in staged (memo) flow. | |
| // Once fees are released, a second call to release_funds should be rejected | |
| // to avoid re-running commissions logic. | |
| if escrow.withdrawal_state.fees_released { | |
| // Staged release already performed; final payout must use withdraw_with_memo. | |
| return Err(ContractError::EscrowAlreadyReleased); | |
| } | |
| if release_signer != &escrow.roles.release_signer { | |
| return Err(ContractError::OnlyReleaseSignerCanReleaseEarnings); | |
| } | |
| if escrow.milestones.is_empty() { | |
| return Err(ContractError::NoMilestoneDefined); | |
| } | |
| if !escrow.milestones.iter().all(|milestone| milestone.approved) { | |
| return Err(ContractError::EscrowNotCompleted); | |
| } | |
| if escrow.flags.disputed { | |
| return Err(ContractError::EscrowOpenedForDisputeResolution); | |
| } | |
| Ok(()) | |
| } |
🤖 Prompt for AI Agents
In Timebased Escrows/contracts/escrow/src/core/validators/escrow.rs around lines
29 to 59, the validator does not check whether fees have already been released
in the staged (memo) flow, allowing a second release call to re-run commission
logic; add a check for the fees_released flag on escrow.flags (e.g., if
escrow.flags.fees_released { return Err(ContractError::FeesAlreadyReleased) } or
reuse EscrowAlreadyReleased per project convention) so the function returns an
error when fees were previously released and prevents double execution of
release_funds.
| EscrowContract::approve_milestone(env.clone(), 0, escrow.roles.approver.clone()).unwrap(); | ||
|
|
||
| // Release funds - should only release fees since memo is present | ||
| EscrowContract::release_funds(&env, token_admin.clone(), env.current_contract_address()).unwrap() |
There was a problem hiding this comment.
🧩 Analysis chain
Incorrect parameter for trustless_work_address
The release_funds function expects a trustless_work_address parameter for platform fee distribution, but env.current_contract_address() is being passed instead. This appears to be sending fees to the escrow contract itself rather than the intended platform address.
Also applies to: 99-99, 151-151
🏁 Script executed:
#!/bin/bash
# Verify the correct usage of trustless_work_address parameter in release_funds calls
# Check other test files for correct usage patterns
rg -A2 -B2 "release_funds.*trustless_work_address" --type=rust
# Look for the intended platform address setup in tests
rg -n "trustless_work.*[Aa]ddress" --type=rust src/tests/Length of output: 2909
🏁 Script executed:
#!/bin/bash
# Check how trustless_work_address is set up in memo_withdrawal_test.rs vs main test.rs
echo "=== Checking trustless_work_address setup in memo_withdrawal_test.rs ==="
rg -n -A5 -B5 "trustless_work" "Timebased Escrows/contracts/escrow/src/tests/test/memo_withdrawal_test.rs"
echo -e "\n=== Checking trustless_work_address setup in main test.rs ==="
rg -n -A5 -B5 "trustless_work.*=" "Timebased Escrows/contracts/escrow/src/tests/test.rs"
echo -e "\n=== Check all release_funds calls in memo_withdrawal_test.rs ==="
rg -n -A2 -B2 "release_funds" "Timebased Escrows/contracts/escrow/src/tests/test/memo_withdrawal_test.rs"Length of output: 6590
Fix trustless_work_address in memo_withdrawal_test.rs
In Timebased Escrows/contracts/escrow/src/tests/test/memo_withdrawal_test.rs (lines 71, 99, 151), the release_funds calls are passing env.current_contract_address() as the platform fee recipient. Replace those with the trustless_work_address value generated at the top of the test to ensure fees are sent to the intended address.
🤖 Prompt for AI Agents
In Timebased Escrows/contracts/escrow/src/tests/test/memo_withdrawal_test.rs
around lines 71, 99 and 151, the EscrowContract::release_funds calls currently
pass env.current_contract_address() as the platform fee recipient; update those
calls to pass the trustless_work_address variable generated at the top of the
test instead so the platform fees go to the intended address, ensuring the
trustless_work_address variable is in scope for each call and running tests
after change.
| "key": { | ||
| "symbol": "fees_released" | ||
| }, | ||
| "val": { | ||
| "bool": true | ||
| } | ||
| }, | ||
| { | ||
| "key": { | ||
| "symbol": "pending_receiver_amount" | ||
| }, | ||
| "val": { | ||
| "i128": "996500" | ||
| } | ||
| } |
There was a problem hiding this comment.
🧩 Analysis chain
Pending vs. escrow balance mismatch after staged release
withdrawal_state.pending_receiver_amount = 996,500 (Lines 399-405), but the escrow’s token balance is 999,500 (Lines 561-565). If only platform fees (500) were released, both amounts should be 999,500. If additional commissions (3,000) were also released, escrow balance should be 996,500 and corresponding recipient balance(s) should appear. Please reconcile either the pending amount or the ledger balances so the accounting identities hold.
Also applies to: 557-565
🏁 Script executed:
#!/bin/bash
# Locate the test snapshot file
file=$(find . -type f -name "test_release_funds_with_memo.1.json" | head -n1)
echo "Found file: $file"
echo "----- Lines 380-420 (pending_receiver_amount & fees_released) -----"
sed -n '380,420p' "$file"
echo "----- Lines 550-580 (escrow token balance) -----"
sed -n '550,580p' "$file"Length of output: 2646
Pending vs escrow balance mismatch in staged release snapshot
Test snapshot test_release_funds_with_memo.1.json records pending_receiver_amount = 996500 (lines 391–405) but escrow amount = 999500 (lines 561–565). If only platform fees (500) were released, both values must be 999500; if commissions (3000) were released, escrow must be 996500 and corresponding recipient balances updated. Reconcile pending_receiver_amount or ledger balances so the accounting identities hold.
🤖 Prompt for AI Agents
In Timebased
Escrows/contracts/escrow/test_snapshots/tests/test/memo_withdrawal_test/test/test_release_funds_with_memo.1.json
around lines 391–405 and 561–565, the snapshot shows pending_receiver_amount =
996500 while escrow amount = 999500, violating accounting identities; update the
snapshot so the pending_receiver_amount matches the escrow remaining after the
recorded release: if only platform fees of 500 were released set
pending_receiver_amount to 999500 and leave escrow at 999500, otherwise if
commissions of 3000 were released set escrow to 996500 and adjust the
recipient/ledger balances accordingly; apply the change consistently across all
related fields in the snapshot (escrow amount, pending_receiver_amount, and any
recipient or ledger balance entries) to preserve overall debits = credits.
|
Excellent work @big14way! I see that you set up the logic to send specific funds to the receiver if a memo is attached to the escrow. But...my question is, with this solution, where exactly can I attach the recipient's memo to the transaction? This is useful in cases where the recipient wants to receive the funds to an account such as those provided by the Meru platform, which require a memo to be attached to the transaction metadata so that the funds can be correctly delivered to that account in Meru. |
|
The contract only stores receiver_memo; the Example ts |
That makes sense! But can I sign this transaction as configured, as you mention, taking into account that the money to be transferred does not come from my wallet funds but from the funds of an external contract (escrow)? |
|
@armandocodecr Yes, you still sign that transaction even though the actual tokens come out of the escrow contract. Your signature (and any additional auth entries required by withdraw_with_memo) proves to Soroban that you’re the authorized withdrawer, and you cover the network fee from your account; the contract then releases its own balance during the host-function execution. |
2 participants
This PR implements memo-based withdrawals in escrow contracts as discussed in #109.
Changes Made
Testing
The implementation has been thoroughly tested with the following scenarios:
All tests are passing and demonstrate proper functionality.
Documentation
The code is self-documented with clear function names and comments explaining the workflow. Key functions added:
withdraw_with_memo: Handles the memo-based withdrawal processrelease_funds: Updated to support staged releases when memo is presentResolves #109
Summary by CodeRabbit