Skip to content
This repository has been archived by the owner on Apr 22, 2024. It is now read-only.

2023-08-18 Executive Spell #222

Merged
merged 20 commits into from
Aug 17, 2023
Merged

2023-08-18 Executive Spell #222

merged 20 commits into from
Aug 17, 2023

Conversation

SidestreamColdMelon
Copy link
Contributor

@SidestreamColdMelon SidestreamColdMelon commented Aug 10, 2023

Description

This PR implements executive spell planned for 2023-08-18.

Contribution Checklist

  • 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 Goerli changelog or known
  • Notify any external teams affected by the spell so they have the opportunity to review
  • Deploy spell to Goerli ETH_GAS_LIMIT="XXX" ETH_GAS_PRICE="YYY" make deploy
  • Ensure contract is verified on Goerli etherscan
  • Change test to use Goerli spell address and deploy timestamp
  • Cast spell on Goerli make spell="0x-deployed-spell-address" cast-spell
  • Run make archive-spell or make date="YYYY-MM-DD" archive-spell to make an archive directory and copy DssSpell.sol, DssSpell.t.sol and DssSpell.t.base.sol
  • squash and merge this PR

@SidestreamColdMelon SidestreamColdMelon self-assigned this Aug 10, 2023
@SidestreamColdMelon
Copy link
Contributor Author

Current state: main spell content is ready, except for the parts that will be supplied by the Spark:

  • "Transfer Spark Proxy Admin Controls" supplied via Add Spark admin change code #223
    • Needs to resolve conflicts
    • CLA needs to be signed
    • Have to be reviewed by a different team familiar with the spark protocol
  • "Trigger Spark Proxy Spell" prepared in [SC-42] August 16 spell marsfoundation/spark-spells#13
    • Including it now breaks the tests, because it depends on the new admin structure
    • Have to be reviewed by a different team familiar with the spark protocol

src/DssSpell.sol Outdated Show resolved Hide resolved
@0xdecr1pto
Copy link
Collaborator

Also DAO Resolution for BlockTower Andromeda is missing

@SidestreamColdMelon
Copy link
Contributor Author

Also DAO Resolution for BlockTower Andromeda is missing

It's there, right before the spark-related sections:

// ---------- DAO Resolution for BlockTower Andromeda ----------
// Forum: https://forum.makerdao.com/t/dao-resolution-to-facilitate-onboarding-of-taco-with-additional-third-parties/21572
// Forum: https://forum.makerdao.com/t/dao-resolution-to-facilitate-onboarding-of-taco-with-additional-third-parties/21572/2
// Include IPFS hash QmUNrCwKK2iK2ki5Spn97jrTCDKqFjDZWKk3wxQ2psgMP5 (not a `doc` update)

@0xdecr1pto
Copy link
Collaborator

Also DAO Resolution for BlockTower Andromeda is missing

It's there, right before the spark-related sections:

// ---------- DAO Resolution for BlockTower Andromeda ----------
// Forum: https://forum.makerdao.com/t/dao-resolution-to-facilitate-onboarding-of-taco-with-additional-third-parties/21572
// Forum: https://forum.makerdao.com/t/dao-resolution-to-facilitate-onboarding-of-taco-with-additional-third-parties/21572/2
// Include IPFS hash QmUNrCwKK2iK2ki5Spn97jrTCDKqFjDZWKk3wxQ2psgMP5 (not a `doc` update)

Does it need to be a var? Or it's not needed for goerli?

@SidestreamColdMelon
Copy link
Contributor Author

You're right! I've added the variable via 6938fb0

@SidestreamColdMelon SidestreamColdMelon marked this pull request as ready for review August 14, 2023 21:30
@0xdecr1pto
Copy link
Collaborator

0xdecr1pto commented Aug 15, 2023

Goerli Executive Spell Review Checklist

Coding Stage

  • Office Hours
    • Off (default, spell casted after deploying, pause delay 60 sec.)
  • Exec Hash
    • empty (default)
    • ensure description is Goerli Spell
  • 30 Days Expiry
  • lib (git submodule update --init --recursive to install/update)
    • dss-exec-lib
      • DssExecLib.address is unchanged by the spell PR (if changed, the new address must match the Latest Release Tag and be approved as a legitimate new release by GovAlpha)
      • if submodule upgrades are present make sure dss-exec-lib is synced as well
        not present
      • git submodule hash (run git submodule status) matches the latest release version or newer (NOTE: dss-exec-lib as installed locally will use GitHub code more recent than the 0.0.9 release)
        69b658f35d8618272cd139dfc18c5713caf6b96b lib/dss-exec-lib (v0.0.9-7-g69b658f)
        7529fa1e043b9195f41549852d9c5bb0714eaa27 lib/dss-test (heads/master)
    • dss-test
      • dss-interfaces
      • forge-std
        • git submodule hash matches version used by dss-test (Non-critical)
          does not seem to be the case but is not critical
          0aa99eb8456693c015350c5e6c4f442ebe912f77 lib/forge-std (v1.3.0-3-g0aa99eb)
  • dss-interfaces
    • used in the current spell
      not used but is ok since the static intrfaces are defined
    • cleanup previous ones
  • Static Interfaces
    • ensure they match dss-interfaces (Where there is a mismatch, use cast interface as the source of truth)
    • check on-chain interface of deployed contract via cast interface <contract_address> to ensure correctness
    • interface naming style should match with Like suffix (e.g. VatLike), with some exceptions
    • ensure they only list used functions in spell code
  • Rates match
    • Compare against IPFS
    • Check manually via make rates pct=<pct> (e.g. pct=0.75, for 0.75%)
      • 3.58 -> 1000000001115362602336059074
      • 3.33 -> 1000000001038735548426731741
      • 4.08 -> 1000000001268063427242299977
      • 5.00 -> 1000000001547125957863212448
      • 5.25 -> 1000000001622535724756171269
      • 5.55 -> 1000000001712791360746325100
      • 5.80 -> 1000000001787808646832390371
      • 6.30 -> 1000000001937312893803622469
      • 7.00 -> 1000000002145441671308778766
    • Variable name conforms to X_PT_Y_Z_PCT_RATE (e.g. ZERO_PT_SEVEN_FIVE_PCT_RATE for 0.75%)
    • Variable visibility declared as internal
    • State mutability declared as constant
  • Math matches
    • Internal Precision
      • RAD = 10 ** 45
    • Units
      • THOUSAND = 10 ** 3
      • MILLION = 10 ** 6
      • BILLION = 10 ** 9
      • Variable visibility declared as internal
      • State mutability declared as constant
      • Ensure they match with config
  • String Constants
    • IPFS hashes or DAO resolutions
    • Must be TOP LEVEL (or ds pause will reject)
    • Exec action comments are above it
    • Name matches purpose per EXEC DOC
    • Visibility is public
    • State mutability MUST BE constant
  • Stability Fee jug.ilk.duty (setIlkStabilityFee)
    • ETH-A -> 3.58% // Increase ETH-A SF by 0.14% from 3.44% to 3.58%
    • ETH-B -> 4.08% // Increase ETH-B SF by 0.14% from 3.94%% to 4.08%
    • ETH-C -> 3.33% // Increase ETH-C SF by 0.14% from 3.19% to 3.33%
    • WSTETH-A -> 5.25% // Increase WSTETH-A SF by 1.81% from 3.44% to 5.25%
    • WSTETH-B -> 5.00% // Increase WSTETH-B SF by 1.81% from 3.19% to 5.00%
    • RETH-A -> 5.25% // Increase RETH-A SF by 1.81% from 3.44% to 5.25%
    • WBTC-A -> 5.80% // Increase WBTC-A SF by 0.11% from 5.69% to 5.80%
    • WBTC-B -> 6.30% // Increase WBTC-B SF by 0.11% from 6.19% to 6.30%
    • WBTC-C -> 5.55$ // Increase WBTC-C SF by 0.11% from 5.44% to 5.55%
  • Dai Savings Rate pot.dsr (setDSR)
    • Double check that PCT_RATE is correct
      • Check manually via make rates pct=<pct> (e.g. pct=0.75, for 0.75%)
        • 5.00 -> 1000000001547125957863212448
      • Compare against IPFS
  • Debt Ceiling Changes
    • Autoline Changes
      • setIlkAutoLineDebtCeiling
        • WSTETH-A -> 750 * MILLION // Increase WSTETH-A line by 250 million DAI from 500 million DAI to 750 million DAI (no change to gap or ttl)
        • RETH-A -> 75 * MILLION // Increase RETH-A line by 25 million DAI from 50 million DAI to 75 million DAI
      • setIlkAutoLineParameters
        • ilk: STETH-B
        • line: 1 * BILLION // Increase WSTETH-B line by 500 million DAi from 500 million DAI to 1 billion DAI
        • gap: 45 * MILLION // Increase WSTETH-B gap by 15 million DAI from 30 million DAI to 45 million DAI
        • ttl: 12 hours // Reduce WSTETH-B ttl by 14,400 seconds from 57,600 seconds to 43,200 seconds
  • Smart Burn Engine Parameter
    • VOW.bump -> 20_000 DAI (RAD precision used) // Increase vow.bump by 15,000 DAI from 5,000 DAI to 20,000 DAI
    • FLAP.hop -> 6_308// Increase hop by 4,731 seconds from 1,577 seconds to 6,308 seconds
  • RWA Updates (RWA002-A)
    • Stability Fee jug.ilk.duty // Increase RWA002-A Stability Fee by 3.5% from 3.5% to 7%
    • Liquidation Ratio spotter.ilk.mat // Reduce Liquidation Ratio by 5% from 105% to 100%
    • Debt Ceiling (line) + Liquidation Oracle Price Bump (val)
      • Increase Ilk Debt Ceiling (set DC + increase Global DC) // Increase RWA002-A Debt Ceiling by 30 million DAI from 20 million DAI to 50 million DAI
      • 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 (see below)
          • New val is calculated with line * [(1 + duty) ** years] * mat - rounded up - and makes sense in context of the rate mechanism. Minimum duration is usually in the Exec Doc of the spell with the RWAXXX ilk onboarding.
          • Comment explaining val formula (Debt ceiling * [ (1 + RWA stability fee ) ^ (minimum deal duration in years) ] * liquidation ratio) is present
          • Accompanying comment above bump line in format // XXM * 1.XX^X * X.XX as a WAD corresponding to the val calculation formula (e.g. // 15M * 1.03^2 * 1.00 as a WAD) is present along with the calculation formula on the line above
          • IF combining val of multiple RWA ilks being combined, val calculation is done once per ilk and added to make the total, with workings provided in code comments. The existing val value can be retrieved by calling read() on PIP_RWAXX and converting the result into decimal.

          We use only one ilk

      • Poke spotter to pull in the new price
  • SubDAO Content
    • SubDAO SubProxy spell execution
      • SubDAO spell address matches Exec Sheet (comment or otherwise)
      • SubDAO spell deployer is purely an EOA
        • Ensure the deployer address is in addresses_deployers.sol as a comment
      • Executed using ProxyLike(SUBDAO_PROXY).exec(SUBDAO_SPELL, abi.encodeWithSignature("execute()"));
      • Execution is NOT delegate call
      • Reviewer Note: Gas cost may be very high as SubDAO spells execute within the main cast execution. (Also note that low level call gas estimation is not done by our scripts)
    • Maker Core (main spell) SubDAO actions (i.e. operate in Pause Proxy DelegateCall context)
      • No SubDAO contract being interacted with is authed on a core contract like vat, etc. (Check comprehensively where the risk is high)
      • SubDAO contract licensing and optimizations generally do not matter (except where they pose a security risk)
      • SubDAO contracts and all libraries / dependencies are verified (Blocking if not true)
      • All SubDAO content addresses (i.e. provided contract addresses or EOAs) present in the Maker Core spell are present in the Exec Sheet and are correct. SubDAO addresses being authed or given any permissions and addresses being called must be confirmed by the SubDAO spell team.

        This is not true, only Spark spell in exec

      • SubDAO actions match Exec Sheet (only where inline with main spell code) and do not affect core contracts
      • External calls for SubDAO content are NOT delegate call
      • Code does not have untoward behavior within the scope of Maker Core Contracts (e.g. up to the SubDAO proxy)
  • ChainLog (OK, without changes)
  • addresses_goerli.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
  • Fetch addresses as type address and wrap with Like suffix interfaces inline (when making calls) unless archive patterns permit otherwise (Such as MKR)
  • Use the DssExecLib Core Address Helpers 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 (e.g. MKR)
  • Spell actions match GovAlpha Spell Content Sheet
  • Local Tests PASS
  • Ensure Good Coverage
  • Ensure every test function is declared as public if enabled or private if disabled

hexonaut
hexonaut previously approved these changes Aug 15, 2023
Copy link
Contributor

@hexonaut hexonaut left a comment

Choose a reason for hiding this comment

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

Good to deploy once Spark Spell is finished review.

Checked:

  • Office Hours off (Always in Goerli)
  • IPFS Hash matches
  • New rate additions to rates.sol are correct
  • Rate constants match rates.sol
  • Address constants/immutables are all correct
  • Setting DSR is correct
  • Stability fee changes are correct
  • SBE Updates are correct
  • Non-DSR Parameter Changes are correct
  • Ignoring Curve stETH/ETH offboarding on Goerli
  • Ignoring delegate comp on Goerli
  • Ignoring Aave/Compound D3M Changes on Goerli
  • RWA002 Changes are correct
  • Spark Admin Transfer Reviewed by others [Skipping]
  • Spark Spell execution line is correct
  • Test config values have been updated correctly
  • Tests configured correctly with proper ones enabled
  • Tests passing

@0xdecr1pto
Copy link
Collaborator

Good to deploy

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

Running 20 tests for src/DssSpell.t.sol:DssSpellTest
[PASS] testAuth() (gas: 52110847)
[PASS] testAuthInSources() (gas: 8666152)
[PASS] testBytecodeMatches() (gas: 2877821)
[PASS] testCastCost() (gas: 1500021)
[PASS] testChainlogValues() (gas: 9850386)
[PASS] testChainlogVersionBump() (gas: 4914295)
[PASS] testContractSize() (gas: 8984)
[PASS] testDeployCost() (gas: 2862049)
[PASS] testFailNotScheduled() (gas: 14397)
[PASS] testFailTooEarly() (gas: 13607)
[PASS] testFailTooLate() (gas: 13606)
[PASS] testFailWrongDay() (gas: 13607)
[PASS] testGeneral() (gas: 39210906)
[PASS] testNextCastTime() (gas: 364790)
[PASS] testOnTime() (gas: 1495660)
[PASS] testPSMs() (gas: 2693741)
[PASS] testSparkAdminTransfer() (gas: 1536201)
[PASS] testSparkSpellIsExecuted() (gas: 1499197)
[PASS] testUseEta() (gas: 363541)
[PASS] test_RWA002_Update() (gas: 1875290)
Test result: ok. 20 passed; 0 failed; finished in 894.93s

@SidestreamColdMelon
Copy link
Contributor Author

The spell is deployed at https://goerli.etherscan.io/address/0x341281316C53a6c9b099581C9f87665FA5815090#code
I've also created tenderly simulation of cast()ing this spell (as we will not be able to actually cast it before EDSR voting passes) https://dashboard.tenderly.co/shared/fork/simulation/63f1a828-15d5-45d3-b14a-2090ef1c3403

@0xdecr1pto
Copy link
Collaborator

0xdecr1pto commented Aug 15, 2023

Deployed Stage

  • Deployed Spell is Verified
    0x341281316C53a6c9b099581C9f87665FA5815090 OK
    • Optimization Enabled: No
      No with 200 runs OK
    • Other Settings: default evmVersion, GNU AGPLv3 license
      OK
  • Deployed Spell Code matches GitHub
    Diff is fine (execlib inlined to deployed spell)
    • diffcheck etherscan source against spell PR (via make diff-deployed-spell)
      Diff OK, just interfaces and execlib
  • Deployed Spell Etherscan Checks
    Doing manually
    • automated checks via make check-deployed-spell
      Doing manually
      • verified
      • license type matches
        AGPLv3 OK
      • solc version matches
        0.8.16 OK
      • optimizations are disabled
        No with 200 Runs OK
      • dss-exec-lib library address used (under 'Libraries Used') matches the hardcoded local DssExecLib.address file
        0x122f6c0dcd898b4a07310e92c3eae5d7ce0c8bb6 OK
        • Check again that the PR did not modify the DssExecLib.sol file (e.g. look under the 'Files Changed' PR tab, etc.)
          OK
      • deployed_spell_created matches deployment timestamp
        1692106800 Matches OK
      • deployed_spell_block matches deployment block number
        9522321 Matches OK
    • manual checks
      • Ensure make deploy-info tx=<tx> matches config
        • deployed_spell_created timestamp
          1692106800 OK
        • deployed_spell_block block number
          9522321 OK
      • Ensure Etherscan Libraries Used matches DssExecLib Latest Release
      • (For your tests to be accurate) git submodule hash matches dss-exec-lib latest release's tag commit and inspect diffs if doesn't match to ensure expected behaviour (Currently Non-Critical pending the next DssExecLib release, double check that exec lib used by the contract matches the latest release)
  • Archive matches src
    • make diff-archive-spell for current date or or date="YYYY-MM-DD" make diff-archive-spell (date as per cast timestamp)
    • ensure date corresponds to target exec date
  • CI tests are passing
  • Local Tests PASS

Waiting for local tests and CI

@0xdecr1pto
Copy link
Collaborator

Good to cast

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

Running 20 tests for src/DssSpell.t.sol:DssSpellTest
[PASS] testAuth() (gas: 52110847)
[PASS] testAuthInSources() (gas: 8666152)
[PASS] testBytecodeMatches() (gas: 2877821)
[PASS] testCastCost() (gas: 1500021)
[PASS] testChainlogValues() (gas: 9850386)
[PASS] testChainlogVersionBump() (gas: 4914295)
[PASS] testContractSize() (gas: 8984)
[PASS] testDeployCost() (gas: 2862049)
[PASS] testFailNotScheduled() (gas: 14397)
[PASS] testFailTooEarly() (gas: 13607)
[PASS] testFailTooLate() (gas: 13606)
[PASS] testFailWrongDay() (gas: 13607)
[PASS] testGeneral() (gas: 39213003)
[PASS] testNextCastTime() (gas: 364790)
[PASS] testOnTime() (gas: 1495660)
[PASS] testPSMs() (gas: 2693741)
[PASS] testSparkAdminTransfer() (gas: 1536201)
[PASS] testSparkSpellIsExecuted() (gas: 1499197)
[PASS] testUseEta() (gas: 363541)
[PASS] test_RWA002_Update() (gas: 1875290)
Test result: ok. 20 passed; 0 failed; finished in 900.81s

Copy link
Contributor

@hexonaut hexonaut left a comment

Choose a reason for hiding this comment

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

Good to cast / merge. Checked:

  • Tests passing
  • Code matches repo
  • Block and timestamp are good
  • DssExecLib matches repo address
  • Archive matches

@SidestreamColdMelon
Copy link
Contributor Author

@SidestreamColdMelon SidestreamColdMelon merged commit e05a44e into master Aug 17, 2023
3 checks passed
@SidestreamColdMelon SidestreamColdMelon deleted the 2023-08-18 branch August 17, 2023 17:38
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants