Skip to content

fix(tholos): account for fee-on-transfer tokens - #216

Open
xtep103 wants to merge 7 commits into
drydocs:mainfrom
xtep103:fix/fee-on-transfer-token
Open

fix(tholos): account for fee-on-transfer tokens#216
xtep103 wants to merge 7 commits into
drydocs:mainfrom
xtep103:fix/fee-on-transfer-token

Conversation

@xtep103

@xtep103 xtep103 commented Sep 11, 2026

Copy link
Copy Markdown
Contributor

Summary

Fix fee-on-transfer token accounting in Tholos.

Incoming token transfers may result in the contract receiving less than the nominal amount requested. Previously, bond accounting relied on the requested transfer amount, which could leave the recorded bond larger than the tokens actually held by the contract.

Changes

  • Added balance-delta verification for incoming token transfers.
  • Record the actual amount received for assertion bonds.
  • Verify the actual amount received for dispute deposits.
  • Prevent resolution payouts from exceeding the contract's available token balance.
  • Added a mock fee-on-transfer token for regression testing.
  • Added tests covering assertion deposits, dispute deposits, and resolution payouts.

Testing

  • cargo test -p tholos
  • All Tholos tests pass.

Related issue

Fixes the fee-on-transfer token accounting issue.
Closes #164

@collinsezedike

Copy link
Copy Markdown
Collaborator

Hey @xtep103, this PR has merge conflicts with main in contracts/tholos/src/lib.rs and contracts/tholos/src/test.rs. GitHub cannot build the merge commit while the conflicts are there, so CI has not run at all (I closed and reopened the PR earlier to try retriggering checks; no run can be created until the branch is rebased).

Please rebase onto latest main and push. I'll review once the conflicts are resolved and checks are green.

Also, does this PR supersede #180? Both are open against issue #164. If this one replaces it, let me know and I'll close #180 so the issue keeps a single active PR.

@xtep103
xtep103 force-pushed the fix/fee-on-transfer-token branch from 2dc2e65 to fb62dad Compare September 12, 2026 05:42
@xtep103

xtep103 commented Sep 12, 2026

Copy link
Copy Markdown
Contributor Author

Thanks for the heads-up. I realized I rebased against my fork's origin/main, not the upstream repository. I'm checking against upstream/main now and will rebase/push any required conflict resolutions so CI can run.

Regarding #180: yes, #216 is intended to replace #180. I opened #216 after running into multiple issues with the earlier implementation and reworked the fee-on-transfer handling approach. Once #216 is in good shape, #180 can be closed so there is only one active PR for issue #164.

@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.

@xtep103 A few things before this can merge.

test_fee_on_transfer_resolve_uses_available_balance only asserts fee_token.balance(&contract_id) >= 0, which is trivially true regardless of whether the payout logic is correct. Assert the actual winner's balance after resolve, and add a second case where the disputer wins (agrees_with_asserter = false), since resolve()'s payout path treats both winners identically but only one is exercised here.

The test itself demonstrates a real asymmetry: dispute() requests assertion.bond (already reduced from 100 to 90 by the fee in assert_outcome) rather than the original nominal bond amount, so the disputer's deposit gets fee-reduced twice in effect (90 requested -> 81 received), ending up with less escrowed than the asserter despite both nominally posting "the bond". Worth reconsidering whether dispute() should request the original bond_amount instead of the already-adjusted assertion.bond, so a single fee application affects both sides equally rather than compounding on the disputer's side specifically.

test.rs has a duplicated comment banner, "Fee-on-transfer token tests" appears twice in a row (lines 5-7 and 9-11 of the diff). Remove the duplicate.

@collinsezedike

Copy link
Copy Markdown
Collaborator

@xtep103 This also duplicates your own #180, which is still open with unaddressed review feedback from Sept 7. More importantly, #180 already introduced the AssertionEscrow storage mechanism that issue #207 (and gloskull's #211) was explicitly written to build on. Switching to a different mechanism here breaks that shared foundation without anyone deciding to change it.

Please pick one PR and close the other. If you want to move away from AssertionEscrow, say so on #207 first since gloskull's work depends on it.

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] No exact-amount transfer verification permanently deadlocks a disputed assertion

2 participants