Skip to content

refactor!: unify the two _transfer implementations - #347

Draft
zguesmi wants to merge 1 commit into
pr/a-dedupe-eventsfrom
pr/b-unify-transfer
Draft

refactor!: unify the two _transfer implementations#347
zguesmi wants to merge 1 commit into
pr/a-dedupe-eventsfrom
pr/b-unify-transfer

Conversation

@zguesmi

@zguesmi zguesmi commented Aug 31, 2026

Copy link
Copy Markdown
Member

Stacked on #346.

The problem

_transfer existed twice over the same $.m_balances storage, with different failure behavior:

contracts/abstract/IexecEscrow.sol contracts/facets/IexecEscrowTokenFacet.sol (_transferUnchecked)
over-balance require(value <= fromBalance, "IexecEscrow: Transfer amount exceeds balance") if (senderBalance < amount) { revert(); } — no reason
zero address "IexecEscrow: Transfer from/to empty address" "ERC20: transfer from/to the zero address"
arithmetic unchecked checked
marked TEMPORARY MIGRATION FIX ... TODO: Remove this in the next major version

So the same over-balance mistake reverted with a message through matchOrders/contribute and silently through transfer/transferFrom.

Which behavior won, and why

The escrow implementation wins: reason strings on all three checks, unchecked arithmetic. It is now internal, and IexecEscrowTokenFacet inherits IexecEscrow instead of carrying its own copy.

Reasoning:

  1. Nothing loses a revert reason; one path gains one. The reverse choice would have had to invent a new bare-revert path or keep the reason-less behavior that is already marked for deletion.
  2. The bare revert() was explicitly temporary. The comment says "remove in the next major version" and this train is the next major (feat!: remove native mode contracts #343 and feat!: remove setName function and related tests for reverse registration #344 are already feat!, release-please has 7.0.0 drafted). Keeping it means carrying it another whole major.
  3. It is the smaller behavior change for users. The IexecEscrow: Transfer amount exceeds balance message is the high-traffic one — it is what an under-funded requester or scheduler sees on matchOrders, contribute and finalize — and it does not move. What changes is the ERC-20 zero-address wording, which is close to unreachable in practice (transfer from the zero address requires msg.sender == address(0)), and the bare revert, which carried no information to break.
  4. unchecked is free here. The balance is checked immediately above, and the total supply is capped with no minting in this path, so neither underflow nor overflow is reachable.

Cost paid: the IexecEscrow: prefix now appears on ERC-20 revert reasons. That is cosmetic — preserving the exact strings that production and 9 test assertions already depend on beat re-prefixing them, which would have broken both sides at once.

Host contract: not FacetBase

The issue suggested contracts/abstract/FacetBase.sol as the host. I did not use it, for one measurable reason: FacetBase is inherited by all 12 facets, and hosting an event-emitting balance mover there requires the Transfer event declaration to sit in FacetBase too. 8 of the 12 facets have no Transfer event in their published ABI today (IexecAccessorsABILegacyFacet, IexecCategoryManagerFacet, IexecConfigurationFacet, IexecConfigurationExtraFacet, IexecOrderManagementFacet, IexecPocoAccessorsFacet, IexecPocoBoostAccessorsFacet, IexecRelayFacet) and would have gained one — 8 published-ABI changes, plus the ability to move balances handed to every facet.

Keeping the implementation in IexecEscrow gives the same single implementation with zero ABI change anywhere: IexecEscrowTokenFacet already carries all six events (Approval, Lock, Reward, Seize, Transfer, Unlock) in its ABI.

The facet's base list needed reordering (FacetBase, IexecERC20, IexecTokenSpender, IexecEscrowToken, IexecEscrow) to keep C3 linearization solvable after #346 introduced the shared event interfaces.

Migration note

Callers must expect a reason string on every failed balance move:

Case Before After
transfer / transferFrom over balance revert, no reason IexecEscrow: Transfer amount exceeds balance
transfer / transferFrom from the zero address ERC20: transfer from the zero address IexecEscrow: Transfer from empty address
transfer / transferFrom to the zero address ERC20: transfer to the zero address IexecEscrow: Transfer to empty address

Unchanged: every function selector, every event, every ABI entry, and the ERC20: approve ... messages (_approve was not touched).

Anything matching on the old strings — SDK error handling, monitoring, integration tests — needs updating. The subgraph is unaffected: no event and no topic0 moved.

Tests

Six assertions in test/byContract/IexecERC20/IexecERC20.test.ts were updated to the new strings, deliberately and not by loosening them: two revertedWithoutReason() calls became revertedWith('IexecEscrow: Transfer amount exceeds balance'), four zero-address messages were re-pointed. The nine existing IexecEscrow: Transfer amount exceeds balance assertions in the PoCo, Boost and escrow suites pass untouched.

Left as revertedWithoutReason() on purpose, since they are allowance and burn paths and out of this PR's scope: transferFrom with too low an allowance, decreaseAllowance below zero, and withdraw over balance (_burn). Their TEMPORARY MIGRATION FIX markers remain.

Verification

Gate Result
npm run build compiled 119 Solidity files successfully
npm run check-storage-layout pass, exit 0, no output
npm run doc regenerated, no change to docs/solidity/index.md
npm run sol-to-uml regenerated, 4 SVGs committed
npx tsc --noEmit the 6 known pre-existing errors, no new ones
npm test 514 passing, 6 pending, 0 failing (same as base)
npm run format:check All matched files use Prettier code style
abis/ diff vs base unchanged beyond #346's four interface files

IexecEscrowTokenFacet runtime bytecode: 6608 -> 6695 bytes (+87). That is the third revert string being added; the inherited lock/unlock/reward/seize internals are unreachable from this facet and were eliminated by the optimizer, which would otherwise have cost several hundred bytes.

`_transfer` existed twice over the same `$.m_balances` storage with different
failure behavior: the escrow one in `contracts/abstract/IexecEscrow.sol`
reverted with a reason string, the ERC-20 one in
`contracts/facets/IexecEscrowTokenFacet.sol` (`_transferUnchecked`) reverted
with no reason at all. An over-balance transfer therefore reverted with a
message on one path and silently on the other.

The escrow implementation wins and becomes the single one. It is now
`internal` and `IexecEscrowTokenFacet` inherits it, so `transfer` and
`transferFrom` route through it. `_transferUnchecked` and its `_transfer`
wrapper are deleted, along with their `TEMPORARY MIGRATION FIX` /
`TODO: Remove this in the next major version` comment.

BREAKING CHANGE: the revert reasons of the ERC-20 entry points change.

-   `transfer` / `transferFrom` over balance: reverted with no reason, now
    reverts with `IexecEscrow: Transfer amount exceeds balance`.
-   `transfer` / `transferFrom` from the zero address:
    `ERC20: transfer from the zero address` becomes
    `IexecEscrow: Transfer from empty address`.
-   `transfer` / `transferFrom` to the zero address:
    `ERC20: transfer to the zero address` becomes
    `IexecEscrow: Transfer to empty address`.

Callers must expect a reason string on every failed balance move. No
selector, no event and no ABI entry changes.

The remaining `TEMPORARY MIGRATION FIX` markers on the allowance checks in
`transferFrom` and `decreaseAllowance`, and on `_burn`, are out of scope and
left untouched.

Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
@codecov

codecov Bot commented Aug 31, 2026

Copy link
Copy Markdown

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 99.53%. Comparing base (6f21cca) to head (f932325).

Additional details and impacted files
@@                  Coverage Diff                   @@
##           pr/a-dedupe-events     #347      +/-   ##
======================================================
- Coverage               99.54%   99.53%   -0.01%     
======================================================
  Files                      31       31              
  Lines                    1095     1083      -12     
  Branches                  212      220       +8     
======================================================
- Hits                     1090     1078      -12     
  Misses                      5        5              

☔ View full report in Codecov by Harness.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.
  • 📦 JS Bundle Analysis: Save yourself from yourself by tracking and limiting bundle sizes in JS merges.

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.

1 participant