search

makerdao / spells-mainnet

Staging repo for MakerDAO weekly executive spells
GNU Affero General Public License v3.0
107 stars 43 forks source link

2023-05-17: Final offboarding of YFI-A, LINK-A, MATIC-A and UNIV2USDCETH-A; MKR payments, DAI streams #343

Closed SidestreamColdMelon closed 1 year ago

SidestreamColdMelon commented 1 year ago

Description

This PR implements 2023-05-17 mainnet spell based on the executive vote summary, adds tests for the offboarded collaterals, DAI vesting and MKR transfers.

Since there are no proper crafter checklist for offboarding collaterals (only a reviewer one, for lerp-based offboardings), we did our own analysis of all previous offboardings in the spell archive and compiled a list of all possible actions. This way, everyone can easily follow and validate our decisions in the code. More information about it is in the 2023-05-17 goerli spell.

Superset of all offboarding actions, based on the archive

  1. Add a comment with the link to the poll (required for full offboardings) and to the forum (always required), example code -> ✅ Done in this PR
  2. Setup clipper (if not yet done during onboarding), example code -> ☑️ Clipper contracts already exist for all offboarded collaterals, checked via chainlog
  3. Set debt ceiling to 0 (if not yet done in the first stage), example code -> ☑️ Dept is already 0, confirmed on etherscan via VAT.ilks().line being 0 for every collateral
  4. Decrease global debt ceiling (always, if collateral debt ceiling has been changed), example code -> ☑️ Was previously done in the 2023-04-26 and 2023-03-16 spells
  5. Remove auto line (always, if previously enabled), example code -> ☑️ Was previously done, confirmed on etherscan via DssAutoLine.ilks() outputting all 0s
  6. Remove liquidation penalty (if requested), example code -> ✅ Done in this PR
  7. Remove keeper incentive (if requested), example code -> ✅ Done in this PR
  8. Enable liquidations via the clipper parameter "stopped" (if not yet enabled), example code -> ☑️ It's already enabled for all collaterals, confirmed via clipper.stopped() being 0 for each collateral
  9. Liquidate vaults (as requested, depends on the amounts being liquidated)
    • Set liquidation ratio very high, example code -> ✅ Done in this PR
    • OR set very long linear interpolation price decrease, example code
  10. Adjust max liquidation amount (if requested by the govalpha / risk), example code
    • Not requested
  11. Update spotter price (always, except for when linear interpolation is being used), example code
    • ✅ Done in this PR
  12. Test (depending on how it's being liquidated, see Liquidate vaults above)

Contribution Checklist

Checklist

SidestreamSweatyPumpkin commented 1 year ago
Checklist * [ ] Office Hours * [x] On (Collateral Onboarding, Keepers, Integrations, ...) * [x] matches exec doc ❔ exec doc: https://github.com/makerdao/community/blob/master/governance/votes/Executive%20Vote%20-%20May%2017%2C%202023.md not sure what should "match" exactly, but the dates(timestamps) seem fine * [ ] Exec Hash * [x] Run `make exec-hash` for current date, or `date=YYYY-MM-DD` * [x] Executive vote file name and date is correct * [x] [community](https://github.com/makerdao/community) repo commit hash corresponds to latest change 📝 the comment is outdated, it seems like there're more commits and the link is not for the latest hash. But hash of the exec doc matches either way. * [x] Raw GitHub URL is correct * [x] Exec hash is correct * [x] `description` date in `DssSpell.sol` matches exec copy date * [x] 30 Days Expiry * [x] `lib` * [x] `dss-exec-lib` * [x] if submodule upgrades are present make sure `dss-exec-lib` is synced as well not present * [ ] git submodule hash matches tag latest release version or above 📓 skipping for now due to previous agreement * [ ] `dss-test` * [x] `dss-interfaces` * [x] git submodule hash matches github master commit * [x] `forge-std` * [x] git submodule hash matches github tag latest release version does not seem to be the case, fine for now * [ ] `dss-interfaces` 📓 not used, skipping * [ ] used in the current spell * [ ] cleanup previous ones * [ ] ensure only single import layout is used (e.g. `import "dss-interfaces/dss/VatAbstract.sol";`) * [ ] Static Interfaces * [x] ensure they match `dss-interfaces` * [ ] check on-chain interface of deployed contract via `cast interfaces ` to ensure correctness 📓 types match, arguments are named in `cast` but not in the declared interface. Fine for me * [x] interface naming style should match with `Like` suffix (e.g. `VatLike`) * [x] ensure they only list used functions in spell code * [ ] Rates match 📓 not used, skipping * [ ] Compare against [IPFS](https://ipfs.io/ipfs/QmVp4mhhbwWGTfbh2BzwQB9eiBrQBKiqcPRZCaAxNUaar6) * [ ] Check manually via `make rates pct=` (e.g. pct=0.75, for 0.75%) * [ ] Variable visibility declared as internal * [ ] Math matches * [ ] Internal Precision * [x] `WAD = 10**18` * [x] `RAY = 10**27` * [ ] `RAD = 10**45` 📓 not used * [x] Ensure they match with [ds-math](https://github.com/dapphub/ds-math/blob/master/src/math.sol) and the [Numerical Ranges](https://github.com/makerdao/dss/wiki/Numerical-Ranges#notation) * [x] Variable visibility declared as internal * [ ] Units 📓 not used, skipping * [ ] `HUNDRED = 10**2` * [ ] `THOUSAND = 10**3` * [ ] `MILLION = 10**6` * [ ] `BILLION = 10**9` * [ ] Ensure they match with [config](https://github.com/makerdao/spells-mainnet/blob/master/src/test/config.sol) * [ ] Variable visibility declared as internal * [ ] Deployed Contracts 📓 no contracts deployed in this spell, nor are external contracts used, skipping * [ ] Verified on etherscan * [ ] Optimizations match Repo * [ ] `GNU AGPLv3` license * [ ] Constructor args ok (e.g. `vat`, `dai`, `dog`, ...) * [ ] Match [ChainLog](https://chainlog.makerdao.com/) * [ ] Wards ok (pause proxy relied, deployer denied) * [ ] Check whether the contract requires to rely the ESM in the spell (in order to allow de-authing the pause proxy during Emergency Shutdown, via `denyProxy`). * [ ] Matches corresponding github source code (i.e. diffcheck via vscode `code --diff etherscan.sol github.sol`) * [ ] Ensure deployer address is included into `addresses_deployers.sol` (**to keep up to date**) * [ ] External Contracts Calls (e.g. Starknet) * [ ] Target Contract don't block spell execution * [ ] External call is NOT delegate call * [ ] Target Contract doesn't have permissions on the Vat * [ ] Target Contract doesn't do anything untoward (e.g. interacting with unsafe contracts) * [ ] MCD Pause Proxy doesn't give any approvals * [ ] All possible actions of the Target Contract are documented * [ ] Target contract is not upgradable * [ ] Target Contract is included in the ChainLog * [ ] Test Coverage is comprehensive * [x] Risk Parameters Changes * [x] `dog.ilk.chop` ([liquidationPenalty](https://github.com/makerdao/dss-exec-lib/blob/v0.0.9/src/DssExecLib.sol#L689)) * [x] `spotter.ilk.mat` ([liquidationRatio](https://github.com/makerdao/dss-exec-lib/blob/v0.0.9/src/DssExecLib.sol#L709)) * [x] `clip.chip` ([kprPctReward](https://github.com/makerdao/dss-exec-lib/blob/v0.0.9/src/DssExecLib.sol#L750)) * [x] `clip.tip`([kprFlatReward](https://github.com/makerdao/dss-exec-lib/blob/v0.0.9/src/DssExecLib.sol#L760)) * [ ] Autoline Changes 📓 skipping * [setIlkAutoLineParameters](https://github.com/makerdao/dss-exec-lib/blob/v0.0.9/src/DssExecLib.sol#L648) * [ ] `ilk` * [ ] `line` * [ ] `gap` * [ ] `ttl` * [setIlkAutoLineDebtCeiling](https://github.com/makerdao/dss-exec-lib/blob/v0.0.9/src/DssExecLib.sol#L658) * [ ] `ilk` * [ ] `line` * [ ] Onboarding 📓 skipping * [ ] [Collateral Onboarding](./collateral-onboarding-checklist.md) * [ ] [RWA Onboarding](./rwa-onboarding-checklist.md) * [ ] [Teleport Onboarding](./teleport-onboarding-checklist.md) * [ ] Offboarding (Lerp `mat`) * [ ] 2nd Stage Spell Actions * [x] Set Ilk Liquidation Penalty `chop` to `0` * [x] Set Keeper Incentive Flat Rate `tip` to `0` * [x] Check IF `chip` is required to be adjusted as well * [ ] RWA Updates 📓 skipping * [ ] `doc` * [ ] `init` the `RwaLiquidationOracle` to reset the `doc` * [ ] Sanity Check `pip` must be set (not the zero address) * [ ] `ilk` follows format "RWAXXX-A" * [ ] `val` price ignored (`0`) if `init` has already been called * [ ] `doc` new legal document (IPFS HASH) matches Doc (or Forum Post) * [ ] `tau` parameter used is the old `tau` value * [ ] Autoline (`line`) + Liquidation Oracle Price Bump (`val`) * [ ] Enable Autoline * [ ] `ilk` follows format "RWAXXX-A" * [ ] `line` (max debt ceiling) * [ ] `gap` * [ ] `ttl` * [ ] `bump` `RwaLiquidationOracle` with new computed increased price (`val`) * [ ] ensure `val` is set accordingly with autoline max debt ceiling (`line`) * [ ] `val` should enable DAI to be drawn over the loan period while taking into account the configured `ink` amount, interest rate and liquidation ratio * [ ] Poke `spotter` to pull in the new price * [ ] Debt Ceiling (`line`) + Liquidation Oracle Price Bump (`val`) * [ ] Increase Ilk Debt Ceiling (set DC + increase Global DC) * [ ] `bump` `RwaLiquidationOracle` with new computed increased price (`val`) * [ ] `val` should enable DAI to be drawn over the loan period while taking into account the configured `ink` amount, interest rate and liquidation ratio * [ ] Poke `spotter` to pull in the new price * [ ] Payments * [ ] Streams (`DssVest`) * [x] `DssVest` Interface is correct * [ ] Ensure that `cap` > max new vest `tot`/`tau` otherwise file cap as well 📝 not required by the exec doc, so PR does not have it. * [x] Timestampts match Doc (`bgn`, `fin`) * [x] CUs Addresses match Doc (`usr`) * [x] Amount matches Doc (`tot`, if decimals are present consider using `ether`) * [x] Vesting Duration matches Doc (`tau`) * [x] Cliff Duration matches Doc (`eta`) * [x] Restricted (by default) * [x] Manager match Doc (`mgr`, set to zero for DAI streams by default) * [x] CUs MKR Transfers * [x] Recipient Addresses match Doc * [x] Transfers Amounts match Doc * [x] Follows Previous Patterns * [ ] Direct SB DAI Payment 📓 skipping * [ ] Recipient Addresses match Doc * [ ] Payment Amounts match Doc * [ ] Follow Previous Patterns * [x] Ensure Recipient Addresses match `addresses_wallets.sol` * [ ] ChainLog 📓 skipping * [ ] Bump ChainLog, accordingly with spec (major, minor, patch) * [ ] MAJOR -> New Vat * [ ] MINOR -> Core Module (DSS) Update (e.g. Flapper) * [ ] PATCH -> Collateral addition or addition/modification * [ ] `addresses_mainnet.sol` matches spell code 📓 skipping * [x] Ensure every spell variable is declared as public/internal * [x] Ensure `immutable` visibility is only used when fetching addresses from the `ChainLog` via `DssExecLib.getChangelogAddress` and `constant` is used instead for static addresses * [x] Spell actions match [GovAlpha Spell Content Sheet](https://docs.google.com/spreadsheets/d/1w_z5WpqxzwreCcaveB2Ye1PP5B8QAHDglzyxKHG3CHw) and hashed exec doc * [x] Tests PASS * [x] Ensure Good Coverage * [x] Ensure every test function is declared as public if enabled or private if disabled * [x] Local Tests and CI PASS
tests' output ``` Running 2 tests for src/test/starknet.t.sol:StarknetTests [PASS] testStarknet() (gas: 1405623) [PASS] testStarknetSpell() (gas: 2346) Test result: ok. 2 passed; 0 failed; finished in 55.32s Running 21 tests for src/DssSpell.t.sol:DssSpellTest [PASS] testAuth() (gas: 9223371487102701143) [PASS] testAuthInSources() (gas: 9223371487099056011) [PASS] testBytecodeMatches() (gas: 2720125) [PASS] testCastCost() (gas: 1259107) [PASS] testChainlogValues() (gas: 9554910) [PASS] testChainlogVersionBump() (gas: 4651351) [PASS] testContractSize() (gas: 8962) [PASS] testDeployCost() (gas: 2704785) [PASS] testFailNotScheduled() (gas: 14406) [PASS] testFailTooEarly() (gas: 417614) [PASS] testFailTooLate() (gas: 417527) [PASS] testFailWrongDay() (gas: 417617) [PASS] testGeneral() (gas: 36768834) [PASS] testIlkClipper() (gas: 3654384) [PASS] testMKRPayments() (gas: 1352679) [PASS] testNewChainlogValues() (gas: 1256243) [PASS] testNextCastTime() (gas: 446343) [PASS] testOnTime() (gas: 1246700) [PASS] testPSMs() (gas: 2788471) [PASS] testUseEta() (gas: 352470) [PASS] testVestDAI() (gas: 1495339) Test result: ok. 21 passed; 0 failed; finished in 891.04s ```
LongForWisdom commented 1 year ago
  • Ensure that cap > max new vest tot/tau otherwise file cap as well 📝 not required by the exec doc, so PR does not have it.

If by 'exec doc' you mean the spell copy and/or the contents sheet, you should likely check this anyway. We're not going to have any idea if this is an issue on our side when the copy + sheet is created.

Apologies if I've misunderstood you here.

SidestreamSweatyPumpkin commented 1 year ago

https://github.com/makerdao/spells-mainnet/pull/343#issuecomment-1551110298

Explainer

If by 'exec doc' you mean the spell copy and/or the contents sheet

https://github.com/makerdao/community/blob/master/governance/votes/Executive%20Vote%20-%20May%2017%2C%202023.md

that is what meant by the exec doc.

for comparison, the previous spell raises cap and it's mentioned in the doc

From this above and the tests passing without require failed i take that:

Check manually

you should likely check this anyway

Thanks for your comment! 🥇

SidestreamColdMelon commented 1 year ago

Technically, we might also test that it will be possible to pay out all streams including (but not limited) to the streams added in the current spell. But I haven't seen this done in the previous tests. This kind of test is better suitable for the base test that is executed each time, no matter the changes. If everyone agrees that it's needed, I can add it here

The cap is only changed via file, so it can't be spent like, for example, allowance. Then, the generic test is not needed

amusingaxl commented 1 year ago

Content Validation

Offboarding Parameter Changes

Payments

Prime Constitutional Delegates

Reserve Constitutional Delegates

Dai Budget Streams

[ ] Budget DAI Amount Start Date End Date Destination Address
[x] Governance Security Engineering 2,200,000 2023-05-01 2024-05-01 0x569fAD613887ddd8c1815b56A00005BCA7FDa9C0
[x] Multichain Engineering 2,300,000 2023-05-01 2024-05-01 0x868B44e8191A2574334deB8E7efA38910df941FA

MKR Transfer

SidestreamColdMelon commented 1 year ago

The spell has been deployed https://etherscan.io/address/0xa1c423ee0bbc927ef5809c7ebb24c86d4284e431#code

SidestreamSweatyPumpkin commented 1 year ago
LongForWisdom commented 1 year ago

I have a couple of notes. Be aware that I'm not super familiar with this process, so apologies in advance if I'm misunderstanding things here.

  • [x] Ensure Etherscan Libraries Used matches DssExecLib Latest Release

  • [x] git submodule hash matches dss-exec-lib latest release's tag commit and inspect diffs if doesn't match to ensure expected behaviour

  • Ensure Etherscan Libraries Used matches DssExecLib Latest Release ❗ Ignored for now

  • [ ] git submodule hash matches dss-exec-lib latest release's tag commit and inspect diffs if doesn't match to ensure expected behaviour ❗ Ignored for now

I'm seeing one of you ignored these, and one didn't? My understanding from speaking to Arbiter was that the latest version of dss-exec-lib in the repo has not been deployed on chain. Is this related to why you skipped these @amusingaxl?

  • Local Tests and CI PASS

@SidestreamSweatyPumpkin, I believe it's customary to post the test output as part of these comments as @amusingaxl did. Do you mind posting your outputs here as well?

SidestreamColdMelon commented 1 year ago

My understanding from speaking to Arbiter was that the latest version of dss-exec-lib in the repo has not been deployed on chain

That's correct. The latest version of dss-exec-lib that is used in all recent spells hasn't been redeployed since last February. @The-Arbiter opened the PR to the checklists to change this particular point and indicate that it's indeed not the strict requirement at the moment. Nevertheless, it still makes sense to check that all methods that we're calling exist in the deployed version and didn't change their behaviour. Here is the comparison between the release and the 69b658f commit we're using, although you might want to use diffchecker.com to compare actual on-chain code with the state on 69b658f.

SidestreamSweatyPumpkin commented 1 year ago

Do you mind posting your outputs here as well?


./scripts/test-dssspell-forge.sh match="" block=""
Using DssExecLib at: 0x8De6DDbCd5053d32292AAA0D2105A32d108484a6
[⠒] Compiling...
[⠊] Compiling 104 files with 0.8.16
[⠑] Solc 0.8.16 finished in 5.86s
Compiler run successful!

Running 2 tests for src/test/starknet.t.sol:StarknetTests
[PASS] testStarknet() (gas: 1405623)
[PASS] testStarknetSpell() (gas: 2346)
Test result: ok. 2 passed; 0 failed; finished in 70.23s

Running 21 tests for src/DssSpell.t.sol:DssSpellTest
[PASS] testAuth() (gas: 9223371487102701143)
[PASS] testAuthInSources() (gas: 9223371487099056011)
[PASS] testBytecodeMatches() (gas: 2720125)
[PASS] testCastCost() (gas: 1259107)
[PASS] testChainlogValues() (gas: 9554910)
[PASS] testChainlogVersionBump() (gas: 4651351)
[PASS] testContractSize() (gas: 8962)
[PASS] testDeployCost() (gas: 2704785)
[PASS] testFailNotScheduled() (gas: 14406)
[PASS] testFailTooEarly() (gas: 417614)
[PASS] testFailTooLate() (gas: 417527)
[PASS] testFailWrongDay() (gas: 417617)
[PASS] testGeneral() (gas: 36770931)
[PASS] testIlkClipper() (gas: 3654384)
[PASS] testMKRPayments() (gas: 1352679)
[PASS] testNewChainlogValues() (gas: 1256243)
[PASS] testNextCastTime() (gas: 446343)
[PASS] testOnTime() (gas: 1246700)
[PASS] testPSMs() (gas: 2788471)
[PASS] testUseEta() (gas: 352470)
[PASS] testVestDAI() (gas: 1495339)
Test result: ok. 21 passed; 0 failed; finished in 1325.18s
LongForWisdom commented 1 year ago

I see, thank you for the explanation @SidestreamColdMelon. I did confirm that we were using the correct version of ExecLib on the deployed spell via etherscan. Given it's been used for over a year, and that you guys have checked it manually, I'm comfortable not checking it myself. I really appreciate the links and the step-by-step instructions though.