Skip to content

fix(tholos): cap reclaim_stalled_dispute payouts at actual escrow (closes #207) - #211

Merged
collinsezedike merged 1 commit into
drydocs:mainfrom
gloskull:reclaim_stalled_dispute
Sep 15, 2026
Merged

collinsezedike merged 1 commit into
drydocs:mainfrom
gloskull:reclaim_stalled_dispute

Conversation

@gloskull

@gloskull gloskull commented Sep 8, 2026 •

Copy link
Copy Markdown

Summary

reclaim_stalled_dispute refunds each side the amount they actually deposited rather than the raw nominal bond, derived from the per-assertion escrow that #216 landed for finalize/resolve. The asserter's payout is the recorded received bond, the disputer's is the escrow remainder, and both are capped by the contract's live token balance, mirroring the same pattern the other payout paths use.

The refunded field of StalledDisputeReclaimed now reports the asserter's refund specifically, since the two sides can differ under a fee-on-transfer token.

Testing

  • Added test_reclaim_stalled_dispute_fee_on_transfer: a stalled dispute funded with a fee-on-transfer token refunds each side's actual deposit and drains the contract to zero rather than trapping on insufficient balance or overpaying from other assertions' pooled funds.
  • cargo test -p tholos (93 passing), cargo fmt --check, cargo clippy --workspace --all-targets --locked -- -D warnings all clean.
  • SDK bindings regenerated against the rebuilt wasm; drift check passes.

Closes #207

@collinsezedike

Copy link
Copy Markdown
Collaborator

This PR's diff includes a large number of unrelated test snapshots across asserter-consumer, demo-consumer, and tholos-v2 that have nothing to do with this fix, plus a self-merge commit in the history. This looks like a branch mix-up on your end (possibly branched from a stale fork main). Please rebase this onto a fresh branch from drydocs/tholos's actual main so the diff only contains the reclaim_stalled_dispute fix and its own test, this is unreviewable as currently scoped.

Separately: this issue explicitly depends on #180's AssertionEscrow mechanism, which is still open with requested changes, not yet merged. Once you've rebased cleanly, make sure your fix builds on top of #180's actual final version rather than reimplementing the escrow tracking separately.

@collinsezedike

Copy link
Copy Markdown
Collaborator

@gloskull Checking in, any progress on rebasing this? Let us know if you're still working it or need to hand it off.

@collinsezedike collinsezedike left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@gloskull Three things need fixing before this can merge:

  1. cargo fmt --check fails on lib.rs (two spots around reclaim_stalled_dispute where the multi-line token_client.transfer(...) calls should collapse to one line). Run cargo fmt and commit the result.

  2. packages/tholos-sdk/src/index.ts is out of date with the contract's new public interface. Regenerate it per packages/tholos-sdk/README.md and commit the result.

  3. This branch now conflicts with main: PR #214 (merged after this branch was opened) moved admin assignment from initialize's first parameter into a __constructor(admin) call made at deploy time, so initialize no longer takes an admin argument. Rebase onto main and update test.rs's initialize calls (including the new test_reclaim_stalled_dispute_fee_on_transfer test) to register with env.register(Tholos, (admin,)) and drop admin from the initialize/try_initialize argument lists, matching the rest of the suite.

@collinsezedike

Copy link
Copy Markdown
Collaborator

@gloskull Checking in, any progress on the three items above? Let us know if you're still working it or need to hand it off.

@gloskull

Copy link
Copy Markdown
Author

Hello @collinsezedike , I'd work on it now.

@collinsezedike

Copy link
Copy Markdown
Collaborator

@gloskull This has drifted into two separate PRs for the same issue: this one and #220, both fixing the same escrow-accounting logic. Please consolidate into one, close the other, and rebase onto main; both are currently marked CONFLICTING.

@collinsezedike

Copy link
Copy Markdown
Collaborator

Hey @gloskull, I've pushed a rework of this PR directly to the branch, superseding the previous commits. Here is why and what changed.

While this PR sat unmerged, #216 landed the AssertionEscrow mechanism on main for #164, covering assert_outcome, dispute, finalize, and resolve. That left two parallel escrow implementations targeting the same storage key, and rebasing your branch would have dragged the duplicate into direct conflict with the merged code. Since #207's stated scope is to mirror the pattern #180/#216 landed in reclaim_stalled_dispute, I rebuilt the fix on top of current main instead:

  • reclaim_stalled_dispute now derives each side's actual deposit from the merged escrow. The asserter's payout is the recorded received bond, the disputer's is the escrow remainder, and both are capped by the contract's live token balance, mirroring finalize and resolve.
  • The refunded field of StalledDisputeReclaimed now reports the asserter's refund specifically, since the two sides can differ under a fee-on-transfer token.
  • Added test_reclaim_stalled_dispute_fee_on_transfer covering the issue's scenario: a stalled dispute funded with a fee token refunds the actual deposits and drains the contract to zero rather than trapping or overpaying.
  • CHANGELOG entry added, SDK bindings regenerated, snapshots updated.

All 93 tests pass locally, fmt and clippy are clean, and CI has been approved and is running on the new head. The original review items (formatting, bindings, rebase) are all resolved as part of the rebuild.

@collinsezedike collinsezedike changed the title Reclaim stalled dispute fix(tholos): cap reclaim_stalled_dispute payouts at actual escrow (closes #207) Sep 15, 2026

@collinsezedike collinsezedike left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hey @gloskull, thank you for the contribution and for getting the original fix started. Merging now.

@collinsezedike
collinsezedike merged commit 4ec0cdf into drydocs:main Sep 15, 2026
4 checks passed
@collinsezedike

Copy link
Copy Markdown
Collaborator

Hey @gloskull, thank you for the contribution. If the work was worth your time, a star on the repo helps other contributors find the project.

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.

[Bug] reclaim_stalled_dispute doesn't account for fee-on-transfer shortfall

2 participants