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 9 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
17 changes: 17 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,20 @@ 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
* [ ] Run `make safeharbor-generate` command in the `spells-mainnet` repo to check for bug bounty updates
* [ ] IF the command outputs a solidity snippet:
Copy link
Contributor

Choose a reason for hiding this comment

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

This places the Safeharbor update responsibility on the crafter. IMHO this has to come from GovFac, meaning that crafter should:

  • check that output of make safeharbor-generate matches the instructions provided by GovFac (who should've used the script in Week 1);
    • if no instructions were provided and script produces "no changes" - all is good;
    • if there is a mismatch, crafter should notify GovFac;
  • when there are instructions to execute, crafter should paste the code and follow the rest of the process as you already outlined

* [ ] 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.

nit:

  1. The address is fetched and not imported;
  2. Let's not specify the exact chainlog key in the checklist, otherwise this item will get outdated after the adoption when a different key is used.
Suggested change
* [ ] Import the `AGREEMENT_ADDRESS` from the `ChainLog`
* [ ] Fetch the agreement address from the `ChainLog`

* [ ] If not already present, add the helper function to perform the call:
```solidity
function _updateSafeHarbor(bytes[] memory calldatas) public {
for (uint256 i = 0; i < calldatas.length; i++) {
(bool success, ) = address(AGREEMENT_ADDRESS).call(calldatas[i]);
require(success, "SaferHarbor call failed");
}
}
```
Copy link
Contributor

Choose a reason for hiding this comment

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

Hmm, not sure we want to have the function in the checklist. Maybe we just reference the archive:

Suggested change
* [ ] If not already present, add the helper function to perform the call:
```solidity
function _updateSafeHarbor(bytes[] memory calldatas) public {
for (uint256 i = 0; i < calldatas.length; i++) {
(bool success, ) = address(AGREEMENT_ADDRESS).call(calldatas[i]);
require(success, "SaferHarbor call failed");
}
}
```
* [ ] 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 +153,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 +233,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
* [ ] Verify `make safeharbor-generate` returns empty diff in the casted environment after spell execution.
* [ ] 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
6 changes: 6 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,11 @@
* [ ] 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` command passing the calldata in the spell to check for it's validity.
Copy link
Contributor

Choose a reason for hiding this comment

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

Reviewers should also make sure that no warnings are produced by the script - neither before nor after deployment

* [ ] Verify that the generated code exactly matches the generated code in the spell.
* [ ] Ensure that `AGREEMENT_ADDRESS` is imported from the ChainLog.
* [ ] Ensure that the helper function to perform the call is present and is implemented correctly.
* IF spell interacts with ChainLog
* [ ] ChainLog version is incremented based on update type
* Major -> New Vat (++.0.0)
Expand Down Expand Up @@ -383,6 +388,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
* [ ] Confirm `make safeharbor-generate` returns "no updates"
* 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