makerdao / spells-mainnet

Staging repo for MakerDAO weekly executive spells

GNU Affero General Public License v3.0

108 stars 45 forks source link

Test wasn't actually checking the POT dsr value + adding a missing er… #177

Closed gbalabasquer closed 3 years ago

gbalabasquer commented 3 years ago

…ror message

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
[ ] Verify expiration (4 days monthly and 30 days for the rest)
[ ] 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
[ ] Keep DssSpell.sol and DssSpell.t.sol the same, but make a copy in archive
[ ] squash and merge this PR

gbalabasquer commented 3 years ago

LGTM, @gbalabasquer how did we discover this?

Actually by pure casualty when doing the goerli spell. I had an issue in the deployed dsr but the test was failing due the rates file not being updated and not actually because the real value was wrong. So basically if the rates file wouldn't have had the error then I wouldn't have caught the missing test nor the real value error.

kmbarry1 commented 3 years ago

LGTM, @gbalabasquer how did we discover this?

Actually by pure casualty when doing the goerli spell. I had an issue in the deployed dsr but the test was failing due the rates file not being updated and not actually because the real value was wrong. So basically if the rates file wouldn't have had the error then I wouldn't have caught the missing test nor the real value error.

:grimacing:

This is a great example of how it's important to test tests...usually I do this informally by after writing a test, seeing if I can make it fail by making the thing it's supposed to check wrong. But for something as important as executives, we might consider a more automated solution. E.g. some continuously-running fuzzing job that tries different single-change spells and confirming that any change makes the tests fail. It might also be something to start doing in code reviews on really high-stakes code. I.e. don't just read and run the tests...confirm that you can make them fail.