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 1 commit
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
16 changes: 12 additions & 4 deletions spell/spell-crafter-mainnet-workflow.md
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -127,10 +127,18 @@ Repo: https://github.com/makerdao/spells-mainnet
* [ ] 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 hex encoded call:
* [ ] Add the output hex encoded call to the spell using low-level Solidity call.
* [ ] The call MUST use the pattern: `(bool succ, bytes memory err) = MULTICALL.call(<encodedDATA>);`
* [ ] Ensure proper error handling after the call (e.g., `require(succ, "Bug bounty update failed");`)
* [ ] 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 Down
9 changes: 5 additions & 4 deletions spell/spell-reviewer-mainnet-checklist.md
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -273,9 +273,10 @@
* [ ] Target Contract is included in the ChainLog
* [ ] Test Coverage is comprehensive
* IF bug bounty registry updates are present
* [ ] Run `make safeharbor-verify calldata=<encodedDATA>` command passing the calldata in the spell to check for it's validity.
* [ ] Verify the call uses the correct pattern: `(bool succ, bytes memory err) = MULTICALL.call(<encodedDATA>);`
* [ ] Confirm proper error handling is implemented for the call
* [ ] 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 @@ -387,7 +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 empty
* [ ] 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