Skip to content

Include sections for Safeharbor/BugBounties updates #47

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
wants to merge 12 commits into
base: master
Choose a base branch
from
Open

Include sections for Safeharbor/BugBounties updates #47

Show file tree
Hide file tree
Changes from 11 commits
Commits
Show all changes
12 commits
Select commit Hold shift + click to select a range
2fef81d
add sections for safeharbor adoption
0xBasset May 30, 2025
1f5dce0
update sections to reflect latest script
0xBasset Sep 17, 2025
f0c79a5
adjust to use make commands
0xBasset Sep 19, 2025
4060365
add calldata to dictionary
0xBasset Sep 19, 2025
ffbea9f
fix script name error and other comments adjustments
0xBasset Sep 22, 2025
0766476
remame script elsewhere too
0xBasset Sep 22, 2025
18f5fab
remove required comments
0xBasset Sep 23, 2025
10ee338
remove item on comment verification
0xBasset Sep 24, 2025
11f6ac4
adjust list to adhere to new code format
0xBasset Oct 13, 2025
93cbffa
remove function from checklist
0xBasset Oct 16, 2025
5a1528b
addressed PR comments
0xBasset Oct 17, 2025
fd2383a
address another round of PR reviews
0xBasset Oct 28, 2025
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
8 changes: 8 additions & 0 deletions .wordlist.txt
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -87,6 +87,9 @@ auth
authed
authing
bytecode
Calc
CalcFab
calldata
casted
checksummed
collateralization
Expand Down Expand Up @@ -129,6 +132,11 @@ precomputed
prepended
redemptions
repos
RPC
RWA
RWAXXX
RWAXYZ
safeharbor
setIlkAutoLineDebtCeiling
setIlkAutoLineParameters
setIlkDebtCeiling
Expand Down
12 changes: 12 additions & 0 deletions spell/spell-crafter-mainnet-workflow.md
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -125,6 +125,15 @@ 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
* Bug Bounty Registry Updates
* [ ] Check that output of make safeharbor-generate matches the instructions provided by Governance Facilitators
Copy link
Contributor

Choose a reason for hiding this comment

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

nit:

Suggested change
* [ ] Check that output of make safeharbor-generate matches the instructions provided by Governance Facilitators
* [ ] Check that output of `make safeharbor-generate` matches the instructions provided by Governance Facilitators

* [ ] If no instructions were provided and script produces "no changes", then no further action is required
* [ ] If there is a mismatch, crafter should notify Governance Facilitators
* [ ] If the scripts outputs a warning indicated by ⚠️ ❗, notify Governance Facilitators.
Comment on lines +133 to +134
Copy link
Contributor

Choose a reason for hiding this comment

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

There's one point about the process which I don't think has been resolved yet - timing. Currently both crafter and reviewer Safe Harbor sections are quite far into the checklists, after the tests, which leaves very little time for reacting to missing instructions (sections would probably be executed after Exec Doc was already merged).

I would therefore strongly recommend coming up with an earlier crafter and/or reviewer check to ensure that SafeHarbor instructions are as expected. As suggested in #47 (comment), this should preferably happen at one of the earliest stages as it's a simple script run + check instructions + notify govfac if needed.

Of course, GovFac would also need to amend their internal checklists, but this is out of scope of this review.

Copy link
Author

Choose a reason for hiding this comment

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

I've added a new item on the beginning of both the crafter and reviewer checklist. Please let me know your thoughts.

* [ ] If the command outputs a solidity snippet that matches the instructions provided by Governance Facilitators:
* [ ] Paste the generated code into the spell as is. The code should not be modified. You may adjust formatting
* [ ] Import the `AGREEMENT_ADDRESS` from the `ChainLog`
Copy link
Contributor

Choose a reason for hiding this comment

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

This hasn't been addressed yet: #47 (comment)

* [ ] If not already present, add the helper function to perform the call, using the established archive pattern.
* Add specific tests in `DssSpell.t.sol` to have sufficient test coverage for every spell action
* [ ] Test new collaterals
* [ ] Test new ilk registry values
Expand All @@ -139,6 +148,8 @@ Repo: https://github.com/makerdao/spells-mainnet
* [ ] Sanity checks of the constructor arguments
* [ ] Sanity checks of all values added/updated by the spell function
* [ ] End-to-end "happy path" interaction with the module
* IF bug bounty updates are present
* [ ] Test that all bug bounty registry calls execute successfully
Copy link
Contributor

Choose a reason for hiding this comment

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

Q: Could you please elaborate on what kind of tests should be written by the crafter? Is vm.expectCall sufficient or would the crafter have to test every new addition or deletion?

Copy link
Author

Choose a reason for hiding this comment

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

Yes, I think making sure that there are no reverts.

Copy link
Contributor

Choose a reason for hiding this comment

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

nit: I would still recommend adding an example of how to test it. You can omit it, however, if we are going to add a permanent test for it

Copy link
Author

Choose a reason for hiding this comment

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

Hmm, I don't think we should include testing examples on this checklist. I think a permanent set of helper functions on the spells-mainnet repo would be enough to make sure a crafter can easily test.

For example a assertAccountIsPresent(address), or general tests that check the number of items on each chain, like there are with chainlog stuff. Either way, I don't think this is the place to add those.

* [ ] Tests PASS via `make test`
* [ ] Ensure `DssExecLib` address used in current spell (`DssExecLib.address`) matches `dss-exec-lib` [Latest Release Tag](https://github.com/makerdao/dss-exec-lib/releases/latest)
* [ ] Push committed content to already opened PR
Expand Down Expand Up @@ -217,6 +228,7 @@ Repo: https://github.com/makerdao/spells-mainnet
* [ ] Create testnet and cast deployed spell there using `make cast-on-tenderly spell=0x...` command
* [ ] Check that returned `public explorer url` is publicly accessible (e.g. using incognito browser mode)
* [ ] IF `cast-on-tenderly` command is executed several times for the same spell, delete all testnets of the same name except the last one
* [ ] `make safeharbor-generate` returns empty diff in the testnet environment after spell was cast
Copy link
Contributor

Choose a reason for hiding this comment

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

nit: "empty diff" can be replaced with "no updates"

* [ ] Archive Spell via `make archive-spell` for the current date (or `make archive-spell date="YYYY-MM-DD"`) using Target Date inside the Exec Doc
* [ ] Commit & push changes for review
* [ ] Wait for CI to PASS
Expand Down
8 changes: 8 additions & 0 deletions spell/spell-reviewer-mainnet-checklist.md
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -272,6 +272,13 @@
* [ ] Target contract is not upgradable
* [ ] Target Contract is included in the ChainLog
* [ ] Test Coverage is comprehensive
* IF bug bounty registry updates are present
* [ ] Run `make safeharbor-generate`
* [ ] Verify that the generated code exactly matches the code in the spell
* [ ] Verify that output matches the instructions provided by Governance Facilitators
* [ ] Ensure that the script does not output any warnings, which are indicated by ⚠️ ❗
* [ ] Ensure that agreement address is fetched from the Chainlog
* [ ] Ensure that the helper function to perform the call is present and is implemented using the established archive pattern
* IF spell interacts with ChainLog
* [ ] ChainLog version is incremented based on update type
* Major -> New Vat (++.0.0)
Expand Down Expand Up @@ -383,6 +390,7 @@ _Insert your local test logs here_
* [ ] All actions are executed in the transaction trace
* [ ] No reverts are present that block execution
* [ ] No out-of-gas errors are present
* [ ] `make safeharbor-generate` against the testnet returns "no updates"
Copy link
Contributor

Choose a reason for hiding this comment

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

Please also add a check for the warnings, according to this comment #47 (comment)

* Archive checks
* [ ] `make diff-archive-spell` for current date or `make diff-archive-spell date="YYYY-MM-DD"`
* [ ] Ensure date corresponds to target Exec Doc date
Expand Down