refactor: declare escrow and ERC-20 events once - #346
Draft
zguesmi wants to merge 5 commits into
Draft
Conversation
Remove the duplicate declarations of `Transfer`, `Reward`, `Seize`, `Lock`
and `Unlock`. They were declared twice, in `IexecERC20` and in the abstract
`IexecEscrow`, and only compiled because the two declaration sites were
inherited by disjoint sets of contracts.
Each event is now declared exactly once, in an interface:
- `IexecERC20Events` holds `Transfer`, which both the ERC-20 surface and the
escrow accounting emit. `IexecERC20` and the abstract `IexecEscrow`
inherit it.
- `IexecEscrowEvents` holds `Reward`, `Seize`, `Lock` and `Unlock`. These are
escrow-accounting events emitted by the PoCo facets, never by ERC-20 code,
so they leave the ERC-20 surface. The abstract `IexecEscrow` and the
`IexecEscrowToken` interface inherit it, which keeps them reachable from
`IexecInterfaceToken`.
`Approval` stays in `IexecERC20`: only the ERC-20 entry points emit it.
Also remove the dead `'dev-token'` entry from `skippedNetworks` in
`scripts/verify.ts`; that network was deleted from `hardhat.config.ts`.
No ABI change: event signatures are untouched, so topic0 does not move, and
the contract-level ABIs are byte-identical.
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #346 +/- ##
=======================================
Coverage 99.54% 99.54%
=======================================
Files 31 31
Lines 1095 1095
Branches 212 223 +11
=======================================
Hits 1090 1090
Misses 5 5 ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
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
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.
Non-breaking cleanup found while reviewing #345.
1. Dead network entry
scripts/verify.tsstill listed'dev-token'inskippedNetworks. That network was deleted fromhardhat.config.tsin a6775e4, so the entry was dead.2. Duplicate event declarations
Transfer,Reward,Seize,LockandUnlockwere each declared twice: incontracts/interfaces/IexecERC20.soland incontracts/abstract/IexecEscrow.sol. It compiled only because the two declaration sites are inherited by disjoint contract sets (IexecEscrow->IexecPoco1Facet,IexecPoco2Facet,IexecPocoBoostFacet;IexecERC20->IexecEscrowTokenFacetand theIexecInterfaceTokenaggregate). Any contract that ended up inheriting both would have failed to compile.Each event is now declared exactly once, in an interface (events are ABI, so an interface is the right home):
IexecERC20EventsTransferIexecERC20, abstractIexecEscrowIexecEscrowEventsLock,Unlock,Reward,SeizeIexecEscrow,IexecEscrowTokenApprovalstays inIexecERC20: only the ERC-20 entry points emit it.3. Escrow events leave the ERC-20 surface
Reward,Seize,LockandUnlockare escrow-accounting events. They are emitted fromcontracts/abstract/IexecEscrow.sol,contracts/facets/IexecPoco2Facet.solandcontracts/facets/IexecPocoBoostFacet.sol, never by ERC-20 code, so they no longer sit onIexecERC20.They stay reachable from
IexecInterfaceTokenthrough theIexecEscrowTokeninterface, so the aggregate's flattened ABI does not change.Two design notes
Transfermoved to its own interface instead of staying inline inIexecERC20. The abstractIexecEscrowalso emitsTransfer, and it cannot inheritIexecERC20(that interface has functions, and the concrete PoCo facets would then have to implementtransfer,approve, ...). A shared events-only parent is the only way to declare the event once and keep it in every current ABI.Approvalis deliberately not in that parent: adding it there would add anApprovalevent to the three PoCo facet ABIs, which do not have one today.IexecEscrowTokennow declares four events it never emits.IexecEscrowTokenFacetcarriesLock,Unlock,RewardandSeizein its published ABI today, inherited by accident fromIexecERC20. Keeping the facet's ABI byte-identical requires preserving that. Dropping them from the escrow facet's ABI is a separate, breaking change.Verification
ABI gate:
abis/snapshotted before the change, rebuilt, diffed. Only the two per-interface files that were supposed to move changed, plus the two new interface files:Every contract-level ABI is byte-identical:
IexecEscrowTokenFacet,IexecInterfaceToken,IexecPoco1Facet,IexecPoco2Facet,IexecPocoBoostFacetand all other facets. ABI entries are sorted by solc, so the reshuffled inheritance does not move them.topic0 is unchanged. No event signature was edited, only the declaration site, so every topic0 stays where it is and the subgraph is unaffected.
Runtime bytecode of
IexecEscrowTokenFacetis identical up to the trailing CBOR metadata hash (first difference at byte offset 6566 of 6609, inside the metadata blob). The change is source-only; only the metadata hash moves.npm run buildnpm run check-storage-layoutnpm run docdocs/solidity/index.mdcommittednpm run sol-to-umlnpx tsc --noEmitnpm testnpm run format:check