Skip to content

[#4989] Serve CoreFacet's views from CoreViewFacet and trim LibCore's transition code - #91

Open
aregng wants to merge 4 commits into
feature/evm_automationfrom
task/issue-4989
Open

aregng wants to merge 4 commits into
feature/evm_automationfrom
task/issue-4989

Conversation

@aregng

@aregng aregng commented Nov 13, 2026 •

Copy link
Copy Markdown

Closes Entropy-Foundation/smr-moonshot#4989 (close by hand after merge; this repository has issues disabled).

Summary

  • New CoreViewFacet serves CoreFacet's seven views: getCycleInfo, getCycleDuration, getTransitionInfo, isAutomationEnabled, isAutomationReadyEnabled, getCycleStateDetails and getRecordMaxTaskCount. ICoreViewFacet declares them. ICoreFacet keeps CoreFacet's events, errors and five state-changing functions. This follows the RegistryViewFacet split (smr-moonshot#4101).
  • LibCore trims (no behaviour change):
    • dropOrChargeTask drops a cancelled or expired task through dropWithWholeDeposit, the helper the other drop paths use.
    • tryWithdrawTaskAutomationFee returns its outcome as one fresh, zeroed TransitionResult instead of several values. This keeps dropOrChargeTasks within the EVM stack limit (it was one slot from exceeding it), and the outcome does not depend on what the caller's struct held.
    • dropOrChargeTasks shortens removedBuffer in place to the removed count instead of copying it.
  • Genesis wiring:
    • CoreViewFacet gets a GenesisTransactionTags variant declared and ranked right after CoreFacet, so the Diamond's genesis address moves by one nonce.
    • FacetsDeployment, the Diamond constructor's facet list, LibDiamondUtils, the deploy and selector-check scripts and compile_config.toml include the new facet.
    • The golden bytecode hashes for CoreFacet, CoreViewFacet and Diamond are regenerated.
  • Guidance for future changes: CoreFacet's header now records three things:
    • Any change to CoreFacet, or to a library it inlines (LibCore, LibAccounting, LibCommon), can grow its bytecode.
    • A new external function belongs in a facet of its own.
    • If a change brings CoreFacet near the limit, the next step is to move removeRegisteredTask and the removal paths only it reaches into a separate facet.
  • CoreViewFacet: isAutomationEnabled and isAutomationReadyEnabled read the flag through one private helper, so the two views cannot disagree on it.
  • Docs: foundry.toml, LibCommon, AUTOMATION_REGISTRY_GAS_GUIDE.md, the README and SupraContractsBindings.sol describe the new layout. foundry.toml states the measured headroom: CoreFacet goes over the limit from 10,000 runs, RegistryFacet from about 20,000. An upgrade that re-derives the record gas figures replaces CoreFacet (the transition code), CoreViewFacet and ConfigFacet together. GenesisTransactionTags' doc comment says a variant is declared next to related contracts until a genesis is released, and appended at the end after that.

Motivation

CoreFacet is the largest contract in the genesis set. Any fix to it, or to a library it inlines, is redeployed as a fresh CoreFacet (smr-moonshot#2772), and that redeployment has to fit under EIP-170's 24,576-byte limit. Before this change it had 1,063 bytes of headroom under [profile.production] (optimizer_runs = 800).

Contract Before After
CoreFacet 23,513 B (1,063 free) 21,857 B (2,719 free)
CoreViewFacet — 2,266 B

Further reductions measured but not included:

Option CoreFacet saving Cost
Move removeRegisteredTask into its own facet −2,966 B One more facet
Lower CoreFacet's own optimizer runs to 200 −1,059 B +0.38% gas on processTasks
Serve the suspend path from a separate facet −1,985 B One more facet
Both facet moves combined −5,210 B Two more facets

The comment in CoreFacet's header sets the first option as the direction to take when the headroom runs low.

Node impact:

  • Selectors are unchanged and still reached through the diamond, so the node needs no code change.
  • The node matches the Diamond's genesis tag by wildcard, so the one-nonce address shift needs no node change either.
  • When the node is pinned to this version, the doc comment at consensus/execution/src/tests/evm_executor.rs:1189 should say ICoreViewFacet.sol instead of ICoreFacet.sol.

Test plan

  • forge test passes 647/647 under both the default and production profiles.
  • New DiamondInit.t.sol::testCoreFacetSplitSelectorRouting checks that all 7 view selectors route to CoreViewFacet and all 5 state-changing selectors route to CoreFacet. It covers the node-pinned processTasks, monitorCycleEnd and removeRegisteredTask. The facet-count assertions go from 7 to 8.
  • check_facet_selectors.sh passes for all 8 facets, and check_event_indexing.sh passes.
  • cargo fmt --check, cargo clippy --release --workspace and cargo nextest run --release --workspace (556 passed, 1 skipped) all pass, including genesis_contract_bytecode_matches_expected_hashes and deployment_order_is_pinned.
  • The comment and doc changes leave every contract's deployed bytecode unchanged, as does the CoreViewFacet helper (the optimizer inlines it): production builds before and after hash the same.

🤖 Generated with Claude Code

… transition code

CoreFacet is deployed as one of the automation registry's diamond facets, and
any fix to it or to a library it inlines is redeployed as a fresh CoreFacet
(Entropy-Foundation/smr-moonshot#2772) that has to fit under EIP-170's
24,576-byte limit. At [profile.production]'s optimizer_runs = 800 it had
1,063 bytes of headroom.

CoreViewFacet now serves CoreFacet's seven views: getCycleInfo,
getCycleDuration, getTransitionInfo, isAutomationEnabled,
isAutomationReadyEnabled, getCycleStateDetails and getRecordMaxTaskCount.
ICoreViewFacet declares them; ICoreFacet keeps CoreFacet's events, errors
and five state-changing functions. The selectors are reached through the
diamond as before, so the node needs no change. This follows the
RegistryViewFacet layout (Entropy-Foundation/smr-moonshot#4101).

LibCore's transition path is reduced without changing behaviour:
- dropOrChargeTask drops a cancelled or expired task through
  dropWithWholeDeposit, the helper the other drop paths use.
- tryWithdrawTaskAutomationFee writes its outcome into the caller's
  TransitionResult. This keeps dropOrChargeTasks within the EVM stack limit,
  which it was one slot from exceeding.
- dropOrChargeTasks shortens removedBuffer in place to the removed count.

CoreFacet goes from 23,513 to 21,957 bytes (2,619 free); CoreViewFacet is
2,266 bytes. The unused EnumerableSet import is removed from CoreFacet.
CoreFacet's header records that a change to it can grow its bytecode, that a
new external function belongs in a facet of its own, and that moving
removeRegisteredTask into a separate facet is the next step if it nears the
limit.

Genesis wiring: CoreViewFacet gets a GenesisTransactionTags variant declared
and ranked right after CoreFacet, so the Diamond's genesis address moves by
one nonce. FacetsDeployment, the Diamond constructor's facet list,
LibDiamondUtils, the deploy and selector-check scripts and compile_config.toml
include it. The pinned bytecode hashes for CoreFacet, Diamond and
CoreViewFacet are regenerated from a --release build.

The record-sizing gas constants are now compiled into CoreViewFacet and
ConfigFacet, and LibCommon and AUTOMATION_REGISTRY_GAS_GUIDE.md say that an
upgrade changing them replaces those two facets. foundry.toml states the
measured headroom: CoreFacet is over the limit from 10,000 runs and
RegistryFacet from about 20,000.

Tests: DiamondInit.t.sol checks that every CoreFacet and CoreViewFacet
selector is served by its own facet, through a helper the RegistryFacet
routing test now shares. Callers in tests and scripts use ICoreViewFacet for
the moved views.

Verified: forge test passes under the default and production profiles,
check_facet_selectors.sh passes for all 8 facets, check_event_indexing.sh
passes, and cargo build, clippy and nextest --release pass for the
workspace.

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

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

Review at 106d86d.

1. The smr-moonshot Integration book and docs.supra.com need updating when this is pinned (outside this diff). The "Node impact" section lists only the evm_executor.rs doc comment, which is now at line 1197. On smr-moonshot origin/dev, under docs/guide/books/integration/src/network/evm/:

  • system-contracts/automation-registry.md:629-633: the JSON ABI recipe runs forge inspect over ICoreFacet and not ICoreViewFacet, so the ABI it builds lacks the seven views, including isAutomationReadyEnabled, which the getting-started page uses. cast call examples are unaffected because they pass the signature.
  • system-contracts/automation-registry.md:604-617 (interface ICoreFacet block) and :525 (selector map) list the views under CoreFacet.
  • predeployed-contracts.md:182 lists the facets without CoreViewFacet; system-contracts/index.md:55 gives the interface count as 18 files.
  • The Devnet AutomationRegistry address 0xa75A2Cd8e61992526Ed2eeAF1e1ab3b9Ec472400 is hard-coded in predeployed-contracts.md:167, system-contracts/automation-registry.md:35, system-contracts/block-meta.md:40 and automation/automated-transactions-over-rpc.md:82, and in the same four pages on docs.supra.com. CoreViewFacet takes a deployer nonce before Diamond (generator.rs:472), so the address changes at the Devnet reset.

These belong in the pin-bump PR and the docs.supra.com port; please list them in the PR body.

2. foundry.toml contradicts itself. Lines 40-43 say RegistryFacet is out of reach of the EIP-170 limit "at any optimizer_runs value tested"; the new text at line 53 says it goes over the limit from about 20,000 runs. One of the two should be removed.

3. LibDiamond.sol:158: the list of node-relied selectors omits RegistryViewFacet::hasActiveTaskOfType. The node calls it before every automated transaction (smr-moonshot consensus/execution/src/evm/executor.rs:1250, hasActiveTaskOfTypeCall). The omission predates this PR, but the PR edits the list, so it can add the selector.

4. LibCore.sol:858: dropWithWholeDeposit's NatSpec still describes it as removing "a task that a governed configuration no longer admits". dropOrChargeTask now also drops cancelled and expired tasks through it.

5. CoreFacet's header comment (CoreFacet.sol:12-24).

  • It records one build's sizes ("21,957 bytes under the production profile, 2,619 bytes below the limit"), which change with the next edit to CoreFacet, LibCore, LibAccounting or LibCommon. The build already enforces the limit (EIP170_DEPLOYED_CODE_LIMIT), so the figures belong in the commit message (CONTRIBUTING.md, "Comments and Documentation Are Literal").
  • "A new external function therefore belongs in a facet of its own" can be read as one new facet per function. Each new facet adds wiring in about eight places and moves the Diamond's genesis address. "In another facet" states the intent.

6. PR body. "The node matches the Diamond's genesis tag by wildcard" is inaccurate: map_tag_to_contract_name in evm-root-accounts matches GenesisTransactionTags::Diamond explicitly, and its wildcard arm maps CoreViewFacet to Custom("CoreViewFacet"). The conclusion holds, because the node reads the registry address from EvmContractsDetails.

Checked with no finding: the 5 + 7 selectors equal the previous 12 with no collisions, and each moved view makes the same storage reads; FacetsDeployment field order matches between Solidity and Rust; Diamond, LibDiamondUtils, generator.rs and the deployment ranks agree, and deployment_order_is_pinned covers them; the LibCore trims make the same calls, set the same TransitionResult fields on the same paths and keep the event order. Merging with open PR #88 conflicts only in the golden bytecode-hash table in generator.rs; whichever merges second regenerates the hashes. The bytecode hashes and byte counts were not rebuilt for this review.

🤖 Generated with Claude Code

Aregnaz Harutyunyan and others added 3 commits November 13, 2026 18:03
…Facet's size rule without figures

The record gas figures measure the transition code, which LibCore inlines
into CoreFacet, so an upgrade that re-derives them replaces CoreFacet as
well as CoreViewFacet and ConfigFacet. AUTOMATION_REGISTRY_GAS_GUIDE.md and
LibCommon's note on the constants now name all three facets.

CoreFacet's header states the size rule without measured byte counts, which
any change to the facet or its libraries would make stale, and points to
foundry.toml and the build's EIP-170 check instead. It describes
CoreViewFacet as the facet that serves the views.

GenesisTransactionTags' doc comment says a variant is declared next to the
contracts it belongs with until a genesis built with the enum is released,
and appended at the end after that.

Deployed bytecode is unchanged for every contract under the production
profile.

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
…Fee as a fresh struct

tryWithdrawTaskAutomationFee returns its outcome as a TransitionResult that
starts zeroed, and dropOrChargeTask assigns it on the UST path. The outcome
no longer depends on the caller passing a zero-initialised struct, and a
single struct return still keeps dropOrChargeTasks within the EVM stack
limit. On that path nothing writes the result before the call, so the
outcome for every task is unchanged.

CoreFacet goes from 21,957 to 21,857 bytes under the production profile;
its pinned bytecode hash is regenerated from a --release build.

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
…ewFacet

isAutomationEnabled and isAutomationReadyEnabled both report the flag
through a private automationEnabled helper, so the two views share one
definition of "automation is enabled". isAutomationEnabled stays external,
which keeps getSelectors listing it as CoreViewFacet.isAutomationEnabled.

The optimizer inlines the helper: CoreViewFacet's deployed bytecode is
unchanged under the production profile.

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
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.

2 participants