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.

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

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

Merged
merged 10 commits into from
May 17, 2023

Conversation

SidestreamColdMelon
Copy link
Contributor

@SidestreamColdMelon SidestreamColdMelon commented May 16, 2023

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

  • PR title matches expected spell date
  • Code approved
  • Tests approved
  • CI Tests pass

Checklist

  • Every contract variable/method declared as public/external private/internal
  • Consider if this PR needs the officeHours modifier override
  • Verify expiration (30 days unless otherwise specified)
  • Verify hash in the description matches here
  • Validate all addresses used are in changelog or known
  • Notify any external teams affected by the spell so they have the opportunity to review
  • Deploy spell ETH_GAS_LIMIT="XXX" ETH_GAS_PRICE="YYY" make deploy
  • Verify mainnet contract on etherscan
  • Change test to use mainnet spell address and deploy timestamp
  • Run make archive-spell or make date="YYYY-MM-DD" archive-spell to make an archive directory and copy DssSpell.sol, DssSpell.t.sol, DssSpell.t.base.sol, and DssSpellCollateralOnboarding.sol
  • squash and merge this PR

@SidestreamSweatyPumpkin
Copy link
Contributor

SidestreamSweatyPumpkin commented May 17, 2023

Checklist
  • Office Hours
  • Exec Hash
    • Run make exec-hash for current date, or date=YYYY-MM-DD
      • Executive vote file name and date is correct
      • 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.
      • Raw GitHub URL is correct
      • Exec hash is correct
    • description date in DssSpell.sol matches exec copy date
  • 30 Days Expiry
  • lib
    • dss-exec-lib
      • 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
      • dss-interfaces
        • git submodule hash matches github master commit
      • forge-std
        • 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
    • ensure they match dss-interfaces
    • check on-chain interface of deployed contract via cast interfaces <contract_address> to ensure correctness
      📓 types match, arguments are named in cast but not in the declared interface. Fine for me
    • interface naming style should match with Like suffix (e.g. VatLike)
    • ensure they only list used functions in spell code
  • Rates match
    📓 not used, skipping
    • Compare against IPFS
    • Check manually via make rates pct=<pct> (e.g. pct=0.75, for 0.75%)
    • Variable visibility declared as internal
  • Math matches
    • Internal Precision
      • WAD = 10**18
      • RAY = 10**27
      • RAD = 10**45
        📓 not used
      • Ensure they match with ds-math and the Numerical Ranges
      • 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
      • 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, ...)
    • 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
  • Risk Parameters Changes
  • Autoline Changes
    📓 skipping
  • Onboarding
    📓 skipping
  • Offboarding (Lerp mat)
    • 2nd Stage Spell Actions
      • Set Ilk Liquidation Penalty chop to 0
      • Set Keeper Incentive Flat Rate tip to 0
      • 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)
      • 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.
      • Timestampts match Doc (bgn, fin)
      • CUs Addresses match Doc (usr)
      • Amount matches Doc (tot, if decimals are present consider using ether)
      • Vesting Duration matches Doc (tau)
      • Cliff Duration matches Doc (eta)
      • Restricted (by default)
      • Manager match Doc (mgr, set to zero for DAI streams by default)
    • CUs MKR Transfers
      • Recipient Addresses match Doc
      • Transfers Amounts match Doc
      • Follows Previous Patterns
    • Direct SB DAI Payment
      📓 skipping
      • Recipient Addresses match Doc
      • Payment Amounts match Doc
      • Follow Previous Patterns
    • 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
  • Ensure every spell variable is declared as public/internal
  • Ensure immutable visibility is only used when fetching addresses from the ChainLog via DssExecLib.getChangelogAddress and constant is used instead for static addresses
  • Spell actions match GovAlpha Spell Content Sheet and hashed exec doc
  • Tests PASS
    • Ensure Good Coverage
    • Ensure every test function is declared as public if enabled or private if disabled
  • 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

Copy link
Contributor

@SidestreamSweatyPumpkin SidestreamSweatyPumpkin left a comment

Choose a reason for hiding this comment

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

LGTM

@LongForWisdom
Copy link

  • 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
Copy link
Contributor

#343 (comment)

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:

  • the cap should not be raised here.
  • the cap is not exceeded.

Check manually

you should likely check this anyway


Thanks for your comment! 🥇

@SidestreamColdMelon
Copy link
Contributor Author

SidestreamColdMelon commented May 17, 2023

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

Copy link
Contributor

@amusingaxl amusingaxl left a comment

Choose a reason for hiding this comment

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

Nitpicks on the comments in the MKR transfers.

Some of the addresses in the comments are not in the checksum format and don't match exactly the ones declared as constants.

image

src/DssSpell.sol Outdated Show resolved Hide resolved
src/DssSpell.sol Outdated Show resolved Hide resolved
src/DssSpell.sol Outdated Show resolved Hide resolved
src/DssSpell.sol Outdated Show resolved Hide resolved
src/DssSpell.sol Show resolved Hide resolved
@amusingaxl
Copy link
Contributor

Content Validation

Offboarding Parameter Changes

  • YFI-A
    • Decrease the Liquidation Penalty (chop) to 0%.
    • Decrease the Flat Kick Incentive (tip) to 0%.
    • Decrease the Proportional Kick Incentive (chip) to 0%.
    • Increase the Liquidation Ratio (mat) to 10,000%.
  • LINK-A
    • Decrease the Liquidation Penalty (chop) to 0%.
    • Decrease the Flat Kick Incentive (tip) to 0%.
    • Decrease the Proportional Kick Incentive (chip) to 0%.
    • Increase the Liquidation Ratio (mat) to 10,000%.
  • MATIC-A
    • Decrease the Liquidation Penalty (chop) to 0%.
    • Decrease the Flat Kick Incentive (tip) to 0%.
    • Decrease the Proportional Kick Incentive (chip) to 0%.
    • Increase the Liquidation Ratio (mat) to 10,000%.
  • UNIV2USDCETH-A
    • Decrease the Liquidation Penalty (chop) to 0%.
    • Decrease the Flat Kick Incentive (tip) to 0%.
    • Decrease the Proportional Kick Incentive (chip) to 0%.
    • Increase the Liquidation Ratio (mat) to 10,000%.

Payments

Prime Constitutional Delegates

  • 23.8 MKR ✔️ will be transferred to 0xDefensor ✔️ at 0x9542b441d65B6BF4dDdd3d4D2a66D8dCB9EE07a9.
  • 23.8 MKR ✔️ will be transferred to BONAPUBLICA ✔️ at 0x167c1a762B08D7e78dbF8f24e5C3f1Ab415021D3.
  • 23.8 MKR ✔️ will be transferred to Frontier Research at 0xA2d55b89654079987CF3985aEff5A7Bd44DA15A8.
  • 23.8 MKR ✔️ will be transferred to GFX ✔️ Labs at 0x9B68c14e936104e9a7a24c712BEecdc220002984.
  • 23.8 MKR ✔️ will be transferred to QGov ✔️ at 0xB0524D8707F76c681901b782372EbeD2d4bA28a6.
  • 23.8 MKR ✔️ will be transferred to TRUE NAME ✔️ at 0x612F7924c367575a0Edf21333D96b15F1B345A5d.
  • 23.8 MKR ✔️ will be transferred to vigilant ✔️ at 0x2474937cB55500601BCCE9f4cb0A0A72Dc226F61.

Reserve Constitutional Delegates

  • 5.95 MKR ✔️ will be transferred to CodeKnight ✔️ at 0xf6006d4cF95d6CB2CD1E24AC215D5BF3bca81e7D.
  • 5.95 MKR ✔️ will be transferred to Flip Flop Flap ✔️ Delegate LLC at 0x3d9751EFd857662f2B007A881e05CfD1D7833484.
  • 5.95 MKR ✔️ will be transferred to PBG ✔️ at 0x8D4df847dB7FfE0B46AF084fE031F7691C6478c2.
  • 5.95 MKR ✔️ will be transferred to UPMaker ✔️ at 0xbB819DF169670DC71A16F58F55956FE642cc6BcD.

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

  • As per their successful MKR vesting proposal, 103.16 MKR will be transferred to the Data Insights Core Unit (DIN-001) wallet 0x7327Aed0Ddf75391098e8753512D8aEc8D740a1F, if this executive proposal passes.


assertTrue(vest.valid(26)); // check for valid contract
// Check the first stream
uint256 govSecurityEngineeringStreamId = prevStreamCount + 1;
Copy link
Contributor

Choose a reason for hiding this comment

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

++ on this... way better than hardcoded IDs.

amusingaxl
amusingaxl previously approved these changes May 17, 2023
Copy link
Contributor

@amusingaxl amusingaxl left a comment

Choose a reason for hiding this comment

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

Checklist

  • Office Hours
    • matches exec doc
  • Exec Hash
    • Run make exec-hash for current date, or date=YYYY-MM-DD
      • Executive vote file name and date is correct
      • community repo commit hash corresponds to latest change
      • Raw GitHub URL is correct
      • Exec hash is correct
    • description date in DssSpell.sol matches exec copy date
  • 30 Days Expiry
  • Static Interfaces
    • ensure they match dss-interfaces
    • check on-chain interface of deployed contract via cast interfaces <contract_address> to ensure correctness
    • interface naming style should match with Like suffix (e.g. VatLike)
    • ensure they only list used functions in spell code
  • Math matches
    • Internal Precision
      • WAD = 10**18
      • RAY = 10**27
      • Ensure they match with ds-math and the Numerical Ranges
      • Variable visibility declared as internal
  • Risk Parameters Changes
  • Offboarding (Lerp mat)
    • 2nd Stage Spell Actions
      • Set Ilk Liquidation Penalty chop to 0
      • Set Keeper Incentive Flat Rate tip to 0
      • Check IF chip is required to be adjusted as well
      • Use DssExecLib.linearInterpolation Not required ✔️
  • Payments
    • Streams (DssVest)
      • DssVest Interface is correct
      • Ensure that cap > max new vest tot/tau otherwise file cap as well
      • Timestampts match Doc (bgn, fin)
      • CUs Addresses match Doc (usr)
      • Amount matches Doc (tot, if decimals are present consider using ether)
      • Vesting Duration matches Doc (tau)
      • Cliff Duration matches Doc (eta)
      • Restricted (by default)
      • Manager match Doc (mgr, set to zero for DAI streams by default)
      • IF DssVestTransferrable is used | ✔️ Not applicable
        • Ensure DssVestTransferrable allowance is increased by new vesting delta (by approving the transferrable vest contract to allowance + new total amount streamed)
    • CUs MKR Transfers
      • Recipient Addresses match Doc
      • Transfers Amounts match Doc
      • Follows Previous Patterns
    • Ensure Recipient Addresses match addresses_wallets.sol
  • addresses_mainnet.sol matches spell code
  • Ensure every spell variable is declared as public/internal
  • Ensure immutable visibility is only used when fetching addresses from the ChainLog via DssExecLib.getChangelogAddress and constant is used instead for static addresses
  • Spell actions match GovAlpha Spell Content Sheet and hashed exec doc
  • Tests PASS
    • Ensure Good Coverage
    • Ensure every test function is declared as public if enabled or private if disabled
  • Local Tests and CI PASS
    Using DssExecLib at: 0x8De6DDbCd5053d32292AAA0D2105A32d108484a6
    [] Compiling...
    [] Compiling 104 files with 0.8.16
    [] Solc 0.8.16 finished in 3.57s
    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 74.03s
    
    Running 20 tests for src/DssSpell.t.sol:DssSpellTest
    [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. 20 passed; 0 failed; finished in 1075.84s

Good to deploy :shipit:

KirillDogadin-std

This comment was marked as duplicate.

@SidestreamColdMelon
Copy link
Contributor Author

@SidestreamSweatyPumpkin
Copy link
Contributor

SidestreamSweatyPumpkin commented May 17, 2023

  • Deployed Spell is Verified
    • Optimization Enabled: No
    • Other Settings: default evmVersion, GNU AGPLv3 license
  • Deployed Spell Code matches GitHub
    • diffcheck etherscan source against spell PR (via make diff-deployed-spell)
      seems fine, etherscan has exec lib added to it
      also used diffchecker against previoius spell to ensure that the added library block is fine.
  • Deployed Spell Etherscan Checks
    • automated checks via make check-deployed-spell
      • verified
      • license type matches
      • solc version matches
      • optimizations are disabled
      • dss-exec-lib library address matches hardcoded local DssExecLib.address
      • deployed_spell_created matches deployment timestamp
        🔴 check fails for me, pls run the command and see if you have the same problem Edit: runs on linux (had problem with m1)
      • deployed_spell_block matches deployment block number
        🔴 check fails for me, pls run the command and see if you have the same problem Edit: runs on linux (had problem with m1)
    • manual checks
      • Ensure make deploy-info tx=<tx> matches config
        • deployed_spell_created timestamp
        • deployed_spell_block block number
      • Ensure Etherscan Libraries Used matches DssExecLib Latest Release
      • git submodule hash matches dss-exec-lib latest release's tag commit and inspect diffs if doesn't match to ensure expected behaviour
  • Archive matches src
    • make diff-archive-spell for current date or or date="YYYY-MM-DD" make diff-archive-spell (date as per target exec date)
    • ensure date corresponds to target exec date
  • Local Tests and CI PASS

Copy link
Contributor

@amusingaxl amusingaxl left a comment

Choose a reason for hiding this comment

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

Checklist

  • Deployed Spell is Verified
    • Optimization Enabled: No
    • Other Settings: default evmVersion, GNU AGPLv3 license
  • Deployed Spell Code matches GitHub
    • diffcheck etherscan source against spell PR (via make diff-deployed-spell)
  • Deployed Spell Etherscan Checks
    • automated checks via make check-deployed-spell
      • verified
      • license type matches
      • solc version matches
      • optimizations are disabled
      • dss-exec-lib library address matches hardcoded local DssExecLib.address
      • deployed_spell_created matches deployment timestamp
      • deployed_spell_block matches deployment block number
    • manual checks
      • Ensure make deploy-info tx=<tx> matches config
        • deployed_spell_created timestamp
        • deployed_spell_block block number
      • 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
  • Archive matches src
    • make diff-archive-spell for current date or or date="YYYY-MM-DD" make diff-archive-spell (date as per target exec date)
    • ensure date corresponds to target exec date
  • Local Tests and CI PASS
    Using DssExecLib at: 0x8De6DDbCd5053d32292AAA0D2105A32d108484a6
    [] Compiling...
    [] Compiling 104 files with 0.8.16
    [] Solc 0.8.16 finished in 3.67s
    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 86.54s
    
    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 1997.66s

Good to handover.

@LongForWisdom
Copy link

LongForWisdom commented May 17, 2023

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.

  • Ensure Etherscan Libraries Used matches DssExecLib Latest Release
  • 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
Copy link
Contributor Author

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
Copy link
Contributor

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
Copy link

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.

@SidestreamColdMelon SidestreamColdMelon merged commit b34e641 into master May 17, 2023
@amusingaxl amusingaxl deleted the 2023-05-17 branch May 18, 2023 16:18
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants