Add priority emergency spells

[amusingaxl]

https://docs.google.com/document/d/1zYgX_YQTDnjl1vO7EYeryicVtEzardikqE1t3tLyo74

[SidestreamColdMelon]

Looking at the chief-keeper block-scheme, I understand that it will try to call spell.schedule() in the loop, until "it is scheduled". On the technical level, "scheduled" is checked via spell.eta != 0. In case we don't modify eta, the keeper will call emergency spell in the loop in every single block and the whole interface "compliancy" wouldn't make much sense.

To solve this loop, we can either:

• A. Always use factories and require deployment, even for multi-variations. Have fully compliment eta logic, expiration and one-time usage logic. I hardly can see when emergency spell with the same hat will need to be executed more than once
• B. Set eta inside spell.schedule to non zero
  • Additional complexity: Setting eta will prevent spell from being directly reusable. We can potentially circumvent this by modifying it back to 0 either via cast or manually/permissionlessly via some other, e.g. reset() method
• C. Modify keeper logic to determine that spell was already scheduled
  • Additional complexity: there no cross-compatible event or state variable that can be used for checking if "a spell" was scheduled. For example, if plot would've been called via emergency spell, keeper can check pause.plans[hash] != true instead of spell.eta != 0

All in all, I would recommend to make some changes to the base contract to prevent keeper loop.

[amusingaxl]

Looking at that line we have:
if spell.done() == False

Notice that all emergency spells have meaningful done() implementations, meaning that right after schedule() is called, it should return true, which would prevent the loop in the keeper.

[amusingaxl]

I hardly can see when emergency spell with the same hat will need to be executed more than once

• Governance might be required to disable a specific DDM more than once during its lifespan
• Oracle malfunctioning might happen more than once during its lifespan
• Temporarily disabling new collateral intake through auto-line might happen more than once for the same ilk

[amusingaxl]

A. Always use factories and require deployment, even for multi-variations. Have fully compliment eta logic, expiration and one-time usage logic. I hardly can see when emergency spell with the same hat will need to be executed more than once

The idea of having pre-deployed spells is to not require any non-engineer to ever provide inputs manually. Even in the cases where there are factories, they would be used by engineers to deploy the contracts and hand over only the addresses to other stakeholders.

[SidestreamColdMelon]

Nit: if we don't need nor expect cast to be triggered, would it make more sense to return type(uint256).max? It shouldn't make a difference in practice, since it will never be planned into pause contract (unless we change this logic based on the previous comment)

[amusingaxl]

I didn't take a deep dive into the chief keeper, just looked at the most important aspects of it.
If any cleanup logic depends on the spell cast() being called, we don't want to clean it.
However, looking again, since cast() is permissionless, it's not guaranteed that the keeper is the one to call it every time, so it would make little sense to have such dependency, so your suggestion makes sense.

[SidestreamColdMelon]

Unrelated to this code review, I wonder how current "whitelisting"-based monitoring implemented by techops works and if it detects votes on a random non-whitelisted contract before it's been too late / spell is already scheduled or if they even depend on the event emitted by plan to detect that something is off

[amusingaxl]

Let me raise that question to them.

[amusingaxl]

From Techops:

We're "whitelisting" the spells in advance before they are voted through and get the hat. Otherwise any spell that is not whitelisted and that gets the hat, will trigger a P1 alert saying that aunothorized spell has the hat. Where we'll have to react by involving stakeholders and doing necessary actions required for blocking it from being cast.
Idea is that if that happens and malicious spell is scheduled, we can quickly get all the people online and put "concurrent" spell that would be executed before the malicious one and will cancel it. Or start marketing and informational campaign notifying the users about it. Or preparing some press-release and informing about our next actions. So whitelist is mainly to not any unauthorized spells be executed silently and notifying us about it.

[SidestreamColdMelon]

Do you think that if we aim to allow potential usage of emergency spells without involvement of engineers, we might want to use less technical, more general terms in describing them? Also, we might want to describe not what they actually do, but when to use each of them.
Anyway, I guess this will be addressed in the place where the deployed contracts will be listed in the end – with some kind of guide on how to use them.

[amusingaxl]

Anyway, I guess this will be addressed in the place where the deployed contracts will be listed in the end – with some kind of guide on how to use them.

That's the intention. This repo's docs are more useful to a technical audience.

[SidestreamColdMelon]

Nit: I think it would be great to link 1) contract you're talking about 2) mom contract

(here, but also in every other section)

[amusingaxl]

Wouldn't it be better to maybe reference the chainlog key instead?
Contracts can be updated, making this document stale.

[SidestreamColdMelon]

Nit: I would recommend to emit uniform event from all emergency contracts, e.g.: EmergencyAction(bytes32 indexed type, bytes32 indexed what)

[amusingaxl]

Why do you believe this would be beneficial?

[DaiFoundation-DevOps]

All committers have signed the CLA.

[oddaf]

LGTM