Improve star checklist #55

SidestreamColdMelon · 2025-10-08T15:53:07Z

Improves star checklists based on the previous reviews
Replaces FOREACH keyword with the new LIST pattern to promote explicit checks for each items, improve external reviewability of the completed checklists and enforce first reviewers to actually fill in required details instead of just ticking all boxes

riccardopersiani

Good that the early mention of the executive doc was removed in favor of checking the forum post, since at that stage the doc is almost never ready.

riccardopersiani · 2025-10-09T13:03:44Z

spell/star-spell-reviewer-checklist.md

+    - [ ] IF the contract have a concept of access control or `wards`:
+      - [ ] Expected admin address for this chain has full access (`SubProxy` on mainnet, `Executor` on other chains).
+      - [ ] Contract deployer address has no access (e.g. `wards(deployer)` is `0`).
+      - [ ] No other addresses has access to this contract.


Maybe it's good to specify: "other than the Star Proxy".

Stating this would create more confusion and contradict the check above. As specified above, SubProxy is just one of the admin options, on L2s SubProxy isn't present, instead Executor contract shall be set as admin. So I would keep this check as is

riccardopersiani · 2025-10-09T13:04:35Z

spell/star-spell-reviewer-checklist.md

+        - [ ] The value has valid external source.
+    - [ ] IF the contract have a concept of access control or `wards`:
+      - [ ] Expected admin address for this chain has full access (`SubProxy` on mainnet, `Executor` on other chains).
+      - [ ] Contract deployer address has no access (e.g. `wards(deployer)` is `0`).


I did not mind the mention of specific functions rely and deny in the previous version.

Stars doesn't really follow rely/deny pattern. In fact, I know zero Prime-owned/developed contracts that follow this pattern. Instead, current ALM system uses AccessControl from OpenZeppelin (see for example MainnetController). So I left wards(deployer) as an e.g. but not as a primary explanation of what "access" means

riccardopersiani · 2025-10-09T13:05:41Z

spell/star-spell-reviewer-checklist.md

+- LIST every submodule or any other imported code used in this spell:
+  - `DEPENDENCY_NAME` imported at commit `COMMIT_HASH` COMMIT_URL
+    - [ ] The dependency commit matches audited commit.
+    - [ ] The dependency commit matches the version of the deployed contracts. (if ALM contracts are updated, then dependency also needs to be updated and vice-versa: dependency shouldn't be updated unless the ALM contracts are updated)


Suggested change

- [ ] The dependency commit matches the version of the deployed contracts. (if ALM contracts are updated, then dependency also needs to be updated and vice-versa: dependency shouldn't be updated unless the ALM contracts are updated)

- [ ] The dependency commit matches the version of the deployed contracts (if ALM contracts are updated, then dependency also needs to be updated and vice-versa: dependency shouldn't be updated unless the ALM contracts are updated).

riccardopersiani · 2025-10-09T13:06:12Z

spell/star-spell-reviewer-checklist.md

+- [ ] Addresses must be fetched from the relevant protocol's address registry (e.g., `spark-address-registry`, `bloom-address-registry`) IF they are present there, OTHERWISE defined as `constant` and have trusted source (e.g., when onboarding new contracts).
+- LIST all addresses used in the spell (defined as `constant` or fetched from the registry repo)
+  - [CHAIN_NAME] `0xADDRESS`, EXTERNAL_SOURCE_URL
+    - [ ] Matches valid external source (previously approved forum post, external docs, etc)


Suggested change

- [ ] Matches valid external source (previously approved forum post, external docs, etc)

- [ ] Matches valid external source (previously approved forum post, external docs, etc).

riccardopersiani · 2025-10-09T13:06:35Z

spell/star-spell-reviewer-checklist.md

- [ ] Addresses must be fetched from the relevant protocol's address registry (e.g., `spark-address-registry`, `bloom-address-registry`) IF they are present, OTHERWISE defined as `constant` when sourced from a trusted source (i.e., new contracts onboarding).
-
+- [ ] Addresses must be fetched from the relevant protocol's address registry (e.g., `spark-address-registry`, `bloom-address-registry`) IF they are present there, OTHERWISE defined as `constant` and have trusted source (e.g., when onboarding new contracts).
+- LIST all addresses used in the spell (defined as `constant` or fetched from the registry repo)


Suggested change

- LIST all addresses used in the spell (defined as `constant` or fetched from the registry repo)

- LIST all addresses used in the spell (defined as `constant` or fetched from the registry repo):

riccardopersiani · 2025-10-09T13:08:08Z

spell/star-spell-reviewer-checklist.md

- [ ] Rates are calculated correctly and match their source (e.g., governance poll).
+- [ ] Every contract variable is declared as either `constant` or `immutable`.
+- [ ] Every precision variable (`WAD`, `RAY`, `RAD`, etc) match their expected value.
+- LIST every variable using precision (`e18`, `e6`, `e...`, `WAD`, `RAY`, `RAD`, etc)


Suggested change

- LIST every variable using precision (`e18`, `e6`, `e...`, `WAD`, `RAY`, `RAD`, etc)

- LIST every variable using precision (`e18`, `e6`, `e...`, `WAD`, `RAY`, `RAD`, etc):

riccardopersiani · 2025-10-09T13:08:29Z

spell/star-spell-reviewer-checklist.md

 #### Testing
+- LIST all tests and explain their coverage
+  - `TEST_FUNCTION_NAME` ensures EXPLANATION_OF_WHAT_IT_ENSURES
+    - [ ] The test is sufficient to ensure correctness the high-level goal behind tested spell action


Suggested change

- [ ] The test is sufficient to ensure correctness the high-level goal behind tested spell action

- [ ] The test is sufficient to ensure correctness the high-level goal behind tested spell action.

riccardopersiani · 2025-10-09T13:08:37Z

spell/star-spell-reviewer-checklist.md

 - [ ] Proper error handling for all external interactions.

 #### Testing
+- LIST all tests and explain their coverage


Suggested change

- LIST all tests and explain their coverage

- LIST all tests and explain their coverage:

I think it'd be better if we list the spell instructions and then point to tests that cover each (like we do on the core spells).

@oddaf: reformulated into the list with spell actions:

- LIST each spell action (each line of code which changes a storage): - INTENDED_SPELL_ACTION tested via TEST_NAME_1, TEST_NAME_2 - [ ] The unit test ensures the new value was changed in the spell. - [ ] The end-to-end test is sufficient to ensure correctness the high-level goal behind this spell action.

The idea is to also have all test names listed in the completed checklist, not only the actions

riccardopersiani · 2025-10-09T13:09:01Z

spell/star-spell-reviewer-checklist.md

+
+#### 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)


Suggested change

- [ ] A new comment in the PR contains link to the deployed spell(s) and Tenderly vnet(s)

- [ ] A new comment in the PR contains link to the deployed spell(s) and Tenderly vnet(s).

riccardopersiani · 2025-10-09T13:09:08Z

spell/star-spell-reviewer-checklist.md

+### Deployment Stage
+
+#### Deployed Contract
+- [ ] Both reviewers gave explicit "Good to deploy"


Suggested change

- [ ] Both reviewers gave explicit "Good to deploy"

- [ ] Both reviewers gave explicit "Good to deploy".

SidestreamColdMelon added 7 commits October 6, 2025 18:34

improve star checklist based on the previous reviews

6920aa8

improve LIST command to it being simply ticked

6959a65

fix spelling mistakes, wordlist

2d8f11d

apply suggestions

bc53d21

fix typecheck

e27adbb

fix typos, remove duplicates

68cf58f

fix grammar

f56842d

SidestreamColdMelon requested a review from oddaf October 8, 2025 15:53

riccardopersiani reviewed Oct 9, 2025

View reviewed changes

address review

a3b76e6

riccardopersiani self-requested a review October 16, 2025 15:17

riccardopersiani approved these changes Oct 16, 2025

View reviewed changes

oddaf approved these changes Oct 17, 2025

View reviewed changes

SidestreamColdMelon merged commit 1b4e8f4 into master Oct 17, 2025
2 checks passed

SidestreamColdMelon deleted the improve-star-checklist branch October 17, 2025 16:43

	- [ ] The dependency commit matches the version of the deployed contracts. (if ALM contracts are updated, then dependency also needs to be updated and vice-versa: dependency shouldn't be updated unless the ALM contracts are updated)
	- [ ] The dependency commit matches the version of the deployed contracts (if ALM contracts are updated, then dependency also needs to be updated and vice-versa: dependency shouldn't be updated unless the ALM contracts are updated).

	- [ ] Matches valid external source (previously approved forum post, external docs, etc)
	- [ ] Matches valid external source (previously approved forum post, external docs, etc).

	- LIST all addresses used in the spell (defined as `constant` or fetched from the registry repo)
	- LIST all addresses used in the spell (defined as `constant` or fetched from the registry repo):

	- LIST every variable using precision (`e18`, `e6`, `e...`, `WAD`, `RAY`, `RAD`, etc)
	- LIST every variable using precision (`e18`, `e6`, `e...`, `WAD`, `RAY`, `RAD`, etc):

	- [ ] The test is sufficient to ensure correctness the high-level goal behind tested spell action
	- [ ] The test is sufficient to ensure correctness the high-level goal behind tested spell action.

	- LIST all tests and explain their coverage
	- LIST all tests and explain their coverage:

	- [ ] A new comment in the PR contains link to the deployed spell(s) and Tenderly vnet(s)
	- [ ] A new comment in the PR contains link to the deployed spell(s) and Tenderly vnet(s).

	- [ ] Both reviewers gave explicit "Good to deploy"
	- [ ] Both reviewers gave explicit "Good to deploy".

Improve star checklist #55

Improve star checklist #55

Uh oh!

Conversation

SidestreamColdMelon commented Oct 8, 2025

Uh oh!

riccardopersiani left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants