Repository navigation
[#4922] Keep automation task ownership out of the foundation wallet - #88
Conversation
…wner contract it administers does Every action of MultiSignatureWallet goes through submit, confirm and execute, so it carries the confirmations that are valid when it executes. An automation task runs with its owner as the sender on every execution, without a new confirmation, so the wallet is not a task owner. - MultiSignatureWallet no longer declares IAutomationTaskOwner and has no supportsInterface. The registry refuses it in register and registerSystemTask with UnsupportedTaskOwner. - IAutomationTaskOwner states that a task owner is used for nothing except its tasks. - grantAuthorization and registerSystemTask NatSpec name a task owner contract that the wallet administers as the account to authorize. - README: "Automation tasks of the foundation" gives the procedure (a contract on AutomationTaskOwnerBase, deployed through the wallet's deployContract, authorized with AuthorizeAccount) and the rules for that contract. - ContractOwnedTasks tests cover the refusal of the wallet for user and system tasks and the registration, cancellation and access control of a system task owned by a wallet-administered task owner. - The pinned genesis bytecode hash of MultiSignatureWallet is regenerated from a release build; no other contract's bytecode changes. See Entropy-Foundation/smr-moonshot#4922. Co-Authored-By: Claude Fable 5.2 <noreply@anthropic.com>
… the task owner rule without rationale - AuthorizeAccount refuses, before submitting, an account that has code, is not an EIP-7702 delegation and does not declare IAutomationTaskOwner, the rule the registry applies to a task owner. The foundation wallet is such an account. The README's refusal table lists the new condition. - GovActions tests: the foundation wallet is refused, and a contract that declares IAutomationTaskOwner is authorized. - The MultiSignatureWallet NatSpec, the IAutomationTaskOwner obligation and the README section state the rule and reference smr-moonshot#4922 only. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
isaacdoidge
left a comment
There was a problem hiding this comment.
Reviewed at 987ec6c.
1. The smr-moonshot e2e suite and Integration book still treat the wallet as a task owner.
remote_env/integration_suite/evm_automation_tests/__main__.pyregister_system_task_through_foundation_walletauthorizes the wallet, registers a system task from it and asserts the wallet owns it. With this change the registration reverts withUnsupportedTaskOwner(wallet). The test runs in every e2e run (e2e_suite/__main__.py:151).docs/guide/books/integration/src/network/evm/automation/how-automation-works.md:438,447,461anddocs/guide/books/integration/src/network/evm/system-contracts/automation-registry.md:358state that the wallet declaresIAutomationTaskOwnerand owns system tasks.
The smr PR that bumps the revm pin needs to move the e2e test to a wallet-deployed task owner contract and update those pages. "Notes for reviewers" names only the pin bump.
2. The new obligation in IAutomationTaskOwner (src/interfaces/IAutomationTaskOwner.sol:33-36) applies to every contract task owner, not only the foundation's.
IAutomationTaskOwner is the interface third-party contracts declare to own user tasks; the Integration book's getting-started page tells integrators to declare it. "It is used for nothing except its tasks: it is not the owner, admin or role holder of another contract, and holds no funds or allowances beyond those its tasks need" excludes, for example, a contract that holds user funds and registers its own upkeep task, which the registry accepts. The rule is a governance rule for the task owner the foundation wallet administers. Suggest stating it in the README section and the grantAuthorization NatSpec, and leaving the interface's obligations as they were.
3. Steps 1, 2 and 4 of "Automation tasks of the foundation" have no governance action.
submit_governance_action.sh runs the actions in script/GovActions.s.sol (InitializeCycleMonitoring, AuthorizeAccount, EnableDisableAutomation), which cover step 3 only. Writing the contract, deploying it through deployContract, and registering, cancelling and stopping its tasks each require the owners to write Solidity and encode wallet transactions by hand. The example the README names, AdminAutomationTaskOwner, is a test fixture (test/AutomationTaskOwnerBase.t.sol:15-69) with a fixed 100,000 gas, priority 2, a 4 gwei gas price cap and a 60.1 ether fee cap. Suggest a parameterised task owner contract in src/ and governance actions that deploy it and register, cancel and stop through it, so that the procedure runs reviewed code.
4. AuthorizeAccount.canOwnTasks (script/GovActions.s.sol:189-198) is a second copy of LibRegistry.validateTaskOwner (src/libraries/LibRegistry.sol:76-88).
The two match today, and the comment says they must be kept in step. The check also runs only in the script: ConfigFacet.grantAuthorization (src/facets/ConfigFacet.sol:26-32) still accepts the wallet when called directly. An alternative is to enforce the rule in grantAuthorization through a library function both sites share, and drop the script's copy.
…reference SystemTaskOwner - LibTaskOwner holds the registry's task owner rule (#4469). registerTask and ConfigFacet.grantAuthorization both apply it, so grantAuthorization refuses an account the registry would refuse as a task owner. The AuthorizeAccount action calls the same library instead of keeping its own copy. - IAutomationTaskOwner's obligations are back to what they were; the rules for the foundation's own task owner contract are stated in the README and the grantAuthorization NatSpec. - src/SystemTaskOwner.sol is a reference task owner for system tasks: two-step ownership that cannot be renounced, owner-only register, cancel, stop and withdraw, and refusal of native SUPRA. It is not deployed at genesis; the governance actions that deploy and drive it are written once the foundation has a concrete system task. - Tests: the reference contract, the wallet procedure through it, and the registration-time check of an authorized submitter. - Pinned genesis bytecode hashes of ConfigFacet and RegistryFacet updated. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
|
Thanks for the review. Addressed in 49da227: 1. smr e2e suite and Integration book. Tracked in Entropy-Foundation/smr-moonshot#4996, the follow-up that moves the node to the next revm rc. It lists 2. 3. Steps without a governance action. Partly taken. 4. Duplicate task owner rule. Done as suggested: |
isaacdoidge
left a comment
There was a problem hiding this comment.
Reviewed at 49da227. The four points from the previous review are addressed; the governance actions for SystemTaskOwner are deferred as the README and the PR description state.
Merge conflict. The branch conflicts with feature/evm_automation after #85 and #87 merged. git merge-tree reports one conflicting file, crates/supra-extension/src/contracts/generator.rs, in the pinned genesis bytecode hashes. ConfigFacet.sol, RegistryFacet.sol and LibRegistry.sol merge textually, but #85 also changed those contracts, so the merged hashes are neither side's: regenerate ConfigFacet, RegistryFacet and MultiSignatureWallet from a release build of the merged tree, and rerun forge test and genesis_contract_bytecode_matches_expected_hashes there.
UnsupportedTaskOwner from ConfigFacet. grantAuthorization now reverts with IRegistryFacet.UnsupportedTaskOwner, which IConfigFacet does not declare (IRegistryFacet.sol:102). A client decoding with the IConfigFacet ABI does not recognise the error. Declaring it in IConfigFacet as well, or listing it under grantAuthorization in the Integration book's error table (smr-moonshot#4996), would cover it.
…orization reverts with The same error as IRegistryFacet.UnsupportedTaskOwner, with the same selector, so that a client decoding with the IConfigFacet ABI recognises it. A test pins the two selectors together. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
|
Thanks. Both addressed: Merge conflict.
|
Summary
MultiSignatureWalletis no longer an automation task owner. Tasks the foundation wants registered are owned by a separate task owner contract that the wallet administers.MultiSignatureWalletno longer declaresIAutomationTaskOwnerand has nosupportsInterface. The registry refuses it as the owner of a user task and of a system task withUnsupportedTaskOwner.src/libraries/LibTaskOwner.solholds the registry's task owner rule (#4469): an account without code, an EIP-7702 delegated account, or a contract that declaresIAutomationTaskOwnerthrough ERC-165.LibRegistry.registerTaskandConfigFacet.grantAuthorizationboth apply it, sograntAuthorizationnow refuses an account the registry would refuse as a task owner, the wallet included;IConfigFacetdeclares the error (UnsupportedTaskOwner, the registry's selector) so the ConfigFacet ABI decodes it. TheAuthorizeAccountgovernance action checks the same library before submitting, instead of keeping its own copy.IAutomationTaskOwneris unchanged from the base branch. The rules for the foundation's own task owner contract are a governance rule, stated in the README and thegrantAuthorizationNatSpec: governance may authorize any account the registry accepts, never one that holds authority over other contracts, and the foundation's own system tasks go through a contract the wallet administers and uses for nothing else.src/SystemTaskOwner.sol, a reference task owner for system tasks onAutomationTaskOwnerBase:Ownable2Stepownership that cannot be renounced; owner-onlyregisterSystemTask(all of the registry's parameters),cancelSystemTasks,stopSystemTasksandwithdrawFunds; native SUPRA refused, since system tasks are charged no fees. It is not deployed at genesis.solidity/supra_contracts/README.md, "Automation tasks of the foundation": the procedure (write the contract, withSystemTaskOwneras the reference; deploy it through the wallet'sdeployContractso that the wallet is its owner; authorize it withAuthorizeAccount; register, cancel and stop through wallet transactions to it) and the rules for that contract and its tasks.crates/supra-extension/src/contracts/generator.rsregenerated from a release build of the tree merged withfeature/evm_automation([#4921] Price EVM user tasks on task occupancy as well as gas occupancy #85, [#4936] Keep BlockMeta ownership recoverable: two-step transfer, no renunciation #87):MultiSignatureWallet,ConfigFacet,RegistryFacet.Motivation
The foundation wallet's authority stays inside its submit, confirm and execute flow; automation tasks are owned by a dedicated contract the wallet administers. The analysis is in the tracking issue: Entropy-Foundation/smr-moonshot#4922.
Notes for reviewers
SystemTaskOwneryet. They are written once the foundation has a concrete system task, against that task's requirements; until then the contract is reviewed reference code with tests, andtest/ContractOwnedTasks.t.solruns the whole procedure through wallet transactions.supportsInterface, and the genesis bytecode of the wallet,ConfigFacetandRegistryFacetchanges. The node-side follow-up (new revm rc pin, the e2e test that registers a system task from the wallet, the Integration book pages) is tracked in Entropy-Foundation/smr-moonshot#4996.Test plan
forge testinsolidity/supra_contracts, default andproductionprofiles: 661 passed, 0 failed (on the merged tree).test/ContractOwnedTasks.t.sol:IERC165norIAutomationTaskOwner;grantAuthorizationrefuses the wallet and a contract that does not declareIAutomationTaskOwner, with the errorIConfigFacetdeclares;SystemTaskOwneris owned by that contract, and the wallet cancels it through it;test/SystemTaskOwner.t.sol: ownership and registry binding, the declared interfaces, register and stop by the owner, every acting function owner-only (including a call from the contract itself), two-step ownership, renunciation disabled, native SUPRA refused, WSUPRA withdrawn by the owner.test/GovActions.t.sol:AuthorizeAccountrefuses the foundation wallet and authorizes a contract that declaresIAutomationTaskOwner.script/check_facet_selectors.shcargo build --release --workspacecargo clippy --release --workspace --all-targets --all-featurescargo nextest run --release --workspace: 556 passed, 1 skipped (includesgenesis_contract_bytecode_matches_expected_hashes)🤖 Generated with Claude Code