Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Feat: add grouped ilk emergency spells #10

Open
wants to merge 33 commits into
base: master
Choose a base branch
from

Conversation

amusingaxl
Copy link
Contributor

@amusingaxl amusingaxl commented Nov 25, 2024

I decided to remove the hard-coded ilks approach in favor of a more general one which allows us to specify the desired ilks in the constructor.

Regular spells are executed through a delegatecall from MCD_PAUSE_PROXY. For that reason, they usually should not have storage variables, as they'd be accessing and interacting with MCD_PAUSE_PROXY's storage, not their own.

However, Emergency Spells are not required to interact with MCD_PAUSE and MCD_PAUSE_PROXY at all. They execute actions through regular call on Mom contracts, so we don't have this limitation.

Even if the contract is somehow misused and used as a regular spell, interacting with MCD_PAUSE, there wouldn't be a problem because the storage cannot be changed outside the constructor.

This approach requires some amount of hacking due to some Solidity limitations (i.e.: no support for immutable arrays, bytes32 <-> string conversion shenanigans).

While there is still some code duplication, this is significantly reduced compared to the previous approach.

amusingaxl and others added 27 commits November 6, 2024 11:40
@amusingaxl amusingaxl changed the title Feat: add batched ilk spells Feat: add batched ilk emergency spells Nov 25, 2024
Comment on lines 31 to 34
/// @dev The min size for the list of ilks
uint256 public constant MIN_ILKS = 2;
/// @dev The max size for the list of ilks
uint256 public constant MAX_ILKS = 3;
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Technically, nothing prevents us from setting MIN_ILKS = 1 and replacing the single-ilk spells with this one. We'd just need to pass a list containing 1 single ilk to the constructor.

@amusingaxl amusingaxl marked this pull request as ready for review November 27, 2024 14:22
@amusingaxl amusingaxl changed the title Feat: add batched ilk emergency spells Feat: add grouped ilk emergency spells Nov 28, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

1 participant