Include sections for Safeharbor/BugBounties updates #47
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -66,6 +66,8 @@ Repo: https://github.com/makerdao/spells-mainnet | |
| * [ ] Remove unused interface declarations | ||
| * Ensure correctness of the cleanup | ||
| * [ ] Run Tests `make test` (or `make test match=<test_name>` to inspect debug traces) | ||
| * [ ] Run `make safeharbor-generate` to ensure that updates matches the bug bounty updates instructions on the Exec Sheet. | ||
| * [ ] If there is a mismatch, notify Governance Facilitators. | ||
| * Add comments to the spell based on the relevant [Exec Sheet](https://docs.google.com/spreadsheets/d/1w_z5WpqxzwreCcaveB2Ye1PP5B8QAHDglzyxKHG3CHw) | ||
| * [ ] Copy every _Section text_ from the Exec Sheet as comment to the spell code | ||
| * [ ] Surround the comment by the set of dashes (e.g. `// ----- Section text -----`) | ||
|
|
@@ -125,6 +127,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 | ||
| * [ ] 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
There was a problem hiding this comment. 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. There was a problem hiding this comment. 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 | ||
| * [ ] Fetch the agreement address from the `ChainLog` | ||
| * [ ] 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 | ||
|
|
@@ -139,6 +150,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 | ||
|
There was a problem hiding this comment. Q: Could you please elaborate on what kind of tests should be written by the crafter? Is There was a problem hiding this comment. Yes, I think making sure that there are no reverts. There was a problem hiding this comment. 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 There was a problem hiding this comment. 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 |
||
| * [ ] 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 | ||
|
|
@@ -217,6 +230,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 "no updates" in the testnet environment after spell was cast | ||
| * [ ] 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 | ||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -19,6 +19,7 @@ | |
| * [ ] Office hours is `true` IF spell introduces a major change that can affect external parties (e.g.: keepers are affected in case of collateral offboarding) OTHERWISE explicitly set to `false` | ||
| * [ ] Office hours value matches the Exec Sheet | ||
| * [ ] 30 days spell expiry set in the constructor (`block.timestamp + 30 days`) | ||
| * [ ] `make safeharbor-generate` outputs matches the instructions on the Exec Sheet. If there is a mismatch, notify Governance Facilitators. | ||
| * Spell description | ||
| * [ ] Description follows the format `TARGET_DATE MakerDAO Executive Spell | Hash: EXEC_DOC_HASH` | ||
| * [ ] `TARGET_DATE` in the description matches the target date | ||
|
|
@@ -272,6 +273,13 @@ | |
| * [ ] Target contract is not upgradable | ||
| * [ ] Target Contract is included in the ChainLog | ||
| * [ ] Test Coverage is comprehensive | ||
| * IF bug bounty registry updates are present | ||
SidestreamBurningBanana marked this conversation as resolved.
Show resolved
Hide resolved
|
||
| * [ ] 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) | ||
|
|
@@ -383,6 +391,8 @@ _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" | ||
|
There was a problem hiding this comment. Please also add a check for the warnings, according to this comment #47 (comment) |
||
| * [ ] If the script outputs a warning indicated by ⚠️ ❗, notify Governance Facilitators. | ||
| * 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 | ||
|
|
||
Uh oh!
There was an error while loading. Please reload this page.