From 3bf937902b371801d3922140b53a38986042b203 Mon Sep 17 00:00:00 2001 From: SidestreamColdMelon Date: Tue, 21 Oct 2025 10:58:32 +0200 Subject: [PATCH 1/8] add StarGuard support to both checklist, rename SubDAOs to Agents --- spell/spell-crafter-mainnet-workflow.md | 7 ++++ spell/spell-reviewer-mainnet-checklist.md | 47 ++++++++++++++--------- 2 files changed, 36 insertions(+), 18 deletions(-) diff --git a/spell/spell-crafter-mainnet-workflow.md b/spell/spell-crafter-mainnet-workflow.md index a43564c9..3572445b 100644 --- a/spell/spell-crafter-mainnet-workflow.md +++ b/spell/spell-crafter-mainnet-workflow.md @@ -125,6 +125,13 @@ Repo: https://github.com/makerdao/spells-mainnet * [ ] Changes are tested via `testNewOrUpdatedChainlogValues` * [ ] Adjust system values, collateral values inside `config.sol` * [ ] Ensure every spell variable is declared as public/internal + * IF Agent spell is provided + * [ ] Handover message matches `XXX spell YYYY-MM-DD deployed to 0x… with hash 0x…, direct execution: yes / no` template + * [ ] IF `direct execution` is `no` + * [ ] The spell is plotted using `StarGuardLike(XXX_STARGUARD).plot(XXX_SPELL, XXX_SPELL_HASH)` + * [ ] IF `direct execution` is `yes` + * [ ] The hash is checked via `require(XXX_SPELL.codehash == XXX_SPELL_HASH, "XXX_SPELL/wrong-codehash");` + * [ ] The spell is executed via `ProxyLike(XXX_PROXY).exec(XXX_SPELL, abi.encodeWithSignature("execute()"));` * Add specific tests in `DssSpell.t.sol` to have sufficient test coverage for every spell action * [ ] Test new collaterals * [ ] Test new ilk registry values diff --git a/spell/spell-reviewer-mainnet-checklist.md b/spell/spell-reviewer-mainnet-checklist.md index beee060b..9042a377 100644 --- a/spell/spell-reviewer-mainnet-checklist.md +++ b/spell/spell-reviewer-mainnet-checklist.md @@ -241,27 +241,38 @@ * [ ] `MCD_VEST_MKR_TREASURY` chainlog address is used for MKR stream `yank` * [ ] `MCD_VEST_DAI` chainlog address is used for DAI stream `yank` * [ ] Tested via `testYankDAI` or `testYankMKR` -* IF SubDAO-related content is present - * IF SubDAO provides SubProxy spell address - * [ ] SubDAO spell address matches Exec Sheet - * [ ] Executed via `ProxyLike(SUBDAO_PROXY).exec(SUBDAO_SPELL, abi.encodeWithSignature("execute()"));` +* IF Agent-related content is present + * IF Agent spell is provided + * [ ] Handover message matches `XXX spell YYYY-MM-DD deployed to 0x… with hash 0x…, direct execution: yes / no` template + * [ ] IF `direct execution` is `no` + * [ ] The spell is plotted using `StarGuardLike(XXX_STARGUARD).plot(XXX_SPELL, XXX_SPELL_HASH)` + * [ ] `XXX` in `XXX_STARGUARD` matches the name of the Agent + * [ ] `XXX_STARGUARD` is fetched from chainlog + * [ ] The test ensures the spell is executable via `StarGuardLike(XXX_STARGUARD).exec()` before `XXX_STARGUARD.maxDelay` + * [ ] IF `direct execution` is `yes` + * [ ] Provided mandatory explanation of why direct execution is required makes sense on the technical level + * [ ] The hash is checked via `require(XXX_SPELL.codehash == XXX_SPELL_HASH, "XXX_SPELL/wrong-codehash");` + * [ ] The spell is executed via `ProxyLike(XXX_PROXY).exec(XXX_SPELL, abi.encodeWithSignature("execute()"));` + * [ ] `XXX` in `XXX_PROXY` matches the name of the Agent + * [ ] `XXX_PROXY` is fetched from chainlog + * [ ] Agent spell address (`XXX_SPELL`) matches Exec Sheet + * [ ] Agent spell hash (`XXX_SPELL_HASH`) matches Exec Sheet * [ ] Execution is NOT delegate call - * [ ] IF SubDAO spell deployer is a smart contract (e.g. multisig or factory), ensure the deployer address is in `addresses_deployers.sol` as an entry - * [ ] Ensure that SubDAO spell have enough gas and does not revert with "out of gas" error inside simulation. Note: low level call gas estimation is not done by our scripts - * IF SubDAO provides instructions to be executed by the main spell (i.e. that will operate within Pause Proxy `DelegateCall` context) - * [ ] No SubDAO contract being interacted with is authed on a core contract like `vat`, etc. (Check comprehensively where the risk is high) - * [ ] SubDAO contract licensing and optimizations generally do not matter (except where they pose a security risk) - * [ ] SubDAO contracts and all libraries / dependencies have verified source code (Blocking) - * Upgradable SubDAO contracts + * [ ] IF Agent spell deployer is a smart contract (e.g. multisig or factory), ensure the deployer address is in `addresses_deployers.sol` as an entry + * IF Agent provides instructions to be executed by the main spell (i.e. that will operate within Pause Proxy `DelegateCall` context) + * [ ] No Agent contract being interacted with is authed on a core contract like `vat`, etc. (Check comprehensively where the risk is high) + * [ ] Agent contract licensing and optimizations generally do not matter (except where they pose a security risk) + * [ ] Agent contracts and all libraries / dependencies have verified source code (Blocking) + * Upgradable Agent contracts * [ ] Upgradable contracts have the `PAUSE_PROXY` as their `admin` (i.e. the party that can upgrade) - * [ ] Any upgradable SubDAO contracts with an `admin` that is not `PAUSE_PROXY` are not authed on any core contracts (Blocking) - * [ ] All SubDAO content addresses (i.e. provided contract addresses or EOAs) present in the Maker Core spell are present in the Exec Sheet and are correct. SubDAO addresses being authed or given any permissions MUST be in the Exec Sheet. SubDAO addresses being called must be confirmed by the SubDAO spell team. - * [ ] IF addresses not PR'ed in by the SubDAO team (use git blame for example), SubDAO content addresses all have inline comment for provenance or source being OKed by SubDAO - * [ ] SubDAO actions match Exec Sheet (only where inline with main spell code) and do not affect core contracts + * [ ] Any upgradable Agent contracts with an `admin` that is not `PAUSE_PROXY` are not authed on any core contracts (Blocking) + * [ ] All Agent content addresses (i.e. provided contract addresses or EOAs) present in the Maker Core spell are present in the Exec Sheet and are correct. Agent addresses being authed or given any permissions MUST be in the Exec Sheet. Agent addresses being called must be confirmed by the Agent spell team. + * [ ] IF addresses not PR'ed in by the Agent team (use git blame for example), Agent content addresses all have inline comment for provenance or source being OKed by the Agent + * [ ] Agent actions match Exec Sheet (only where inline with main spell code) and do not affect core contracts * [ ] Core contract knock-on actions (such as offboarding or setting DC to 0) are present in the exec and match the code - * [ ] External calls for SubDAO content are NOT delegate call - * [ ] Code does not have untoward behavior within the scope of Maker Core Contracts (e.g. up to the SubDAO proxy) -* IF external contracts calls are present (Not SubDAOs, e.g. Starknet) + * [ ] External calls for Agent content are NOT delegate call + * [ ] Code does not have untoward behavior within the scope of Maker Core Contracts (e.g. up to the Agent proxy) +* IF external contracts calls are present (Not Agents, e.g. Starknet) * [ ] Target Contract doesn't block spell execution * [ ] External call is NOT `delegatecall` * [ ] Target Contract doesn't have permissions on the Vat From 7d3e2cc068849ab240bd5ee9cb3d3cea1de53028 Mon Sep 17 00:00:00 2001 From: SidestreamColdMelon Date: Tue, 21 Oct 2025 12:45:36 +0200 Subject: [PATCH 2/8] use Prime Agent to be more specific --- spell/spell-reviewer-mainnet-checklist.md | 38 +++++++++++------------ 1 file changed, 19 insertions(+), 19 deletions(-) diff --git a/spell/spell-reviewer-mainnet-checklist.md b/spell/spell-reviewer-mainnet-checklist.md index 9042a377..bc28990c 100644 --- a/spell/spell-reviewer-mainnet-checklist.md +++ b/spell/spell-reviewer-mainnet-checklist.md @@ -241,38 +241,38 @@ * [ ] `MCD_VEST_MKR_TREASURY` chainlog address is used for MKR stream `yank` * [ ] `MCD_VEST_DAI` chainlog address is used for DAI stream `yank` * [ ] Tested via `testYankDAI` or `testYankMKR` -* IF Agent-related content is present - * IF Agent spell is provided +* IF content related to a Prime Agent is present + * IF Prime spell is provided * [ ] Handover message matches `XXX spell YYYY-MM-DD deployed to 0x… with hash 0x…, direct execution: yes / no` template * [ ] IF `direct execution` is `no` * [ ] The spell is plotted using `StarGuardLike(XXX_STARGUARD).plot(XXX_SPELL, XXX_SPELL_HASH)` - * [ ] `XXX` in `XXX_STARGUARD` matches the name of the Agent + * [ ] `XXX` in `XXX_STARGUARD` matches the name of the Prime Agent * [ ] `XXX_STARGUARD` is fetched from chainlog * [ ] The test ensures the spell is executable via `StarGuardLike(XXX_STARGUARD).exec()` before `XXX_STARGUARD.maxDelay` * [ ] IF `direct execution` is `yes` * [ ] Provided mandatory explanation of why direct execution is required makes sense on the technical level * [ ] The hash is checked via `require(XXX_SPELL.codehash == XXX_SPELL_HASH, "XXX_SPELL/wrong-codehash");` * [ ] The spell is executed via `ProxyLike(XXX_PROXY).exec(XXX_SPELL, abi.encodeWithSignature("execute()"));` - * [ ] `XXX` in `XXX_PROXY` matches the name of the Agent + * [ ] `XXX` in `XXX_PROXY` matches the name of the Prime Agent * [ ] `XXX_PROXY` is fetched from chainlog - * [ ] Agent spell address (`XXX_SPELL`) matches Exec Sheet - * [ ] Agent spell hash (`XXX_SPELL_HASH`) matches Exec Sheet + * [ ] Prime spell address (`XXX_SPELL`) matches Exec Sheet + * [ ] Prime spell hash (`XXX_SPELL_HASH`) matches Exec Sheet * [ ] Execution is NOT delegate call - * [ ] IF Agent spell deployer is a smart contract (e.g. multisig or factory), ensure the deployer address is in `addresses_deployers.sol` as an entry - * IF Agent provides instructions to be executed by the main spell (i.e. that will operate within Pause Proxy `DelegateCall` context) - * [ ] No Agent contract being interacted with is authed on a core contract like `vat`, etc. (Check comprehensively where the risk is high) - * [ ] Agent contract licensing and optimizations generally do not matter (except where they pose a security risk) - * [ ] Agent contracts and all libraries / dependencies have verified source code (Blocking) - * Upgradable Agent contracts + * [ ] IF Prime spell deployer is a smart contract (e.g. multisig or factory), ensure the deployer address is in `addresses_deployers.sol` as an entry + * IF Prime Agent provides instructions to be executed by the main spell (i.e. that will operate within Pause Proxy `DelegateCall` context) + * [ ] No Prime Agent contract being interacted with is authed on a core contract like `vat`, etc. (Check comprehensively where the risk is high) + * [ ] Prime Agent contract licensing and optimizations generally do not matter (except where they pose a security risk) + * [ ] Prime Agent contracts and all libraries / dependencies have verified source code (Blocking) + * Upgradable Prime Agent contracts * [ ] Upgradable contracts have the `PAUSE_PROXY` as their `admin` (i.e. the party that can upgrade) - * [ ] Any upgradable Agent contracts with an `admin` that is not `PAUSE_PROXY` are not authed on any core contracts (Blocking) - * [ ] All Agent content addresses (i.e. provided contract addresses or EOAs) present in the Maker Core spell are present in the Exec Sheet and are correct. Agent addresses being authed or given any permissions MUST be in the Exec Sheet. Agent addresses being called must be confirmed by the Agent spell team. - * [ ] IF addresses not PR'ed in by the Agent team (use git blame for example), Agent content addresses all have inline comment for provenance or source being OKed by the Agent - * [ ] Agent actions match Exec Sheet (only where inline with main spell code) and do not affect core contracts + * [ ] Any upgradable Prime Agent contracts with an `admin` that is not `PAUSE_PROXY` are not authed on any core contracts (Blocking) + * [ ] All Prime Agent content addresses (i.e. provided contract addresses or EOAs) present in the Maker Core spell are present in the Exec Sheet and are correct. Prime Agent addresses being authed or given any permissions MUST be in the Exec Sheet. Prime Agent addresses being called must be confirmed by the Prime Agent spell team. + * [ ] IF addresses not PR'ed in by the Prime Agent team (use git blame for example), Prime Agent content addresses all have inline comment for provenance or source being OKed by the Prime Agent + * [ ] Prime Agent actions match Exec Sheet (only where inline with main spell code) and do not affect core contracts * [ ] Core contract knock-on actions (such as offboarding or setting DC to 0) are present in the exec and match the code - * [ ] External calls for Agent content are NOT delegate call - * [ ] Code does not have untoward behavior within the scope of Maker Core Contracts (e.g. up to the Agent proxy) -* IF external contracts calls are present (Not Agents, e.g. Starknet) + * [ ] External calls for Prime Agent content are NOT delegate call + * [ ] Code does not have untoward behavior within the scope of Maker Core Contracts (e.g. up to the Prime Agent proxy) +* IF external contracts calls are present (Not Prime Agents, e.g. Starknet) * [ ] Target Contract doesn't block spell execution * [ ] External call is NOT `delegatecall` * [ ] Target Contract doesn't have permissions on the Vat From cc3d405ab1b71ac3814cbe3b605cb00440bdccf5 Mon Sep 17 00:00:00 2001 From: SidestreamColdMelon Date: Tue, 21 Oct 2025 16:38:47 +0200 Subject: [PATCH 3/8] address review, improve --- spell/spell-crafter-mainnet-workflow.md | 4 ++-- spell/spell-reviewer-mainnet-checklist.md | 9 +++++---- 2 files changed, 7 insertions(+), 6 deletions(-) diff --git a/spell/spell-crafter-mainnet-workflow.md b/spell/spell-crafter-mainnet-workflow.md index 3572445b..7115da60 100644 --- a/spell/spell-crafter-mainnet-workflow.md +++ b/spell/spell-crafter-mainnet-workflow.md @@ -125,12 +125,12 @@ Repo: https://github.com/makerdao/spells-mainnet * [ ] Changes are tested via `testNewOrUpdatedChainlogValues` * [ ] Adjust system values, collateral values inside `config.sol` * [ ] Ensure every spell variable is declared as public/internal - * IF Agent spell is provided + * IF Prime Agent spell is provided * [ ] Handover message matches `XXX spell YYYY-MM-DD deployed to 0x… with hash 0x…, direct execution: yes / no` template * [ ] IF `direct execution` is `no` * [ ] The spell is plotted using `StarGuardLike(XXX_STARGUARD).plot(XXX_SPELL, XXX_SPELL_HASH)` * [ ] IF `direct execution` is `yes` - * [ ] The hash is checked via `require(XXX_SPELL.codehash == XXX_SPELL_HASH, "XXX_SPELL/wrong-codehash");` + * [ ] The hash is checked via `require(XXX_SPELL.codehash == XXX_SPELL_HASH, "XXX_SPELL/wrong-codehash");` inside Core spell * [ ] The spell is executed via `ProxyLike(XXX_PROXY).exec(XXX_SPELL, abi.encodeWithSignature("execute()"));` * Add specific tests in `DssSpell.t.sol` to have sufficient test coverage for every spell action * [ ] Test new collaterals diff --git a/spell/spell-reviewer-mainnet-checklist.md b/spell/spell-reviewer-mainnet-checklist.md index bc28990c..40f4f85a 100644 --- a/spell/spell-reviewer-mainnet-checklist.md +++ b/spell/spell-reviewer-mainnet-checklist.md @@ -245,14 +245,15 @@ * IF Prime spell is provided * [ ] Handover message matches `XXX spell YYYY-MM-DD deployed to 0x… with hash 0x…, direct execution: yes / no` template * [ ] IF `direct execution` is `no` - * [ ] The spell is plotted using `StarGuardLike(XXX_STARGUARD).plot(XXX_SPELL, XXX_SPELL_HASH)` + * [ ] The Prime spell is plotted using `StarGuardLike(XXX_STARGUARD).plot(XXX_SPELL, XXX_SPELL_HASH)` * [ ] `XXX` in `XXX_STARGUARD` matches the name of the Prime Agent * [ ] `XXX_STARGUARD` is fetched from chainlog - * [ ] The test ensures the spell is executable via `StarGuardLike(XXX_STARGUARD).exec()` before `XXX_STARGUARD.maxDelay` + * [ ] The test ensures the `XXX_SPELL` Prime spell is executable via `StarGuardLike(XXX_STARGUARD).exec()` before `XXX_STARGUARD.maxDelay` + * [ ] IF plotted but not yet executed spell is still present in the `XXX_STARGUARD`, Governance Facilitators are aware, already notified * [ ] IF `direct execution` is `yes` * [ ] Provided mandatory explanation of why direct execution is required makes sense on the technical level - * [ ] The hash is checked via `require(XXX_SPELL.codehash == XXX_SPELL_HASH, "XXX_SPELL/wrong-codehash");` - * [ ] The spell is executed via `ProxyLike(XXX_PROXY).exec(XXX_SPELL, abi.encodeWithSignature("execute()"));` + * [ ] The hash is checked via `require(XXX_SPELL.codehash == XXX_SPELL_HASH, "XXX_SPELL/wrong-codehash");` inside the Core spell + * [ ] The Prime spell is executed via `ProxyLike(XXX_PROXY).exec(XXX_SPELL, abi.encodeWithSignature("execute()"));` * [ ] `XXX` in `XXX_PROXY` matches the name of the Prime Agent * [ ] `XXX_PROXY` is fetched from chainlog * [ ] Prime spell address (`XXX_SPELL`) matches Exec Sheet From 7ea352c03fdeb4f6ed7df42f8e59ddcf54e52e60 Mon Sep 17 00:00:00 2001 From: SidestreamColdMelon Date: Tue, 21 Oct 2025 17:58:10 +0200 Subject: [PATCH 4/8] expand star-spell-reviewer-checklist with new items --- spell/star-spell-reviewer-checklist.md | 16 +++++++++++++++- 1 file changed, 15 insertions(+), 1 deletion(-) diff --git a/spell/star-spell-reviewer-checklist.md b/spell/star-spell-reviewer-checklist.md index 096d7e2b..adde0fd6 100644 --- a/spell/star-spell-reviewer-checklist.md +++ b/spell/star-spell-reviewer-checklist.md @@ -148,6 +148,15 @@ This section outlines the review process and provides concrete action items for - [CHAIN_NAME] `0xADDRESS`, EXTERNAL_SOURCE_URL - [ ] Matches valid external source (previously approved forum post, external docs, etc). +### StarGuard execution +- [ ] IF a [StarGuard module](https://github.com/sky-ecosystem/star-guard) is onboarded for this Star, the following additional checks are done: +- [ ] The spell exposes view-only interface `function isExecutable() external view returns (bool result)`. +- [ ] `isExecutable` either simply returns `true` or implements additional logic communicated via the relevant forum post (e.g.: by describing "earliest launch date" or "office hours" logic, etc). +- [ ] The test is preset to ensure that `isExecutable` outputs `true` before `StarGuard.maxDelay()` exires the spell. +- [ ] Third-party actors can not take advantage of the fact that Spell will be executed in a later block than the Core spell, otherwise suggest `direct execution`. +- [ ] IF Prime spell can not be executed in a later block OR have to be executed sequentially to another Prime spell, `direct execution` is clearly proposed in the forum post together with elaborated explanation why it is needed. +- [ ] The `direct execution` explanation makes sense on the technical level and can not be circumvented by the use of `isExecutable()` interface. + #### On-boarding New Contracts - LIST every new contract present in the spell: - [CHAIN_NAME] `CONTRACT_NAME`, LINK_TO_THE_DEPLOYED_CONTRACT @@ -240,6 +249,8 @@ EXECUTED_TESTS_LOGS #### Deployed Contract - [ ] Both reviewers gave explicit "Good to deploy". - [ ] A new comment in the PR contains link to the deployed spell(s) and Tenderly vnet(s). +- [ ] The comment also contains codehash of the deployed mainnet spell. +- [ ] The codehash matches one produced locally from the reviewed source code. - [ ] Every spell is verified on Etherscan or other primary block explorer for this chain. - [ ] Every spell code matches local source code at the "good to deploy" commit. - [ ] Etherscan settings (optimizer, EVM version, license) match local ones. @@ -264,7 +275,10 @@ EXECUTED_TESTS_LOGS #### Confirmed Handover - [ ] Both reviewers gave explicit "Good to handover". - [ ] All review comments have been addressed or resolved. -- [ ] The spell address posted by the crafter in the `#govops` thread matches the spell evaluated above. +- [ ] The spell address, the codehash and the direct execution are posted by the crafter in the `#govops` in the `XXX spell YYYY-MM-DD deployed to 0x… with hash 0x…, direct execution: yes / no` format. +- [ ] Posted spell address matches the spell evaluated above. +- [ ] Posted spell codehash matches the one noted down above. +- [ ] Posted direct execution matches expected value. - [ ] Confirm the address (via a separate "reply to" message, restating the address to avoid edits). - [ ] Ensure that no changes were made to the code since the spell was deployed and archived. - [ ] Approve spell PR for merge via 'Approve' review option. From 61630f4766910cdc9ce72155c21d9139b666e346 Mon Sep 17 00:00:00 2001 From: SidestreamColdMelon Date: Wed, 22 Oct 2025 09:29:37 +0200 Subject: [PATCH 5/8] fix spellcheck --- .wordlist.txt | 2 ++ spell/star-spell-reviewer-checklist.md | 2 +- 2 files changed, 3 insertions(+), 1 deletion(-) diff --git a/.wordlist.txt b/.wordlist.txt index 974b858d..099e266c 100644 --- a/.wordlist.txt +++ b/.wordlist.txt @@ -68,6 +68,7 @@ Repo Solmate SPDX StairstepExponentialDecrease +StarGuard Starknet SubDAO SubDAOs @@ -90,6 +91,7 @@ authing bytecode casted checksummed +codehash collateralization collaterals config diff --git a/spell/star-spell-reviewer-checklist.md b/spell/star-spell-reviewer-checklist.md index adde0fd6..d8f00bce 100644 --- a/spell/star-spell-reviewer-checklist.md +++ b/spell/star-spell-reviewer-checklist.md @@ -152,7 +152,7 @@ This section outlines the review process and provides concrete action items for - [ ] IF a [StarGuard module](https://github.com/sky-ecosystem/star-guard) is onboarded for this Star, the following additional checks are done: - [ ] The spell exposes view-only interface `function isExecutable() external view returns (bool result)`. - [ ] `isExecutable` either simply returns `true` or implements additional logic communicated via the relevant forum post (e.g.: by describing "earliest launch date" or "office hours" logic, etc). -- [ ] The test is preset to ensure that `isExecutable` outputs `true` before `StarGuard.maxDelay()` exires the spell. +- [ ] The test is preset to ensure that `isExecutable` outputs `true` before `StarGuard.maxDelay()` expires the spell. - [ ] Third-party actors can not take advantage of the fact that Spell will be executed in a later block than the Core spell, otherwise suggest `direct execution`. - [ ] IF Prime spell can not be executed in a later block OR have to be executed sequentially to another Prime spell, `direct execution` is clearly proposed in the forum post together with elaborated explanation why it is needed. - [ ] The `direct execution` explanation makes sense on the technical level and can not be circumvented by the use of `isExecutable()` interface. From 4931fbc859240fe302ffea60faa601fd3250cedf Mon Sep 17 00:00:00 2001 From: SidestreamColdMelon Date: Wed, 22 Oct 2025 14:16:28 +0200 Subject: [PATCH 6/8] apply suggestions --- spell/star-spell-reviewer-checklist.md | 14 +++++++------- 1 file changed, 7 insertions(+), 7 deletions(-) diff --git a/spell/star-spell-reviewer-checklist.md b/spell/star-spell-reviewer-checklist.md index d8f00bce..ea784fe2 100644 --- a/spell/star-spell-reviewer-checklist.md +++ b/spell/star-spell-reviewer-checklist.md @@ -150,12 +150,12 @@ This section outlines the review process and provides concrete action items for ### StarGuard execution - [ ] IF a [StarGuard module](https://github.com/sky-ecosystem/star-guard) is onboarded for this Star, the following additional checks are done: -- [ ] The spell exposes view-only interface `function isExecutable() external view returns (bool result)`. -- [ ] `isExecutable` either simply returns `true` or implements additional logic communicated via the relevant forum post (e.g.: by describing "earliest launch date" or "office hours" logic, etc). -- [ ] The test is preset to ensure that `isExecutable` outputs `true` before `StarGuard.maxDelay()` expires the spell. -- [ ] Third-party actors can not take advantage of the fact that Spell will be executed in a later block than the Core spell, otherwise suggest `direct execution`. -- [ ] IF Prime spell can not be executed in a later block OR have to be executed sequentially to another Prime spell, `direct execution` is clearly proposed in the forum post together with elaborated explanation why it is needed. -- [ ] The `direct execution` explanation makes sense on the technical level and can not be circumvented by the use of `isExecutable()` interface. + - [ ] The spell exposes view-only interface `function isExecutable() external view returns (bool result)`. + - [ ] `isExecutable` either simply returns `true` or implements additional logic communicated via the relevant forum post (e.g.: by describing "earliest launch date" or "office hours" logic, etc). + - [ ] The test ensures the spell is executable before expiration (i.e. `isExecutable` outputs `true` before `StarGuard.maxDelay()` is passed). + - [ ] Third-party actors can not take advantage of the fact that Spell will be executed in a later block than the Core spell, otherwise suggest `direct execution`. + - [ ] IF Prime spell can not be executed in a later block OR have to be executed sequentially to another Prime spell, `direct execution` is clearly mentioned in the forum post together with elaborated explanation why it is needed. + - [ ] The `direct execution` explanation makes sense on the technical level and can not be circumvented by the use of `isExecutable()` interface. #### On-boarding New Contracts - LIST every new contract present in the spell: @@ -278,7 +278,7 @@ EXECUTED_TESTS_LOGS - [ ] The spell address, the codehash and the direct execution are posted by the crafter in the `#govops` in the `XXX spell YYYY-MM-DD deployed to 0x… with hash 0x…, direct execution: yes / no` format. - [ ] Posted spell address matches the spell evaluated above. - [ ] Posted spell codehash matches the one noted down above. -- [ ] Posted direct execution matches expected value. +- [ ] Posted direct execution value matches the forum post. - [ ] Confirm the address (via a separate "reply to" message, restating the address to avoid edits). - [ ] Ensure that no changes were made to the code since the spell was deployed and archived. - [ ] Approve spell PR for merge via 'Approve' review option. From 75e4a86594e613b4393c2c25ece31ccfe1300d37 Mon Sep 17 00:00:00 2001 From: SidestreamColdMelon Date: Thu, 30 Oct 2025 19:10:48 +0100 Subject: [PATCH 7/8] address feedback --- spell/spell-crafter-mainnet-workflow.md | 4 +-- spell/spell-reviewer-mainnet-checklist.md | 16 +++++----- spell/star-spell-reviewer-checklist.md | 38 +++++++++++------------ 3 files changed, 29 insertions(+), 29 deletions(-) diff --git a/spell/spell-crafter-mainnet-workflow.md b/spell/spell-crafter-mainnet-workflow.md index 7115da60..2326103f 100644 --- a/spell/spell-crafter-mainnet-workflow.md +++ b/spell/spell-crafter-mainnet-workflow.md @@ -128,10 +128,10 @@ Repo: https://github.com/makerdao/spells-mainnet * IF Prime Agent spell is provided * [ ] Handover message matches `XXX spell YYYY-MM-DD deployed to 0x… with hash 0x…, direct execution: yes / no` template * [ ] IF `direct execution` is `no` - * [ ] The spell is plotted using `StarGuardLike(XXX_STARGUARD).plot(XXX_SPELL, XXX_SPELL_HASH)` + * [ ] The Prime Agent spell is plotted using `StarGuardLike(XXX_STARGUARD).plot(XXX_SPELL, XXX_SPELL_HASH)` * [ ] IF `direct execution` is `yes` * [ ] The hash is checked via `require(XXX_SPELL.codehash == XXX_SPELL_HASH, "XXX_SPELL/wrong-codehash");` inside Core spell - * [ ] The spell is executed via `ProxyLike(XXX_PROXY).exec(XXX_SPELL, abi.encodeWithSignature("execute()"));` + * [ ] The Prime Agent spell is executed via `ProxyLike(XXX_PROXY).exec(XXX_SPELL, abi.encodeWithSignature("execute()"));` * Add specific tests in `DssSpell.t.sol` to have sufficient test coverage for every spell action * [ ] Test new collaterals * [ ] Test new ilk registry values diff --git a/spell/spell-reviewer-mainnet-checklist.md b/spell/spell-reviewer-mainnet-checklist.md index 40f4f85a..896c4f00 100644 --- a/spell/spell-reviewer-mainnet-checklist.md +++ b/spell/spell-reviewer-mainnet-checklist.md @@ -242,24 +242,24 @@ * [ ] `MCD_VEST_DAI` chainlog address is used for DAI stream `yank` * [ ] Tested via `testYankDAI` or `testYankMKR` * IF content related to a Prime Agent is present - * IF Prime spell is provided + * IF Prime Agent spell is provided * [ ] Handover message matches `XXX spell YYYY-MM-DD deployed to 0x… with hash 0x…, direct execution: yes / no` template * [ ] IF `direct execution` is `no` - * [ ] The Prime spell is plotted using `StarGuardLike(XXX_STARGUARD).plot(XXX_SPELL, XXX_SPELL_HASH)` + * [ ] The Prime Agent spell is plotted using `StarGuardLike(XXX_STARGUARD).plot(XXX_SPELL, XXX_SPELL_HASH)` * [ ] `XXX` in `XXX_STARGUARD` matches the name of the Prime Agent * [ ] `XXX_STARGUARD` is fetched from chainlog - * [ ] The test ensures the `XXX_SPELL` Prime spell is executable via `StarGuardLike(XXX_STARGUARD).exec()` before `XXX_STARGUARD.maxDelay` - * [ ] IF plotted but not yet executed spell is still present in the `XXX_STARGUARD`, Governance Facilitators are aware, already notified + * [ ] The test ensures the `XXX_SPELL` Prime Agent spell is executable via `StarGuardLike(XXX_STARGUARD).exec()` before `XXX_STARGUARD.maxDelay` + * [ ] IF plotted but not yet executed spell is still present in the `XXX_STARGUARD`, Governance Facilitators are aware or already notified * [ ] IF `direct execution` is `yes` * [ ] Provided mandatory explanation of why direct execution is required makes sense on the technical level * [ ] The hash is checked via `require(XXX_SPELL.codehash == XXX_SPELL_HASH, "XXX_SPELL/wrong-codehash");` inside the Core spell - * [ ] The Prime spell is executed via `ProxyLike(XXX_PROXY).exec(XXX_SPELL, abi.encodeWithSignature("execute()"));` + * [ ] The Prime Agent spell is executed via `ProxyLike(XXX_PROXY).exec(XXX_SPELL, abi.encodeWithSignature("execute()"));` * [ ] `XXX` in `XXX_PROXY` matches the name of the Prime Agent * [ ] `XXX_PROXY` is fetched from chainlog - * [ ] Prime spell address (`XXX_SPELL`) matches Exec Sheet - * [ ] Prime spell hash (`XXX_SPELL_HASH`) matches Exec Sheet + * [ ] Prime Agent spell address (`XXX_SPELL`) matches Exec Sheet + * [ ] Prime Agent spell hash (`XXX_SPELL_HASH`) matches Exec Sheet * [ ] Execution is NOT delegate call - * [ ] IF Prime spell deployer is a smart contract (e.g. multisig or factory), ensure the deployer address is in `addresses_deployers.sol` as an entry + * [ ] IF Prime Agent spell deployer is a smart contract (e.g. multisig or factory), ensure the deployer address is in `addresses_deployers.sol` as an entry * IF Prime Agent provides instructions to be executed by the main spell (i.e. that will operate within Pause Proxy `DelegateCall` context) * [ ] No Prime Agent contract being interacted with is authed on a core contract like `vat`, etc. (Check comprehensively where the risk is high) * [ ] Prime Agent contract licensing and optimizations generally do not matter (except where they pose a security risk) diff --git a/spell/star-spell-reviewer-checklist.md b/spell/star-spell-reviewer-checklist.md index ea784fe2..c42b7d16 100644 --- a/spell/star-spell-reviewer-checklist.md +++ b/spell/star-spell-reviewer-checklist.md @@ -1,12 +1,12 @@ -# Star Spells Reviewer Checklist +# Prime Agent Spells Reviewer Checklist -This checklist provides guidance for crafting and reviewing spells for Star protocols (Spark, Bloom, etc.). It focuses on common practices and should be used alongside protocol-specific knowledge. +This checklist provides guidance for crafting and reviewing spells for the Prime Agents (Spark, Grove, Keel, etc.). It focuses on common practices and should be used alongside protocol-specific knowledge. ## Governance Architecture Considerations ### Spells Contracts Relationships -The diagram below illustrates that, despite the name, `StarSpell` is functionally more similar to `DssSpellAction` than to `DssSpell`. Both `StarSpell` and `DssSpellAction` contain executable logic, whereas `DssSpell` primarily handles scheduling and execution triggering through `MCD_PAUSE`. This architectural similarity is important to understand when reviewing and developing spells for Star protocols. +The diagram below illustrates that, despite the name, `PrimeAgentSpell` is functionally more similar to `DssSpellAction` than to `DssSpell`. Both `PrimeAgentSpell` and `DssSpellAction` contain executable logic, whereas `DssSpell` primarily handles scheduling and execution triggering through `MCD_PAUSE`. This architectural similarity is important to understand when reviewing and developing spells for the Prime Agent protocols.
View Diagram @@ -26,7 +26,7 @@ classDiagram DssSpell ..> DssSpellAction : instantiates %% Functional similarity - DssSpellAction <.. StarSpell : functionally similar + DssSpellAction <.. PrimeAgentSpell : functionally similar %% Reference relationship DssExec --> DssAction : references @@ -36,7 +36,7 @@ classDiagram ### Execution Flow -The diagram bellow illustrates the execution flow of a spell from the Core up to the Star protocol. +The diagram bellow illustrates the execution flow of a spell from the Core up to the Prime Agent protocol.
View Diagram @@ -62,9 +62,9 @@ graph TB MCD_PAUSE_PROXY[MCD_PAUSE_PROXY] end - subgraph StarCode ["Star Protocol"] + subgraph StarCode ["Prime Agent Protocol"] STAR_PROXY[STAR_PROXY] - StarSpell[StarSpell] + PrimeAgentSpell[PrimeAgentSpell] end %% Execution Flow with multi-level numbered steps @@ -75,7 +75,7 @@ graph TB MCD_PAUSE -->|2.2\. exec| MCD_PAUSE_PROXY MCD_PAUSE_PROXY -->|2.3\. delegatecall| DssSpellAction DssSpellAction -->|2.4\. call| STAR_PROXY - STAR_PROXY -->|2.5\. delegatecall| StarSpell + STAR_PROXY -->|2.5\. delegatecall| PrimeAgentSpell end %% Legend as a single node with HTML table - one item per row - no borders @@ -87,7 +87,7 @@ graph TB - Star Contracts + Prime Agent Contracts @@ -102,14 +102,14 @@ graph TB class DssSpell,DssSpellAction core class MCD_PAUSE,MCD_PAUSE_PROXY core class Governance governance - class STAR_PROXY,StarSpell star + class STAR_PROXY,PrimeAgentSpell star class MainDiagram transparent class CoreCode,StarCode boundary ```
-## Star Spells Review Process +## Prime Agent Spells Review Process This section outlines the review process and provides concrete action items for both the crafter and reviewers of the spell. The document is divided into separate stages ("Development", "Deployment", "Handover"). Both reviewers must complete all checks in the relevant stage and publish them as the PR comment at the end of each stage. @@ -120,7 +120,7 @@ This section outlines the review process and provides concrete action items for - `COMMIT_TITLE`, URL_TO_THE_PR_OR_THE_COMMIT - [ ] Content matches description: no unrelated changes. - [ ] No security-related changes are present in this commit. -- [ ] Verify solc version matches the Star protocol standard based on prior Star contracts. +- [ ] Verify solc version matches the Prime Agent protocol standard based on prior contracts. #### Spell Description & Comments - [ ] Spell PR has clear description. @@ -130,12 +130,12 @@ This section outlines the review process and provides concrete action items for - [ ] Every parameter change is clearly commented with before/after values. #### Proposed changes -- LIST every forum post proposing changes for this particular Star, particular target date: +- LIST every forum post proposing changes for this particular Prime Agent, particular target date: - FORUM_POST_TITLE, FORUM_POST_URL - [ ] Forum post follows the [known template](https://docs.google.com/document/d/1vLqeP-zXmxKo2OpoxnL2z0ZczPe4nWN49-3URx-iKVA/edit?tab=t.nkz4n7by2dnh). - [ ] Verify spell content matches the combined scope of the forum posts listed above. - [ ] Verify forum posts contain all new addresses directly or indirectly used in the spell, their constructor arguments and rate limits. -- [ ] IF the Star Spell introduces a major change that can affect external parties, suggest Governance Facilitators to set Core Spell office hours to `true`. +- [ ] IF the Prime Agent spell introduces a major change that can affect external parties, suggest Governance Facilitators to set Core Spell office hours to `true`. #### Contract Structure & Code Quality - [ ] The only external non-view function in the spell contract is `execute()`. @@ -149,12 +149,12 @@ This section outlines the review process and provides concrete action items for - [ ] Matches valid external source (previously approved forum post, external docs, etc). ### StarGuard execution -- [ ] IF a [StarGuard module](https://github.com/sky-ecosystem/star-guard) is onboarded for this Star, the following additional checks are done: +- [ ] IF a [StarGuard module](https://github.com/sky-ecosystem/star-guard) is onboarded for this Prime Agent, the following additional checks are done: - [ ] The spell exposes view-only interface `function isExecutable() external view returns (bool result)`. - [ ] `isExecutable` either simply returns `true` or implements additional logic communicated via the relevant forum post (e.g.: by describing "earliest launch date" or "office hours" logic, etc). - [ ] The test ensures the spell is executable before expiration (i.e. `isExecutable` outputs `true` before `StarGuard.maxDelay()` is passed). - [ ] Third-party actors can not take advantage of the fact that Spell will be executed in a later block than the Core spell, otherwise suggest `direct execution`. - - [ ] IF Prime spell can not be executed in a later block OR have to be executed sequentially to another Prime spell, `direct execution` is clearly mentioned in the forum post together with elaborated explanation why it is needed. + - [ ] IF Prime Agent spell can not be executed in a later block OR have to be executed sequentially to another Prime Agent spell, `direct execution` is clearly mentioned in the forum post together with elaborated explanation why it is needed. - [ ] The `direct execution` explanation makes sense on the technical level and can not be circumvented by the use of `isExecutable()` interface. #### On-boarding New Contracts @@ -211,7 +211,7 @@ This section outlines the review process and provides concrete action items for - [ ] No privileged functions accessible by unauthorized users. #### Parameter Changes & Protocol Integration -- [ ] Star Protocol invariants are maintained after spell execution. +- [ ] Prime Agent protocol invariants are maintained after spell execution. - [ ] All parameter changes use the appropriate helper functions IF available. - [ ] Parameter changes match the Executive Sheet exactly. - [ ] Spell interacts correctly with existing protocol components. @@ -276,8 +276,8 @@ EXECUTED_TESTS_LOGS - [ ] Both reviewers gave explicit "Good to handover". - [ ] All review comments have been addressed or resolved. - [ ] The spell address, the codehash and the direct execution are posted by the crafter in the `#govops` in the `XXX spell YYYY-MM-DD deployed to 0x… with hash 0x…, direct execution: yes / no` format. -- [ ] Posted spell address matches the spell evaluated above. -- [ ] Posted spell codehash matches the one noted down above. +- [ ] Posted spell address matches spell address approved for handover. +- [ ] Posted spell codehash matches codehash that you verified locally. - [ ] Posted direct execution value matches the forum post. - [ ] Confirm the address (via a separate "reply to" message, restating the address to avoid edits). - [ ] Ensure that no changes were made to the code since the spell was deployed and archived. From f106c3f3b1c9d67110475bfbfd29dff9d438e59e Mon Sep 17 00:00:00 2001 From: SidestreamColdMelon Date: Fri, 31 Oct 2025 12:47:14 +0100 Subject: [PATCH 8/8] forgotten nit --- spell/star-spell-reviewer-checklist.md | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/spell/star-spell-reviewer-checklist.md b/spell/star-spell-reviewer-checklist.md index c42b7d16..8e965d52 100644 --- a/spell/star-spell-reviewer-checklist.md +++ b/spell/star-spell-reviewer-checklist.md @@ -151,7 +151,7 @@ This section outlines the review process and provides concrete action items for ### StarGuard execution - [ ] IF a [StarGuard module](https://github.com/sky-ecosystem/star-guard) is onboarded for this Prime Agent, the following additional checks are done: - [ ] The spell exposes view-only interface `function isExecutable() external view returns (bool result)`. - - [ ] `isExecutable` either simply returns `true` or implements additional logic communicated via the relevant forum post (e.g.: by describing "earliest launch date" or "office hours" logic, etc). + - [ ] `isExecutable` either simply returns `true` or implements additional logic communicated via the relevant forum post (e.g., by describing "earliest launch date" or "office hours" logic, etc). - [ ] The test ensures the spell is executable before expiration (i.e. `isExecutable` outputs `true` before `StarGuard.maxDelay()` is passed). - [ ] Third-party actors can not take advantage of the fact that Spell will be executed in a later block than the Core spell, otherwise suggest `direct execution`. - [ ] IF Prime Agent spell can not be executed in a later block OR have to be executed sequentially to another Prime Agent spell, `direct execution` is clearly mentioned in the forum post together with elaborated explanation why it is needed.