- 
                Notifications
    You must be signed in to change notification settings 
- Fork 9
Refresh the checklists to match latest practices #54
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.
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
Conversation
| * [ ] Test DAI/MKR/USDS/SKY/SPK streams and payments, lerps | ||
| * [ ] Test the sum of all DAI/MKR/USDS/SKY/SPK payments matches the Exec Sheet | 
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I believe it's safe to say that we can remove DAI and MKR mentions here.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I believe that both DAI and MKR, while not recently used in payments or streams, are still owned by the protocol. In case of DAI, it is still used under the hood of several things (like transferUsds).
Keeping them in seems like a zero-effort and no-compromise solution to me as they can be ignored unless required.
I would also be fine with removing only MKR as it is now officially retired and there is practically zero chance of getting new MKR payments or streams.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There will be no DAI or MKR payments/vesting streams in the future. Since we're making the effort to update the checklist, I'd say we should simply remove mentions to it.
Also notice that sendPaymentFromSurplusBuffer will support USDS in the next release of DssExecLib.
| * Interfaces imported from `dss-interfaces` | ||
| * [ ] No unused `dss-interfaces` | ||
| * [ ] Only single import layout is used (e.g. `import "dss-interfaces/dss/VatAbstract.sol";`) | ||
| * [ ] Only single import layout is used (e.g. `import { VatAbstract } from "dss-interfaces/dss/VatAbstract.sol";`) | 
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There are 744 matches in the archive with named imports WITHOUT spaces, while there are 254 examples of imports WITH spaces.
forge fmt by default formats imports without spaces, even though we still don't use it in this repo.
| * [ ] Only single import layout is used (e.g. `import { VatAbstract } from "dss-interfaces/dss/VatAbstract.sol";`) | |
| * [ ] Only single import layout is used (e.g. `import {VatAbstract} from "dss-interfaces/dss/VatAbstract.sol";`) | 
| * [ ] IF vest amount is expressed in 'per year' or similar in the Exec Sheet, account for leap days | ||
| * [ ] `bgn` (Vest start timestamp) matches Exec Sheet | ||
| * [ ] `tau` is expressed as `bgn - fin` (i.e. `MONTH_DD_YYYY - MONTH_DD_YYYY`) | ||
| * [ ] `tau` is expressed as `fin - bgn` (i.e. `MONTH_DD_YYYY - MONTH_DD_YYYY`) | 
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
That's not always the case.
This item was included back when Core Units were paid by streams with predefined bgn and fin, but more recently the most common use cases for vesting streams are the farms.
Usually tau is expressed as an interval in such cases.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
You are right, I can mention both formats as acceptable since both seem to be used:
- fin - bgn: https://github.com/sky-ecosystem/spells-mainnet/blob/fd9dbf23a5b6724b7367b6d4024b8d983743a796/archive/2025-01-23-DssSpell/DssSpell.sol#L240
- exact time: https://github.com/sky-ecosystem/spells-mainnet/blob/fd9dbf23a5b6724b7367b6d4024b8d983743a796/archive/2025-05-29-DssSpell/DssSpell.sol#L222
| * [ ] Tested via: | ||
| * `testYankDAI` | ||
| * `testYankMKR` | ||
| * `testYankSky` | ||
| * `testYankSkyMint` | ||
| * `testYankUsds` | ||
| * `testYankSpk` | 
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This was recently refactored. Now both creations and yanks are handled by the testVest{TOKEN} tests.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I believe we should remove anything related to DAI/MKR payments/vests from the checklists.
| * [ ] Check if oracle deployment is required (e.g. univ3-lp-oracle, new ilk pip, ...) with responsible ecosystem actor | ||
| * IF addresses are used in the spell | ||
| * [ ] Use `immutable` visibility when declaring addresses using `DssExecLib.getChangelogAddress`, OTHERWISE use `constant` for statically defined addresses | ||
| * [ ] Fetch addresses as type `address` and wrap with `Like` suffix interfaces inline (when making calls), EXCEPT `MKR` and vesting contracts | 
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think we should remove this, or at least remove the exception?
| ## Development Stage | ||
|  | ||
| * Install stable Foundry version | ||
| * [ ] Find the first [Foundry release](https://github.com/foundry-rs/foundry/releases) that is older than 7 days from now | 
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
| * [ ] Find the first [Foundry release](https://github.com/foundry-rs/foundry/releases) that is older than 7 days from now | 
With Amusing, we were considering removing this line and the next one.
| 
 I have removed MKR mentions from the checklists. Unfortunately, there are still several DAI vests with  If it's fine with you, I'd prefer to keep DAI in the checklists for now. | 
| * IF `DAI` surplus buffer transfers are present | ||
| * [ ] Recipient address in the instruction is in the checksummed format | ||
| * [ ] Recipient address matches Exec Sheet | ||
| * [ ] Recipient address variable name matches one found in `addresses_wallets.sol` | ||
| * [ ] Transfer amount matches Exec Sheet | ||
| * [ ] The transfers are tested via `testDAIPayments` test | ||
| * [ ] Sum of all DAI transfers tested in `testDAIPayments` matches number in the Exec Sheet | ||
| * IF `MKR` or `DAI` streams (`DssVest`) are created | ||
| * [ ] The transfers are tested via `testPayments` test | ||
| * [ ] Sum of all DAI transfers tested in `testPayments` matches number in the Exec Sheet | 
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This one for sure will no longer be used. Could be removed.
| * IF SKY stream ([DssVestTransferrable](https://github.com/sky-ecosystem/dss-vest/blob/master/src/DssVest.sol#L463)) is present | ||
| * [ ] Vest contract's SKY allowance increased by the cumulative `total` (the sum of all `tot` values) | 
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
| * IF SKY stream ([DssVestTransferrable](https://github.com/sky-ecosystem/dss-vest/blob/master/src/DssVest.sol#L463)) is present | |
| * [ ] Vest contract's SKY allowance increased by the cumulative `total` (the sum of all `tot` values) | |
| * IF new SKY streams ([DssVestTransferrable](https://github.com/sky-ecosystem/dss-vest/blob/master/src/DssVest.sol#L463)) are present | |
| * [ ] Vest contract's SKY allowance increased by the cumulative total (the sum of all `tot` values) | 
| * IF SPK stream ([DssVestTransferrable](https://github.com/sky-ecosystem/dss-vest/blob/master/src/DssVest.sol#L463)) is present | ||
| * [ ] Vest contract's SPK allowance increased by the cumulative `total` (the sum of all `tot` values) | 
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
| * IF SPK stream ([DssVestTransferrable](https://github.com/sky-ecosystem/dss-vest/blob/master/src/DssVest.sol#L463)) is present | |
| * [ ] Vest contract's SPK allowance increased by the cumulative `total` (the sum of all `tot` values) | |
| * IF new SPK streams ([DssVestTransferrable](https://github.com/sky-ecosystem/dss-vest/blob/master/src/DssVest.sol#L463)) are present | |
| * [ ] Vest contract's SPK allowance increased by the cumulative total (the sum of all `tot` values) | 
| * [ ] Where addresses are fetched from the `ChainLog`, the variable name must match the value of the ChainLog key for that address (e.g. `MCD_VAT` rather than `vat`), except where the archive pattern differs from this pattern (e.g. MKR) | ||
| * [ ] Fetch addresses as type `address` and wrap with `Like` suffix interfaces inline (when making calls), UNLESS archive patterns permit otherwise (such as `SKY`) | ||
| * [ ] Use the [DssExecLib Core Address Helpers](https://github.com/sky-ecosystem/dss-exec-lib/blob/master/src/DssExecLib.sol#L166) where possible (e.g. `DssExecLib.vat()`) | ||
| * [ ] Where addresses are fetched from the ChainLog, the variable name must match the value of the ChainLog key for that address (e.g. `MCD_VAT` rather than `vat`), EXCEPT where the archive pattern differs from this pattern | 
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'd suggest we remove this exception, as it doesn't bring any value.
| * [ ] Where addresses are fetched from the ChainLog, the variable name must match the value of the ChainLog key for that address (e.g. `MCD_VAT` rather than `vat`), EXCEPT where the archive pattern differs from this pattern | |
| * [ ] Where addresses are fetched from the ChainLog, the variable name must match the value of the ChainLog key for that address (e.g. `MCD_VAT` rather than `vat`) | 
| * [ ] Exec Doc for the specified date is found in the [`makerdao/community` GitHub repo](https://github.com/makerdao/community/tree/master/governance/votes) | ||
| * [ ] Exec Doc file name follows the format `Executive vote - Month DD, YYYY.md` | ||
| * [ ] Exec Doc for the specified date is found in the [`sky-ecosystem/executive-votes` GitHub repo](https://github.com/sky-ecosystem/executive-votes) | ||
| * [ ] Exec Doc is located in the directory matching the target spell date year (e.g. `2025/`) | 
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nit: we can be more generic here:
| * [ ] Exec Doc is located in the directory matching the target spell date year (e.g. `2025/`) | |
| * [ ] Exec Doc is located in the directory matching the target spell date year (e.g. `YYYY/`) | 
This PR applies several changes to the checklists in order to make them up-to-date.
Context: It has been opened to be used as a comparing point towards #52 and #53. The PR contents itself have been independently developed internally in September.
Differences from #52 and #53
Missing in this PR
Note: items marked with ✅ have been added to this PR.
✅ Foundry installation instructions have been changed:
pe-checklists/spell/spell-crafter-mainnet-workflow.md
Line 40 in 7cb1544
pe-checklists/spell/spell-reviewer-mainnet-checklist.md
Line 10 in 7b03757
Fetch addresses as type addressitem has been removed: https://github.com/sky-ecosystem/pe-checklists/pull/52/files#diff-fa1cbbb341c9d0c8abfea5082cffca265ac7f5e983b53c971d2e214bf731c155L115✅ Test name has been changed:
pe-checklists/spell/spell-crafter-mainnet-workflow.md
Line 125 in 7cb1544
✅ There was a repo link added to reviewer checklist:
pe-checklists/spell/spell-reviewer-mainnet-checklist.md
Line 3 in 7b03757
✅
ethercomment has been changed:pe-checklists/spell/spell-reviewer-mainnet-checklist.md
Line 202 in 7b03757
✅ Test has been added:
pe-checklists/spell/spell-reviewer-mainnet-checklist.md
Line 284 in 7b03757
Added in this PR
Commit order re-organized to match the actual flow:
pe-checklists/spell/spell-crafter-mainnet-workflow.md
Line 69 in a85c611
pe-checklists/spell/spell-crafter-mainnet-workflow.md
Line 86 in a85c611
pe-checklists/spell/spell-crafter-mainnet-workflow.md
Line 89 in a85c611
All possible tokens are mentioned for crafter:
pe-checklists/spell/spell-crafter-mainnet-workflow.md
Lines 134 to 135 in a85c611
Proof URL rule extended to include
httpscheck:pe-checklists/spell/spell-reviewer-mainnet-checklist.md
Lines 32 to 40 in a85c611
Single import layout rule changed:
pe-checklists/spell/spell-reviewer-mainnet-checklist.md
Line 52 in a85c611
All possible tokens are mentioned for reviewer:
pe-checklists/spell/spell-reviewer-mainnet-checklist.md
Lines 197 to 227 in a85c611
pe-checklists/spell/spell-reviewer-mainnet-checklist.md
Lines 253 to 282 in a85c611
tauerror is fixed:pe-checklists/spell/spell-reviewer-mainnet-checklist.md
Line 237 in a85c611
Code actuality check is added:
pe-checklists/spell/spell-reviewer-mainnet-checklist.md
Line 404 in a85c611
"Good to Handover" item is added:
pe-checklists/spell/spell-reviewer-mainnet-checklist.md
Line 436 in a85c611