makerdao / spells-mainnet

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

Emergency 2023-06-16: Coinbase Custody DAO Resolution #347

Closed amusingaxl closed 1 year ago

amusingaxl commented 1 year ago

Description

Contribution Checklist

Checklist

The-Arbiter commented 1 year ago

There are almost 50 lines of unused interfaces from the old spell at the top of the spell file.

Please finish crafting the spell before marking it as ready for review.

The-Arbiter commented 1 year ago

Unused imports not removed Unused interfaces not removed Nit: An incorrect raw.githubusercontent.com URL is present in the exec hash comment instead of any TODO (though admittedly this may not be ready yet, we need to mark it TODO so we don't forget to change it) Test comment for the test that has always been commented as "don't disable" has been changed to "amke private to disable" [sic] <-- Ignoring the typo, this is not the time or place to be changing how we treat tests that have been marked clearly as "don't disable".

Focus on cleaning up the spell and then the new content for the emergency spell once the cleanup is finished.

As it stands, this spell is half-baked and not ready for review. The reviewers job is not to finish half of the crafting process; this shouldn't have been marked as ready for review until it had been looked over by the crafter at least once, which would've made evident things like half of the spell file being unused interfaces from the last spell.

The-Arbiter commented 1 year ago

Noting here that the deployed version of the spell does not match the latest commit.

Typically this would be a blocking change and require a redeploy however this appears to only be a missing line of provenance comment (the MIP). I will check more in-depth to make sure that the diff is only limited to this line, in which case this will be non-blocking (as the comment is non-functional and serves as supporting rather than primary provenance).

The-Arbiter commented 1 year ago

Spell is not archived yet.

Please archive while my tests run.