Skip to content

chore: improve star reviewer checklist #60

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Open
SidestreamStrongStrawberry wants to merge 3 commits into
base: master
Choose a base branch
from
Open

chore: improve star reviewer checklist #60

Changes from all commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
0b2af5d
chore: update checklist
SidestreamStrongStrawberry Jan 13, 2026
c7b8951
chore: updating checklist
SidestreamStrongStrawberry Jan 13, 2026
0e5592e
chore: update structure
SidestreamStrongStrawberry Jan 20, 2026
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
9 changes: 7 additions & 2 deletions spell/star-spell-reviewer-checklist.md
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -121,10 +121,13 @@ This section outlines the review process and provides concrete action items for
- [ ] Content matches description: no unrelated changes.
- [ ] No security-related changes are present in this commit.
- [ ] Verify solc version matches the Prime Agent protocol standard based on prior contracts.
- [ ] Specify the correct spell target data: `YYYY-MM-DD`

#### Spell Description & Comments
- [ ] Spell PR has clear description.
- [ ] Spell PR has a clear description.
- [ ] Spell PR has a correct spell target date.
- [ ] Spell contract has a clear description.
- [ ] Spell contract has a correct spell target date.
- [ ] Every significant action and parameter change are clearly commented in the code.
- [ ] Every significant action has valid source url (forum post, poll, atlas).
- [ ] Every parameter change is clearly commented with before/after values.
Expand Down Expand Up @@ -165,13 +168,15 @@ This section outlines the review process and provides concrete action items for
- [ ] IF source code is not audited, there is a clear explanation that was agreed upon by governance beforehand (i.e.: reusing unaudited contracts with lots of Lindy effect).
- [ ] Compilation optimizations match deployment settings defined in the source code repo.
- [ ] Consistent license.
- [ ] Deployer address was not used on other chains that star is onboarded UNLESS there is a valid reason for it (e.g., external contract, the same deployer was used to keep addresses the same across chains, etc).
Copy link
Copy Markdown
Contributor

@0xBasset 0xBasset Jan 28, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What's the context on this? Why is it a problem?

Reading from a fresh perspective, seems like an item that won't add anything beside more work to reviewers.

Copy link
Copy Markdown
Contributor Author

@SidestreamStrongStrawberry SidestreamStrongStrawberry Feb 2, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This was added to avoid the contract addresses collision on different chains. As it can be confusing when the different contracts on the different chains use the same address.

Copy link
Copy Markdown
Contributor

@0xBasset 0xBasset Feb 10, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

On a lot of cases, address collision on different chains is a feature, not a bug. And even if non-intentional, I'm not sure if it's a problem at all.

- LIST every constructor argument:
- `CONSTRUCTOR_ARGUMENT_NAME` being `CONSTRUCTOR_ARGUMENT_VALUE` from EXTERNAL_SOURCE_URL
- [ ] 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`).
- [ ] No other addresses has access to this contract.
- [ ] IF the contract is a vault, each vault role that can have access to the funds has to be validated against trusted external sources (i.e. docs listing contracts which have that role in the vault) or against other verifiable sources.

This comment was marked as resolved.

Sign in to view

Copy link
Copy Markdown
Contributor Author

@SidestreamStrongStrawberry SidestreamStrongStrawberry Jan 20, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Vault is vague from a technical viewpoint.

I think this is rather more on functional side so what the smart contract do.
Main point here is verifying the access to big amount of tokens.

As a reviewer what is a trusted external source or other verifiable sources?

IMO, it can be previously onboarded contract, or any form of off-chain information from the vault owner.

I agree this sounds vague. I wanted to make it bit more general instead the action locked to 1 possible solution which hopefully allows reviewers to make the correct judge on what makes sense at the point of review.

As we are using the checklist, if we fix a custom for checking this value we can always update.
But also if you prefer a specific check, feel free to suggest!

This seems like it might make more sense as part of the wards sub-checklist?

Wards check already have IF branch, which means the sub checks are not done when the contract doesn't have wards (or auth) concept.

So I'd prefer keeping them separately.

Copy link
Copy Markdown
Contributor

@0xBasset 0xBasset Jan 28, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Does the whole section refers mostly to "internal" (start deployed) or "external" (other protocols) contracts?

IF it's external, then I don't think this item is reasonable, as it would require the reviewer in depth understanding of another protocol access control rules and internal key management policies.

IF it's internal, then this might be fine, although I don't think it brings that much extra security.

ps. I do think this checklist would benefit from a clear separation between internal onboarded and external onboarded contract. The section was inspired by core checklist, which rarely interact with external protocols, but the scenario is the exact opposite for starts.

Copy link
Copy Markdown
Contributor Author

@SidestreamStrongStrawberry SidestreamStrongStrawberry Feb 2, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The Vault contract can be external or internal.

I think this can make the trust level stronger, as it does not only require to provide the address who has access to funds but also requires the source of the address.
For example, if an external vault contract is onboarded, the address who can access to contract owned tokens should be verified by a reliable source and this can add extra layer of transparency.

Probably star team already has done this in their process, so it would be mainly the reviewer who needs to do extra validation.

This approach still has limitations but I hope this can make the system more trustworthy!

Let me know if you have any other suggestions on this! :)

Copy link
Copy Markdown
Contributor

@0xBasset 0xBasset Feb 10, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I personally don't think this is reasonable for a start checklist, specially given how short the time for reviewing this stuff is. Vaults comes in many different flavors and each protocol has its own process to on how to manage access control and permissions.

Besides, from a first principle, this should be a risk/due-dilligence review, not a technical one. I don't think it's wise to take this into our own hands as our job is to just make sure that whatever the stars decides to execute is technically viable and won't break downstream and that's it. If we start having opinions on how good an investment is based on loose concepts like addresses that can access the funds, we become liable for future investments.

That's my opinion tho: including an item like this is we would be shooting ourselves in the foot on a massive way, for a very marginal "trust" benefit.


#### Dependency checks
- LIST every submodule or any other imported code used in this spell:
Expand Down Expand Up @@ -284,5 +289,5 @@ EXECUTED_TESTS_LOGS
- [ ] 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.
- [ ] Confirm that no changes have been made to the code since the "Good to deploy" comment was posted, EXCEPT for changes related to deployment. (i.e. adding the deployed spell payload, updating test code to use the deployed contract for testing).
- [ ] IF no blockers were found, post the completed "Handover Stage" checklist stage with the explicit pull request approval via 'Approve' review option.