refactor: convert abstract contracts to libraries - #353
Draft
zguesmi wants to merge 14 commits into
Draft
Conversation
The abstract contract was a stateless helper over diamond storage that used nothing from FacetBase, so inheriting it forced three facets into a base they did not need. A library expresses the same thing without the inheritance edge, and `internal` library functions are inlined, so the call sites keep their previous cost. The function loses its leading underscore, which marked it as internal to a contract and means nothing in a library. The two `using IexecLibOrders_v5` directives were unreferenced and are dropped. No ABI change: the contract exported no ABI entries, having only internal members.
…ibrary Like the deal volume helper, the abstract contract was a stateless helper over diamond storage and imposed itself as a base on five facets. A library removes that inheritance edge; `internal` library functions are inlined, so the 66 call sites keep their previous cost. The one member it did take from FacetBase, the GROUPMEMBER_PURPOSE constant, is redeclared in the library, since a library cannot inherit. The functions lose their leading underscores, which marked them as internal to a contract and mean nothing in a library. IexecPocoAccessorsFacet and IexecOrderManagementFacet reached FacetBase only through SignatureVerifier and use it (the deadline ratios, the kitty constants, _msgSender), so they now declare it directly. The other three facets still reach it through IexecEscrow. No ABI change: the contract exported no ABI entries, having only internal members.
…ew libraries The two converted helpers moved out of `contracts/abstract/`, so the docgen exclude list named files that no longer exist. Both libraries are still excluded for the same reason as before: they expose only internal members, so they would document nothing.
The shorter name matches the other helper libraries and lets 29 of the 66 call sites fit back on a single line.
Converting SignatureVerifier to a library forced a second declaration of GROUPMEMBER_PURPOSE, because a library cannot inherit FacetBase and a contract constant is not reachable from outside the contract at any visibility: solc rejects FacetBase.GROUPMEMBER_PURPOSE from a library with "Member not found or not visible after argument-dependent lookup". A library constant has no such restriction, so ConstantsLib becomes the single declaration of all eight protocol constants. FacetBase keeps the eight names as aliases of the library's values, so the 31 bare uses in the facets and the eight public getters on IexecPocoAccessorsFacet are untouched. No ABI change. Constant folding is preserved: the deployed bytecode still holds one PUSH each for 1e9, the kitty address and the stake ratio, and the same count of the literal 4. Four facets grew between 8 and 48 bytes of jump-offset churn under viaIR; the largest, IexecPocoBoostFacet, sits at 16665 of the 24576-byte limit.
This reverts commit 737221a. The constants move is deferred to the next PR, so SignatureLib keeps its own GROUPMEMBER_PURPOSE for now and FacetBase carries a TODO recording why the duplicate exists and what replaces it.
The two libraries served the same five facets and each held helpers that are stateless reads over diamond storage, so one library keeps the import lists shorter without changing what the facets can reach. computeDealVolume goes last, after the signature and restriction helpers. The deployed bytecode of every facet is byte-identical to before the merge, and no ABI changes: internal library functions are inlined either way.
The library and facet names still carry suffixes the next PR removes: IexecLibCore_v5 becomes CoreLib, IexecLibOrders_v5 becomes OrdersLib and IexecEscrowTokenFacet becomes IexecEscrowFacet. IexecInterfaceToken already carried its own note.
The last of the three stateless helpers over diamond storage leaves contracts/abstract/, which now holds FacetBase alone. The four facets that inherited the escrow declare FacetBase directly instead; each of them uses it. IexecEscrowTestContract becomes a plain wrapper around the library. The library declares the five events it emits. solc 0.8.21 cannot compile a qualified `emit IexecEscrowEvents.Lock(...)` when userdoc is requested and docgen always requests it, so a second declaration is the only option; IexecEscrowEvents stays as the declaration that feeds IexecInterfaceToken, whose ABI is unchanged. Events now follow the emit path, so the dead entries disappear from the facet ABIs: IexecPoco1Facet drops from six events to Transfer and Lock, and IexecPoco2Facet and IexecPocoBoostFacet drop Approval. IexecEscrowTokenFacet is the exception. It reaches the events through IexecEscrowToken and also emits Transfer through the library, so its ABI lists Transfer twice and typechain generates 14 duplicate-identifier errors for it. Moving _mint, _burn and _approve into the library removes the duplicate; that is recorded as a TODO on the contract and deferred to the next PR. The members lose their leading underscores, which marked them as internal to a contract, so _transfer is now EscrowLib.transfer. The revert strings still read "IexecEscrow: ..." because tests assert on them.
The IexecEscrow section becomes EscrowLib and now documents the five events the library declares. The class diagrams pick up CommonLib, EscrowLib and the facets that reach FacetBase directly. sol-to-uml.mjs named IexecEscrow as a base contract of the IexecEscrows diagram; it is EscrowLib now.
Coverage failed with 25 errors as soon as the shared helpers became
libraries. The first failure is the real one, the other 24 cascade from it:
TypeError: Cannot read properties of undefined (reading 'replace')
at Function.linkBytecode (typechain/factories/.../IexecPocoAccessorsFacet__factory.ts)
CommonLib and EscrowLib expose only internal functions, so their code is
inlined and their addresses are never used at runtime. But solc's IR
pipeline emits a linkersymbol reference for every library a contract uses
and leaves the Yul unused pruner to drop it once it is unreachable. The
main build runs that pruner, `optimizerSteps: 'u'`; .solcover.js disabled
every step with `optimizerSteps: ''`, so the dead reference survived into
the artifact. Typechain then generated the linkLibraryAddresses variant of
the factory constructor for facets that need no linking, and
deploy/0_deploy.ts builds those with a signer alone.
The throw left CreateX half-deployed, so every later loadFixture re-ran
against dirty state and failed with "Nonce too low. Expected nonce to be 1
but got 0".
Enabling the pruner removes the reference. Verified: the only link
reference left on the facets is IexecLibOrders_v5, which does have public
functions and is linked in both builds; coverage compiles without the
stack-too-deep that a full optimizer step list causes; and the totals are
unchanged at 99.69 / 96.55 / 99.44 / 99.53, with CommonLib and EscrowLib
at 100.
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## refactor/remove-duplicate-events #353 +/- ##
=================================================================
Coverage 99.53% 99.53%
=================================================================
Files 31 30 -1
Lines 1069 1069
Branches 215 204 -11
=================================================================
Hits 1064 1064
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
1 participant
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.
contracts/abstract/IexecPocoCommon.sol→contracts/libs/CommonLib.solcontracts/abstract/SignatureVerifier.sol→contracts/libs/CommonLib.solcontracts/abstract/IexecEscrow.sol→contracts/libs/EscrowLib.solNew digram
flowchart LR subgraph abstract FacetBase end subgraph libs CommonLib EscrowLib end IexecAccessorsABILegacyFacet --> FacetBase IexecCategoryManagerFacet --> FacetBase IexecConfigurationExtraFacet --> FacetBase IexecConfigurationFacet --> FacetBase IexecEscrowTokenFacet --> FacetBase IexecOrderManagementFacet --> FacetBase IexecPoco1Facet --> FacetBase IexecPoco2Facet --> FacetBase IexecPocoAccessorsFacet --> FacetBase IexecPocoBoostAccessorsFacet --> FacetBase IexecPocoBoostFacet --> FacetBase IexecRelayFacet --> FacetBase IexecEscrowTokenFacet -.-> EscrowLib IexecOrderManagementFacet -.-> CommonLib