Add priority emergency spells #1
Conversation
Make sure the spell does not revert if any of the `Clip` contracts has not relied on `ClipperMom`.
Make sure the spell does not revert if any of the `Clip` contracts has not relied on `ClipperMom`.
Define the expiration as `type(uint26).max`, meaning the emergency spells should never expire.
The new name conveys the meaning better.
In the unlikely case the spell emergency actions would revert due to block gas limit, there is an escape hatch through a custom method that would allow users to split the execution into batches.
In the unlikely case the spell emergency actions would revert due to block gas limit, there is an escape hatch through a custom method that would allow users to split the execution into batches.
Add config to run tests automatically on pull requests
| function schedule() external { | ||
| _emergencyActions(); | ||
| } |
There was a problem hiding this comment.
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
etalogic, 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
etainsidespell.scheduleto non zero- Additional complexity: Setting
etawill prevent spell from being directly reusable. We can potentially circumvent this by modifying it back to 0 either viacastor manually/permissionlessly via some other, e.g.reset()method
- Additional complexity: Setting
- 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
plotwould've been called via emergency spell, keeper can checkpause.plans[hash] != trueinstead ofspell.eta != 0
- Additional complexity: there no cross-compatible event or state variable that can be used for checking if "a spell" was scheduled. For example, if
All in all, I would recommend to make some changes to the base contract to prevent keeper loop.
| function nextCastTime() external view returns (uint256 castTime) { | ||
| return _nextCastTime; | ||
| } |
There was a problem hiding this comment.
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)
| 1. No `MCD_PAUSE` interaction: as its name might suggest, the main goal of `MCD_PAUSE` is to introduce a _pause_ (GSM | ||
| delay) between the approval of a spell and its execution. Emergency spells by definition bypass the GSM delay, so | ||
| there is no strong reason to `plan` them in `MCD_PAUSE` as regular spells. |
There was a problem hiding this comment.
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
| | Description | Single ilk | Universal | | ||
| | :---------- | :--------: | :-------: | | ||
| | Wipe `AutoLine` | :white_check_mark: | :white_check_mark: | | ||
| | Set `Clip` breaker | :white_check_mark: | :white_check_mark: | | ||
| | Disable `DDM` | :white_check_mark: | :x: | | ||
| | Stop `OSM` | :white_check_mark: | :white_check_mark: | |
There was a problem hiding this comment.
Do you that 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.
| No further debt can be generated from an ilk which is wiped from `AutoLine`. It also prevents the debt ceiling | ||
| (`line`) for the affected ilk from being changed without Governance interference. |
There was a problem hiding this comment.
I think it would be great to link 1) contract you're talking about 2) mom contract
(here, but also in every other section)
| /** | ||
| * @notice Returns `nextCastTime`, regardless of the input parameter. | ||
| * @dev this function exists only to keep interface compatibility with regular spells. | ||
| */ | ||
| function nextCastTime(uint256) external view returns (uint256 castTime) { | ||
| return _nextCastTime; | ||
| } |
There was a problem hiding this comment.
This function is defined two times
| IlkRegistryLike public immutable ilkReg = IlkRegistryLike(_log.getAddress("ILK_REGISTRY")); | ||
| LineMomLike public immutable lineMom = LineMomLike(_log.getAddress("LINE_MOM")); | ||
|
|
||
| event Wipe(bytes32 indexed ilk); |
There was a problem hiding this comment.
I would recommend to emit uniform event from all emergency contracts, e.g.: EmergencyAction(bytes32 indexed type, bytes32 indexed what)
https://docs.google.com/document/d/1zYgX_YQTDnjl1vO7EYeryicVtEzardikqE1t3tLyo74