Skip to content
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

[PSM Checklists] General improvements after feedback #35

Merged
merged 6 commits into from
Aug 20, 2024
Merged

[PSM Checklists] General improvements after feedback #35

Show file tree
Hide file tree
Changes from 5 commits
Commits
Show all changes
6 commits
Select commit Hold shift + click to select a range
84ea577
fix: bad precision on `mat`
amusingaxl Jul 24, 2024
71dbf16
refactor: reorganize checks
amusingaxl Aug 1, 2024
f8d21e2
refactor: normalize hyphenation for "onboarding" and "offboarding"
amusingaxl Aug 1, 2024
7857efc
feat: add checklist items for additional spell dependencies
amusingaxl Aug 1, 2024
afa642a
chore: add missing words to wordslist
amusingaxl Aug 1, 2024
a787516
refactor: address review comments
amusingaxl Aug 12, 2024
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
2 changes: 2 additions & 0 deletions .wordlist.txt
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -49,6 +49,7 @@ ExecLib
Filable
Fileable
fileables
FOREACH
Getter
github
goerli
Expand Down Expand Up @@ -92,6 +93,7 @@ Onboardings
OpenZeppelin
OSM
params
permalink
permissioned
permissioning
permittedDrop
Expand Down
115 changes: 59 additions & 56 deletions spell/psm-checklists.md
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2,8 +2,9 @@

## PSM Migration Checklist

- [ ] IF contracts are being on-boarded in the current spell, execute the full [LitePSM On-boarding Checklist](#litepsm-on-boarding-checklist)
- [ ] IF a PSM is deprecated in the migration, execute the full [PSM Off-boarding Checklist](#psm-off-boarding-checklist)
- [ ] IF LitePSM MOM is being on-boarded in the current spell, execute the full [LitePSM MOM Onboarding Checklist](#litepsm-mom-onboarding-checklist)
- [ ] IF LitePSM contracts are being on-boarded in the current spell, execute the full [LitePSM Onboarding Checklist](#litepsm-onboarding-checklist)
- [ ] IF a PSM is deprecated in the migration, execute the full [PSM Offboarding Checklist](#psm-offboarding-checklist)
- [ ] IF present, max amount of gems to move (`dstWant`) matches the Exec Sheet
- [ ] IF present, min amount of gems to keep (`srcKeep`) matches the Exec Sheet
- Source PSM:
Expand All @@ -30,10 +31,10 @@
- [ ] `vat.vice()` is unchanged after the spell execution
- [ ] `vat.sin(MCD_PAUSE_PROXY)` is unchanged after the spell execution

## LitePSM On-boarding Checklist
## LitePSM MOM Onboarding Checklist

- Deployed Contracts
- `DssLitePsm`
- [ ] `DssLitePsmMom`
- [ ] Optimizer is **enabled**
- [ ] Optimize runs is set to `200`
- [ ] Contract has been reviewed by minimum 2 specialized [Active Ecosystem Actors](https://mips.makerdao.com/mips/details/MIP101#2-8-2-active-ecosystem-actors)
Expand All @@ -44,15 +45,19 @@
- [ ] EITHER code diff check for each individual file
- [ ] OR bytecode verification tool such as `forge verify-bytecode`
- [ ] Deployed contract matches the code in the [repo](https://github.com/makerdao/dss-lite-psm/)
- Constructor params:
- [ ] ilk is named `LITE-PSM-{TOKEN_SYMBOL}-A` (i.e. `LITE-PSM-USDC-A`)
- [ ] `gem` matches the expected token address
- [ ] `daiJoin` matches `MCD_JOIN_DAI` from the chainlog
- [ ] `pocket` matches the pocket address in the Exec Sheet
- [ ] Deployer no longer has privileged access (`wards(deployer) == 0`)
- [ ] `MCD_PAUSE_PROXY` has been authed (i.e. `require(WardsLike(LITE_PSM).wards(MCD_PAUSE_PROXY) == 1)`)
- [ ] Sanity check: `DssLitePsm` has `type(uint256).max` approval to spend `gem` on behalf of `pocket`
- `DssLitePsmMom`
- [ ] Deployer is no longer the `owner`
- [ ] `MCD_PAUSE_PROXY` is the `owner` of the contract (i.e. `require(MomLike(MOM).owner() == MCD_PAUSE_PROXY`))
- Initialization:
- [ ] `DssLitePsmMom`:
- [ ] The chief (`MCD_ADM`) is set as the `authority` (i.e. `MomLike(MOM).setAuthority(MCD_ADM)`)
- Test Coverage:
- [ ] `DssLitePsmMom`:
- [ ] The chief (`MCD_ADM`) is set as the `authority` (i.e. `MomLike(MOM).setAuthority(MCD_ADM)`)

## LitePSM Onboarding Checklist

- Deployed Contracts
- [ ] `DssLitePsm`
- [ ] Optimizer is **enabled**
- [ ] Optimize runs is set to `200`
- [ ] Contract has been reviewed by minimum 2 specialized [Active Ecosystem Actors](https://mips.makerdao.com/mips/details/MIP101#2-8-2-active-ecosystem-actors)
Expand All @@ -63,89 +68,87 @@
- [ ] EITHER code diff check for each individual file
- [ ] OR bytecode verification tool such as `forge verify-bytecode`
- [ ] Deployed contract matches the code in the [repo](https://github.com/makerdao/dss-lite-psm/)
- [ ] Deployer is no longer the `owner`
- [ ] `MCD_PAUSE_PROXY` is the `owner` of the contract (i.e. `require(MomLike(MOM).owner() == MCD_PAUSE_PROXY`))
- `LitePsmJob`
- Constructor params:
- [ ] ilk is named `LITE-PSM-{TOKEN_SYMBOL}-A` (i.e. `LITE-PSM-USDC-A`)
- [ ] `gem` matches the expected token address
- [ ] `daiJoin` matches `MCD_JOIN_DAI` from the chainlog
- [ ] `pocket` matches the pocket address in the Exec Sheet
- [ ] Deployer no longer has privileged access (`wards(deployer) == 0`)
- [ ] `MCD_PAUSE_PROXY` has been authed (i.e. `require(WardsLike({LITE_PSM}).wards(MCD_PAUSE_PROXY) == 1)`)
- [ ] Sanity check: `DssLitePsm` has `type(uint256).max` approval to spend `gem` on behalf of `pocket`
- [ ] `LitePsmJob`
- [ ] Uses the default Solidity version in [`dss-cron`](https://github.com/makerdao/dss-cron)
- [ ] Optimizer is **enabled**
- [ ] Deployed contract matches the code in the [repo](https://github.com/makerdao/dss-cron)
- Constructor params:
- [ ] `_sequencer` matches `CRON_SEQUENCER` address from the chainlog
- [ ] `_litePsm` matches `DssLitePsm` address
- [ ] `_litePsm` matches `{LITE_PSM}` address
- [ ] `_rushThreshold` matches the Exec Sheet (it might be named as "fill threshold")
- [ ] `_gushThreshold` matches the Exec Sheet (it might be named as "trim threshold")
- [ ] `_cutThreshold` matches the Exec Sheet (it might be named as "chug threshold")
- Initialization
- `DssLitePsm`:
- [ ] `DssLitePsm`:
- [ ] `buf` is set to the value specified in the Exec Sheet
- [ ] `vow` is set to `MCD_VOW` from the chainlog
- [ ] `MCD_PAUSE_PROXY` is whitelisted to execute swaps with no fees (i.e. `KissLike(LITE_PSM).kiss(MCD_PAUSE_PROXY)`)
- [ ] `DssLitePsmMom` is authed (i.e. `RelyLike(LITE_PSM).rely(MOM)`)
- `DssLitePsmMom`:
- [ ] The chief (`MCD_ADM`) is set as the `authority` (i.e. `MomLike(MOM).setAuthority(MCD_ADM)`)
- `LitePsmJob`:
- [ ] `MCD_PAUSE_PROXY` is whitelisted to execute swaps with no fees (i.e. `KissLike({LITE_PSM}).kiss(MCD_PAUSE_PROXY)`)
- [ ] `DssLitePsmMom` is authed (i.e. `RelyLike({LITE_PSM}).rely(MOM)`)
- [ ] `LitePsmJob`:
- [ ] Job is added to `CRON_SEQUENCER`
- [ ] `MCD_VAT`: new ilk is initialized
- [ ] `MCD_JUG`: new ilk is initialized
- `MCD_SPOT`:
- [ ] `MCD_SPOT`:
Copy link
Contributor

Choose a reason for hiding this comment

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

I would recommend to only have checkboxes for points that can be checked / validated as "correct". In other words, if a reviewer doesn't need to go somewhere and actively ensure some rule, no checkbox should be present. Otherwise, the mental meaning of checkbox is mixed up and might lead to confusion during the review. In the worst case, a reviewer will tick some checkbox without even checking anything.

If you agree, please apply this rule beyond this line (above and below in the checklist). I can at least see the same issue on lines 37, 51, 54, 60, 79, 90, 95, 102, 108, 112, 124, 129, 130, 132, 139, 140, 147, 151

Copy link
Contributor Author

Choose a reason for hiding this comment

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

For 102 and 108 it kind of makes sense to have the check because they are conditional branches, so it you mark it, it means you are following that branch.

Also for visual consistency, maybe we should not be that strict that items with 1 single sub-item should be inlined.

Copy link
Contributor

@SidestreamColdMelon SidestreamColdMelon Aug 14, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

it kind of makes sense to have the check because they are conditional branches, so it you mark it, it means you are following that branch.

But ticking the items found in the branch would already indicate which branch you followed. We also don't normally have checkboxes in all other branching IFs. I would recommend to remove them, but it's kind of hard to draw the line between e.g. IF on line 5 and IF on line 104, I wouldn't make it a blocker – sadly it's no a code for which we can have a linter

- [ ] Sanity check: Dai parity (`par`) current value is 1 (`1 * RAY`)
- [ ] Collateralization ratio (`mat`) is set to 100% (`1 * WAD`) for the ilk
- IF a new `pip` is being used:
- [ ] Collateralization ratio (`mat`) is set to 100% (`1 * RAY`) for the ilk
- [ ] IF a new `pip` is being used:
- [ ] `pip` is set for the ilk
- [ ] `pip` is a `DSValue` instance
- [ ] The value on `pip` is set to 1 (`1 * WAD`)
- [ ] `pip` deployer is no longer the `owner`
- [ ] `MCD_PAUSE_PROXY` is the `owner` on `pip`
- OTHERWISE when reusing an existing `pip`:
- [ ] OTHERWISE when reusing an existing `pip`:
- [ ] `pip` is set for the ilk
- [ ] `pip` in the chainlog matches `gem` symbol (i.e. `PIP_USDC` for `USDC`)
- [ ] `ILK_REGISTRY`: new ilk is added to the registry
- `CHAINLOG`:
- [ ] `CHAINLOG`:
- [ ] `DssLitePsm` is added to the chainlog and `addresses_mainnet.sol` according to the Exec Sheet
- [ ] `pocket` is added to the chainlog and `addresses_mainnet.sol` according to the Exec Sheet
- [ ] `DssLitePsmMom` is added to the chainlog and `addresses_mainnet.sol` according to the Exec Sheet
- [ ] `LitePsmJob` is added to the chainlog and `addresses_mainnet.sol` according to the Exec Sheet
- [ ] IF a new `pip` is being added, it is added to the chainlog and `addresses_mainnet.sol` as `PIP_{TOKEN_SYMBOL}` (i.e. `PIP_USDC`)
- Test coverage:
- `DssLitePsm`:
- [ ] `DssLitePsm`:
- [ ] `vow` is set to `MCD_VOW`
- [ ] `buf` matches the Exec Sheet
- [ ] `MCD_PAUSE_PROXY` is whitelisted to execute swaps with no fees (i.e. `KissLike(LITE_PSM).bud(MCD_PAUSE_PROXY) == 1`)
- [ ] `DssLitePsmMom` is authed (i.e. `WardsLike(LITE_PSM).wards(MOM) == 1`)
- E2E tests:
- [ ] `MCD_PAUSE_PROXY` is whitelisted to execute swaps with no fees (i.e. `KissLike({LITE_PSM}).bud(MCD_PAUSE_PROXY) == 1`)
- [ ] `DssLitePsmMom` is authed (i.e. `WardsLike({LITE_PSM}).wards(MOM) == 1`)
- [ ] E2E tests:
- [ ] `buyGem` works as expected
- [ ] `sellGem` works as expected
- [ ] `buyGemNoFee` works as expected
- [ ] `sellGemNoFee` works as expected
- `DssLitePsmMom`:
- Chief (`MCD_ADM`) is set as the `authority` (i.e. `MomLike(MOM).authority() == MCD_ADM`)
- E2E tests:
- [ ] `halt` works as expected
- `LitePsmJob`:
- [ ] `DssLitePsmMom`:
- [ ] E2E tests:
- [ ] `halt` prevents new swaps in `{LITE_PSM}`
- [ ] `LitePsmJob`:
- [ ] Job is added to `CRON_SEQUENCER` (i.e.: `SequencerLike(CRON_SEQUENCER).hasJob(LITE_PSM_JOB) == true`)
- [ ] `_sequencer` matches `CRON_SEQUENCER` address from the chainlog
- [ ] `_litePsm` matches `DssLitePsm` address
- [ ] `_rushThreshold` matches the Exec Sheet (it might be named as "fill threshold")
- [ ] `_gushThreshold` matches the Exec Sheet (it might be named as "trim threshold")
- [ ] `_cutThreshold` matches the Exec Sheet (it might be named as "chug threshold")
- E2E tests:
- [ ] `workable` returns the correct value:
- [ ] IF `rush() > 0`, returns `(true, litePsm.fill.selector)`
- [ ] OTHERWISE IF `cut() > 0`, returns `(true, litePsm.chug.selector)`
- [ ] OTHERWISE IF `gush() > 0`, returns `(true, litePsm.trim.selector)`
- [ ] OTHERWISE returns `(false, "")`
- [ ] `sequencer` matches `CRON_SEQUENCER` address from the chainlog
- [ ] `litePsm` matches `{LITE_PSM}` address
- [ ] `rushThreshold` matches the Exec Sheet (it might be named as "fill threshold")
- [ ] `gushThreshold` matches the Exec Sheet (it might be named as "trim threshold")
- [ ] `cutThreshold` matches the Exec Sheet (it might be named as "chug threshold")
- [ ] E2E tests:
- [ ] `work` has the desired effect:
- [ ] IF `rush() > 0`, `fill` is called
- [ ] OTHERWISE IF `cut() > 0`, `chug` is called
- [ ] OTHERWISE IF `gush() > 0`, `trim` is called
- [ ] OTHERWISE reverts
- [ ] `MCD_VAT`: `urns[ilk][LITE_PSM].ink` is set to the max value that will not cause an overflow (`int256(type(uint256).max / RAY)`)
- [ ] IF `rush() > _rushThreshold`, `fill` is called
- [ ] OTHERWISE IF `cut() > _cutThreshold`, `chug` is called
- [ ] OTHERWISE IF `gush() > _gushThreshold`, `trim` is called
- [ ] OTHERWISE it reverts
- [ ] `MCD_VAT`: `urns[ilk][{LITE_PSM}].ink` is set to the max value that will not cause an overflow (`int256(type(uint256).max / RAY)`)
- [ ] `MCD_JUG`: Stability fee (`duty`) is set to 0% (`1 * RAY`) for the ilk (:information_source: covered in `config.sol`)
- `MCD_SPOT`:
- [ ] `MCD_SPOT`:
- [ ] `spotter(DST_ILK).mat` is set to 100% (`1 * RAY`) (:information_source: covered in `config.sol`)
- [ ] `spotter(DST_ILK).pip` is set correctly
- [ ] `CHAINLOG`: version is bumped: `{x}.{y}.{z+1}` (:information_source: covered in `config.sol`)
- `ILK_REGISTRY`:
- [ ] `ILK_REGISTRY`:
- [ ] New ilk is added to the registry
- [ ] `name` matches `gem` name
- [ ] `symbol` matches `gem` symbol
Expand All @@ -155,7 +158,7 @@
- [ ] `gemJoin` is set to `address(0)`
- [ ] `clip` is set to `address(0)`

## PSM Off-boarding Checklist
## PSM Offboarding Checklist

- [ ] ilk is removed from `AutoLine`
- [ ] ilk `line` is set to `0`
Expand Down
5 changes: 5 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 @@ -135,6 +135,11 @@
* [`DssExecLib.setGlobalDebtCeiling(amount)`](https://github.com/makerdao/dss-exec-lib/blob/v0.0.9/src/DssExecLib.sol#L428)
* [`DssExecLib.increaseGlobalDebtCeiling(amount)`](https://github.com/makerdao/dss-exec-lib/blob/v0.0.9/src/DssExecLib.sol#L436)
* [`DssExecLib.decreaseGlobalDebtCeiling(amount)`](https://github.com/makerdao/dss-exec-lib/blob/v0.0.9/src/DssExecLib.sol#L445C14-L445C39)
* IF additional dependencies (i.e. `./src/dependencies/` directory) are present:
* [ ] IF the dependencies contracts/libraries have been audited
* [ ] FOREACH contract/library ensure its content matches (i.e. diff check) the latest audited version
* [ ] OTHERWISE obtain the permalink to the relevant repository from a trusted party (i.e. Gov Facilitators)
* [ ] FOREACH contract/library ensure its content matches (i.e. diff check) the one from the permalink
* IF onboarding is present
* [ ] Insert and follow the relevant checklists below:
* [Collateral Onboarding](./collateral-onboarding-checklist.md)
Expand Down
Loading