Dewiz x Sidestream x Arby Spell Improvements

[The-Arbiter]

Hi,

Changes as required from my 2023-05-10 Goerli review.

More to come as necessary.

[ghost]

For complete clarity this is the GovAlpha perspective:

• ES is a core component of the game theory in the MCD design used by the Maker Protocol (see: whitepaper).
• Failure to appropriately authorise the ESM on new contracts can lead to a failure mode in the event ES is called, with potential significant downsides for DAI holders as a result.
• We have recently paid out big bug bounties as a result of the identification of ES issues.
• It is, therefore, in my opinion, fine from a governance perspective to assume that the ESM should be authed for new contracts unless explicitly stated otherwise.

That said, things may change if, at some point, ES is removed from being a core component of the Protocol, and this should be reviewed if and/or when that happens.

[SidestreamColdMelon]

A suggestion from LongForWisdom | GovAlpha from the #new-spells discord channel: move Fill Spell Crafter Related Boxes in the Main Exec Doc Sheet point to much earlier in the process.

[LongForWisdom]

So, there are three sets of things to confirm on the sheet.

1. (Contents sheet) 'Spellcrafter Confirm Content is feasible' - Ideally, this should be filled in shortly after GovAlpha creates the content sheet for the spell. Likely on Wednesday. Confirmation here indicates that the crafter understands and accepts the desired contents for the spell.
2. (Contents sheet) 'Spellcrafter confirm content in spell' - Ideally, this should be filled in after the mainnet spell is deployed? I guess? Open to feedback on when this should happen.
3. (Main sheet) The rows in green. We should discuss what should be included in the main sheet related to technical spell work. I suspect it's largely useless to you guys. For us it could be a good way to see what stage the spell is at without bothering you.

The spell sheet itself has not been brought up to date recently. There are a number of rows that are no longer relevant, and the 'When?' column was never updated for the new schedule. We should discuss, and then GovAlpha should do an update pass on it.

[The-Arbiter]

I agree. Let's sync up on this relatively soon so that we can balance overhead / effort expended with transparency. IMO we used to be a little slack on this due to the nature of content coming in late-ish or perhaps just due to the cadence.

Having one "official spell comms channel" might be useful when it comes to these kinds of things too (re: you guys knowing where spells are at). Spell crafters determining feasibility of the content is a default yes (IMO) unless the content comes in late, since practically all types of content can be done, it's simply a matter of what gets done when. Pushback on here would rarely happen via sheet, I would assume. So you know, typically (obviously this will likely change with the introduction of subDAOs) all spell content is completed concurrently, with reviews (ideally) covering all included content at once in order to minimize the risk of something sneaking in through a later commit.

Happy to explore all the options available here with you and other spell participants at some point in the future; if you have any feedback or recommendations they are always appreciated!

[The-Arbiter]

Considering enforcing that 1.00 rate (for SF) is called ONE_PT_ZERO_ZERO_PCT_RATE (i.e. we explicitly name trailing zeroes) given the move away from X.X5 and X.X0 type rates (divisible by 5 when in basis points) such as 3.49% this week (in order to keep names consistent and a predictable length and also to allow for simpler automation of spell checking).

Open to feedback on this

[The-Arbiter]

Let's merge this after this spell ships so we can be done with this PR and the content is on master :)

[SidestreamColdMelon]

Few comments are still pending (sadly, I can't collapse resolved comments, as I don't have access to this repo):

• Goerli payments https://github.com/makerdao/pe-checklists/pull/12#discussion_r1276295250
• SubDAO link to PR https://github.com/makerdao/pe-checklists/pull/12#discussion_r1276324433
• SubDAO wording https://github.com/makerdao/pe-checklists/pull/12#discussion_r1276316897
• SubDAO responsibility https://github.com/makerdao/pe-checklists/pull/12#discussion_r1276316967

[The-Arbiter]

I renamed Risk Parameter Changes to Core System Parameter Changes in order to better reflect the nature of the checks performed within this section:

• DSR changes are not explicitly 'risk' associated changes (though this could be argued) and are more 'reward' oriented
• Other auction parameters such as starting price factor (buf) are also only somewhat 'risk' in my opinion.

A better name was 'Risk and Reward Parameter Changes' but the name 'Core System Parameter Changes' better reflects these items in this section as there are risk parameter changes (RWA, etc.) which occur outside of this section.

An alternative name might be 'Core System Risk Parameter Changes' but this is too wordy for my liking and System Parameter Changes was too general, so I went with Core System Parameter Changes.

Anyone is free to provide feedback here or veto this name change if they feel it is inappropriate.

Noting that TypoCheck is failing but I will fix it before this PR gets merged.

[The-Arbiter]

I removed the following lines:

* [ ] CI tests are passing
* [ ] Local Tests PASS

_Insert your passing local tests here_

Under the Cast Stage section for the following reasons:

• This is never complied with because local tests should almost always fail after a spell is cast. As such, these items were unused in the first place.
• This check originated from repeatedly adding 'tests are passing' style checks after various stages in the checklist. Lack of sections / headings within the checklist probably caused confusion and led to this being here.

Anyone is free to veto this change as it is a functionality related change and removes the need to run tests.

[The-Arbiter]

I made a bunch of changes with the above:

• Full rework of the Payments section in order to be easier to use, with more explicit instructions and removing extraneous information.

These changes formalize the MONTH_DD_YYYY - MONTH_DD_YYYY structure for cliff duration expression (we used to do 365 days but this isn't ALWAYS equal to a year, so this is technically more accurate).

I wrote 'Cliff Date' as clf since it doesn't HAVE to be a date (can just be a timestamp) and I don't want to let exec doc wording / formatting patterns hurt our ability to adapt (i.e. if we want to have a cliff 4 hours into a specific day or something due to timezone requirements or something).

These changes extend mgr being zero on DAI streams (as the default) to the default for both MKR streams and DAI streams, which doesn't impact much since the exec doc overrides that item anyway.

I also fixed some wording with 'Exec Doc' and 'Exec Sheet' and did a bunch of cleanup across the sheets.

• Overhaul of 'Tests', which is now a section within the checklist

The motivation for this is that these items have been visually (in the checklist) small and semi-ambiguous with what they want from you as a reviewer. As such, a new structure is present which makes checking that CI tests are passing a useful check (via noting the commit hash of the passing CI).

This rework also puts an emphasis on coverage, which has presented an issue in some previous spells. This format is how I personally work with that checklist item (Ensure coverage --> Ensure each spell action has sufficient test coverage) and makes it so that this checklist item is actually doing its job and ensuring coverage.

Running tests locally has also been reworked slightly, whereby the checklist now states that the function to run is make test (note: not the script directly) and by adding an env check which ensures that match and block env isn't being pulled in, which can lead to passing tests when they would fail without these vars (or not running all tests).

This has been done in a way which minimally impacts reviewer workflow (i.e. does not mandate that you clear all your env or something) and still relies on trusting the reviewer (for now).

• Typos and Cleanup

Various typos (such as DssVest as the name of the interface which should have been DssVestLike) were fixed and a lot of small wording things were changed.

Useless wording such as check spell is cast being 'only on Goerli' has been removed (since that check is not present on Mainnet either way).

There are functional implications of these changes, however, though these are all intended and fix discrepancies in the checklists. For example, there used to be reference to instructions from the Exec Sheet in the Mainnet reviewer checklist, which should not have been present.

I removed reference to the old PE-XXXX ticketing system naming of PRs, though the crafter checklists are both far from perfect.

• Handover Stage and Associated Post-Archival Actions

Handover actions were previously documented in the crafter checklist only. These changes extend the interactions required in new-spells (i.e. posting the spell address as a crafter and confirming it as a reviewer) to both checklists, meaning that this behaviour is standardized.

The section 'Handover' refers to the Mainnet equivalent of 'Cast Stage' on Goerli, however this was undocumented until now. These changes add the section and some basic checks.

Post-archival actions (specifically, approving the PR so that it can be merged) have also presented themselves as friction points for us - probably since they weren't documented in the checklists - so they are now documented and standardized.

• Local CI and Tests Pass (After Cast) Removal

I removed the unnecessary tests runs here, I think I covered this in another comment.

• Single Point Of Failure Reduction

It was previously possible for a crafter or reviewer to commit to the PR after archival with potentially malicious code, which would not be present in the archives and would not be caught by tests (since they aren't run after the 'Deploy and Archive Stage' test run). This is now checked as an item, which acts to mitigate this kind of attack.

Additionally, checks that the spell address being handed over is actually correct were added. This really takes ten seconds to do, so this is just formalizing it and keeping the spell team engaged until they approve the PR and it can be merged.

• SubDAO deployer address treatment

As mentioned in another comment thread, these addresses affect test execution duration and therefore have been treated differently to prevent test execution time bloat.

Overall, this is a small revamp of a few sections, with most changes being in 'Payments'.

[The-Arbiter]

There are still TODO items here, ignore them for now, this is still a WIP

[The-Arbiter]

Made more changes:

• Added 'Per Exec Doc / Sheet' for Mainnet/Goerli respectively as part of 'Spell Actions' for clarity
• Added instruction for spell actions section to ensure compliance
• Renamed coding stage to development stage to accurately reflect what is being done (not always 'coding'
• Made office hours options more clear and removed decision making context (as spell team can no longer decide this, it is input instructions from the exec doc)
• Cleaned up initial structural checks at top of checklist (spell description etc)
• Renamed 'Exec Hash' section to 'Spell Description' for better accuracy
• Renamed 'Exec Hash' to 'Exec Doc Hash' for clarity
• Formalized commit anchor generation method
• Enforced a spell naming structure of TARGET_DATE MakerDAO Executive Spell | Hash: EXEC_DOC_HASH)
• Fixed make exec-hash instructions, which didn't work
• Reworded some items to be binary / boolean checklist items (i.e. checking the box means "Yes, this is true")
• Moved '30 Days expiry'
• More reformatting of Exec Doc section We can potentially remove wget and use curl instead for the description accompanying comment if we want to, but I won't do that as that changes the spell code (comment) so this needs to be OK'ed by spell participants before it changes. Just noting it here.
• Added sections to add raw URL and Exec Doc Hash when generated to make copy-pasting them less cumbersome and to record what was used when and by whom
• Reformatted how we check exec doc hash (formalize commit anchor fetching method, re-list the steps a reviewer does when checking this and making them checklist items, etc.)
• Added checklist heading item Developer Environment Checks (which will be refined shortly)

[The-Arbiter]

Made more changes:

• Added stuff to Dev Environment Checks / Actions (see below) --> Renamed to Local Environment Actions as this is more accurate
• Mandatory FoundryUp
• Mandatory reinstall libraries (delete lib and re-install) This will prevent clashing interface issues as we have had in the past and make sure that reviewers are only using necessary libraries, while also ensuring that they are up-to-date. This may pose a supply chain attack risk if Foundry becomes unmaintained in the future.
• Added submodule path copy-paste area so we can detect env mismatches (transparently) and origination of env changes (when we look back through review and spell PR history)

Noting here that this env check stuff is not just 'work for the sake of work' and that it (when done properly) provides invaluable information about what libraries and what versions of dependencies were used for a given spell. It should be noted that this information is not stored in the archives at present (it is only local env setup) and therefore this new structure does a few things:

• Adds transparency (we know who is running what version of which library and when they are doing it)
• Keeps a historical log of dependencies used Ssome items were previously worded with phrases like "if submodule upgrades are present..." which implicitly refer to the fact that (under the old operating paradigm) spell team members knew what the commit hashes for dependencies like Exec Lib looked like in the past and could detect 'upgrades' by looking at the hash. This "copy-paste" checklist comment (under "Install libraries using git submodule update --init --recursive" now functions to historically log which libraries were used by whom and when. This makes detecting submodule upgrades much more feasible for new spell team members or for those who have not done a spell in a while. It also allows for easier debugging (i.e. we can answer the question "did the libraries get updated or something?"). Note that I haven't done this for Foundry version (specifically the cast and forge versions) yet since I don't want this to be a painful thing to comply with; the current process is a copy-paste into a pre-made box that explicitly tells you what it wants
• Flags supply-chain attacks and mismatches which were overlooked (due to trivial security concerns, etc).

IMO these changes and requirements add a total of 30-60 seconds to the checklist completion process (for a reviewer, I tried it myself and that's how long it took) and act to drastically reduce the likelihood or severity the following issues:

• Mismatching Env between reviewers leading to inconsistency in test results or script outputs
• Outdated foundry version leading between reviewers leading to inconsistency in test results or script outputs
• Unknown library version usage of past reviewers leading to more effort diagnosing issues in the future ("are your libraries correct?" - this happened quite a bit under PE)

This can eventually be scripted such that it outputs the foundry version outputs and submodule hashes to one text block (which can be copy-pasted into the review) or (even better) into a text block which somehow gets propagated to the archives (as a comment) so it can be searched more easily. That is a long way away, however.

[The-Arbiter]

Ideas (Need feedback)

Remove precision unit redundancy check * [ ] Ensure they match with [ds-math](https://github.com/dapphub/ds-math/blob/master/src/math.sol) and the [Numerical Ranges](https://github.com/makerdao/dss/wiki/Numerical-Ranges#notation). I do this sometimes but TBH this is for redundancy and the sources being used are no safer than the checklist itself. I think we can (99%) safely remove this one.

Remove math unit redundancy check * [ ] Ensure they match with [config](https://github.com/makerdao/spells-mainnet/blob/master/src/test/config.sol). As with the precision unit check, this is semi-useful as redundancy, but if config.sol has a diff where units change, we would find it under test coverage or some other way. This is multiple times more useful than the precision unit check, but I still question the utility here vs the time it takes as an action.

Change ZERO_PT_SEVEN_PCT_RATE format to ZERO_PT_SEVEN_ZERO_PCT_RATE (a bit tedious but they'll all be the same length). I kind of don't want to do this tbh but it would make them permanently the same, machine-processable and unable to be misinterpreted by different spell team members, so I'm entertaining it.

I am keeping rates check with IPFS and timestamp Epoch conversion check using this website in there but this tool is not mandatory to use as otherwise we can get supply chain attacked by it. We can probably eventually script it when this gets automated.

Keen to hear thoughts on the above.

[The-Arbiter]

More changes (Working on Goerli checklist for now, will diff and replicate later):

• Updated submodule checks to add a few and mark some as 'non-critical' (i.e. might be broken right now, are not presently a security concern, but are necessary in the checklist for security reasons or otherwise)
• Formalized no 'ZERO' in PCT_RATE naming if Y,Z==0 (i.e. SIX_PCT_RATE for 6.00%) (for now...) -> Open to objections / feedback here
• Wording changes in constants section
• Simplified library reinstall method via rm -r lib && git submodule update --init --recursive (deletes them for you) -> I didn't do 'foundryup' here too as I don't want to be too invasive for people working with spells. Eventually this can be a script if we want it to be one. The only other footgun protection I want here is a check that src/DssSpell.sol is present or something, since I don't want this deleting people's stuff on their machines if they were in the wrong folder. Anyway, that is future work...
• Marked config.sol Math Constant checks as non-critical (they'll be caught in test coverage checks OR a PR diff check normally)
• Marked ds-math Precision Constant checks as non-critical (it was a check for redundancy with barely any usage in the real world tbh) -> It can be removed once the spell team comes to a consensus on it if we think there are no significant security risks associated with its removal.
• Added formal address declaration rules (for constants AND immutables) which shift review workload earlier on in the checklist (from payments for example, it moves to the constant checking section) -> Good for reviewer mental workflow IMO, open to objections here
• Added formal string declaration rules (for IPFS hashes and the like) based on archive patterns
• Renamed 'dependency checks' to 'library checks'
• Elaborated on chainlog update increment item wording
• Added no unused variable rule (because I removed a similar rule about 'no unused addresses') which will be reworked in a future commit (I think...)

Please ignore any 'TODO' or similar in the checklist atm as it is a WIP. These will be cleaned up at the end.

Proposals:

• Move Rate constants below Math but above addresses?
• Echo time, some hash, env, etc and env before test runs. This will add to the copy-paste size but not to test run time and will provide transparent info about test execution environments in spell review comments with no overhead for reviewers.

[The-Arbiter]

Surprisingly we didn't have any stuff for line or Line, so I added some scaffolding there.

Indenting for the comment blocks was fixed by committing the suggestion.

[The-Arbiter]

Issue raised by AmusingAxl:

The following mainnet checklist items fail because Governance Facilitators made unrelated commits to the repo:

  * [ ] Search the ['Community' GitHub repo](https://github.com/makerdao/community/tree/master/governance/votes) for the corresponding Exec Doc
  * [ ] Click ['History'](https://github.com/makerdao/community/commits/master/governance/votes/) for the corresponding Exec Doc

Commenting here in order to preserve this issue as open for future work on this PR.

Thanks to AmusingAxl for raising this

[Le-Bateleur-2023]

The updates to dev checklist (moving Technical Point confirmations for Exec Vote items up earlier in the process) were requested and agreed to at 12/09/2023 GovOps meeting on Discord. Governance approves these changes.