makerdao / spells-mainnet

Staging repo for MakerDAO weekly executive spells

GNU Affero General Public License v3.0

106 stars 45 forks source link

PE-989 Executive Spell #246

Closed gbalabasquer closed 2 years ago

gbalabasquer commented 2 years ago

Description

Contribution Checklist

[ ] PR title starts with (PE-<TICKET_NUMBER>)
[ ] 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="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

iamchrissmith commented 2 years ago

can we remove the substance of these old vest tests: https://github.com/makerdao/spells-mainnet/pull/246/files#diff-cd597c47b9a6e2bae57aab767ee9e3d5bb3d8edeb3a2248b4519b9af791cb468R422

gbalabasquer commented 2 years ago

1 small comment from my side: We should probably comment out the content of the collateral onboarding function (as we usually do).

Ah yes I forgot to do it for mainnet spell, good catch. Will change it now. This is btw why I don't like much that separation. It is easy to forget what happens in that file.

gbalabasquer commented 2 years ago

can we remove the substance of these old vest tests: https://github.com/makerdao/spells-mainnet/pull/246/files#diff-cd597c47b9a6e2bae57aab767ee9e3d5bb3d8edeb3a2248b4519b9af791cb468R422

Not really sure if this link works as expected but I think I understood what you meant and made the changes.

gbalabasquer commented 2 years ago

Have we confirmed with GovAlpha that Office Hours is ON for this spell?

Well in theory is them confirming with us. But yeah it is not in the doc summary, so just pinged LFW. Good catch!

talbaneth commented 2 years ago

Spell code matches deployed spell. Tests pass, good to publish from my side.

iamchrissmith commented 2 years ago

Checked post deployment and

[X] Tests pass
[x] Deployed Spell Verified
- [x] Optimization Enabled: No
- [x] Other Settings: default evmVersion, GNU AGPLv3 license
[x] Deployed Spell Code Matches GitHub
[ ] Archive Ok

let me know when archive is done and i can provide another review, but should be good to publish with deployed spell

gbalabasquer commented 2 years ago

@talbaneth @iamchrissmith archive done!