refactor!: retire the Token suffix - #348
Draft
zguesmi wants to merge 1 commit into
Draft
Conversation
Native-asset mode is gone, so the `Token` suffix no longer distinguishes
anything.
- `IexecEscrowTokenFacet` becomes `IexecEscrowFacet`
- the interface `IexecEscrowToken` becomes `IexecEscrow`
- `IexecInterfaceToken` becomes `IexecInterface`
The abstract contract that was named `IexecEscrow` becomes `IexecEscrowBase`,
so the clean name goes to the interface. Interfaces are the published surface
consumers compile against, and the repository already uses the `Base` suffix
for shared abstracts (`FacetBase`).
`IexecTokenSpender` keeps its name: it describes ERC-20 `receiveApproval`
semantics, not a native/token distinction.
The `IexecEscrow:` prefix of the `_transfer` revert reasons is left alone.
Those strings are observable behavior; renaming a contract must not change
them.
BREAKING CHANGE: published ABI paths and artifact names change.
- `abis/contracts/facets/IexecEscrowTokenFacet.json` becomes
`abis/contracts/facets/IexecEscrowFacet.json`
- `abis/contracts/IexecInterfaceToken.json` becomes
`abis/contracts/IexecInterface.json`
- `abis/contracts/interfaces/IexecEscrowToken.json` becomes
`abis/contracts/interfaces/IexecEscrow.json`
- `abis/contracts/abstract/IexecEscrow.json` becomes
`abis/contracts/abstract/IexecEscrowBase.json`
Function selectors are unaffected: a contract name is not part of a selector,
so there is no protocol break. The ABI contents are byte-identical to the
previous paths.
The iExec SDK and the subgraph need advance notice, since the SDK references
`contracts/IexecInterfaceToken.sol` by path.
`deployments/{arbitrum,arbitrumSepolia}/IexecEscrowTokenFacet.json` is left in
place: it records what is deployed. `hardhat-deploy` keys artifacts by contract
name, so the next upgrade writes `IexecEscrowFacet.json` next to it and
`removeDanglingFacetDeploymentArtifacts` deletes the stale one.
The hand-maintained lists were updated: `getAllLocalFacetFunctions` in
`utils/proxy-tools.ts` and the facet groups in `scripts/tools/sol-to-uml.mjs`.
The historical upgrade reports under `scripts/upgrades/` keep the old names.
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 @@
## pr/b-unify-transfer #348 +/- ##
====================================================
Coverage 99.53% 99.53%
====================================================
Files 31 31
Lines 1083 1083
Branches 220 220
====================================================
Hits 1078 1078
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.
Stacked on #347.
Native-asset mode is gone (#343), so the
Tokensuffix no longer distinguishes anything.contracts/facets/IexecEscrowTokenFacet.sol->IexecEscrowTokenFacetcontracts/facets/IexecEscrowFacet.sol->IexecEscrowFacetcontracts/interfaces/IexecEscrowToken.sol->IexecEscrowTokencontracts/interfaces/IexecEscrow.sol->IexecEscrowcontracts/IexecInterfaceToken.sol->IexecInterfaceTokencontracts/IexecInterface.sol->IexecInterface(retires the TODO)contracts/abstract/IexecEscrow.sol->IexecEscrowcontracts/abstract/IexecEscrowBase.sol->IexecEscrowBaseIexecTokenSpenderkeeps its name: it describes ERC-20receiveApprovalsemantics, not a native/token distinction.The name collision
contracts/abstract/IexecEscrow.solalready declared an abstract contract namedIexecEscrow, so one of the two had to move. The abstract was renamed toIexecEscrowBaseand the interface took the clean name.Why that way round:
abis/contracts/interfaces/and are what the SDK and integrators compile against. The abstract is an internal implementation detail that never appears in a consumer's import.Basesuffix for exactly this role:FacetBaseis the shared abstract every facet inherits.IexecEscrowBasereads the same way.IexecEscrowFacet is ... IexecEscrowBaseis self-explanatory, whereIexecEscrowFacet is ... IexecEscrowwould have read as "the facet inherits the interface it implements" and hidden which of the twoIexecEscrowdeclarations was meant.Two
IexecEscrowdeclarations in different directories are not shipped: there is exactly one, the interface.Consequences
Function selectors are unaffected. A contract's name is not part of a selector, so nothing about the deployed protocol surface changes. The ABI contents are byte-identical to the old paths, verified by diffing against the pre-rename snapshot:
Published ABI paths change (breaking for anyone importing them by path):
abis/contracts/facets/IexecEscrowTokenFacet.jsonabis/contracts/facets/IexecEscrowFacet.jsonabis/contracts/IexecInterfaceToken.jsonabis/contracts/IexecInterface.jsonabis/contracts/interfaces/IexecEscrowToken.jsonabis/contracts/interfaces/IexecEscrow.jsonabis/contracts/abstract/IexecEscrow.jsonabis/contracts/abstract/IexecEscrowBase.json(plus the mirrored
human-readable-abis/entries)Deployment artifacts.
deployments/{arbitrum,arbitrumSepolia}/IexecEscrowTokenFacet.jsonis deliberately left in place — it records what is deployed right now.hardhat-deploykeys artifacts by contract name, so the next upgrade writesIexecEscrowFacet.jsonalongside the old file;removeDanglingFacetDeploymentArtifactsinutils/proxy-tools.tsdeletes the stale one once the new facet is on-chain.Hand-maintained lists updated, since neither is generated:
getAllLocalFacetFunctionsinutils/proxy-tools.tsscripts/tools/sol-to-uml.mjs(['IexecEscrowFacet', 'IexecEscrowBase'])Historical records left alone. The upgrade reports under
scripts/upgrades/*.mdandCHANGELOG.mdkeep the old names — they are the record of what was actually deployed.scripts/upgrades/v6.2.0.tsneeded the one change that makes it compile (the typechain factory identifier); its recordedname: 'IexecEscrowTokenFacet'strings and addresses are untouched, with a comment saying why. That file already carries aname: 'IexecERC20Facet'entry for a facet that no longer exists, so this matches how the script is treated.Two test files were renamed with the contracts they cover, and one more to keep the names unambiguous:
test/byContract/IexecEscrow/IexecEscrowToken.test.tstest/byContract/IexecEscrow/IexecEscrow.test.tstest/byContract/IexecEscrow/IexecEscrowToken-receiveApproval.test.tstest/byContract/IexecEscrow/IexecEscrow-receiveApproval.test.tstest/byContract/IexecPocoBoost/IexecEscrow.test.tstest/byContract/IexecPocoBoost/IexecEscrowBase.test.tsThe last one exercises the abstract through
IexecEscrowTestContract; without the rename the repository would have had twoIexecEscrow.test.tsfiles meaning different things.Downstream notice needed
The iExec SDK and the subgraph need advance notice. The SDK references the aggregate by path (
contracts/IexecInterfaceToken.sol) — that path and the interface name both change. The subgraph is unaffected at the event level (no topic0 moves, no ABI content changes) but will need the new artifact path if it consumesabis/by filename.Not changed
The
IexecEscrow:prefix on the_transferrevert reasons stays. Those strings are observable behavior; a rename must not move them. They now name the interface rather than the abstract, which is documented inIexecEscrowBase.Verification
npm run buildnpm run check-storage-layoutnpm run docdocs/solidity/index.mdcommittednpm run sol-to-umlnpx tsc --noEmitnpm testnpm run format:checknpm run formatwas needed after the rename: five test files had import lines that reflowed once the identifiers got shorter.