-
Notifications
You must be signed in to change notification settings - Fork 1
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: Build initial implementation with unit testing (SC-204) #1
Conversation
SC-204 Build Spark GSM Bypass
This contract should allow the Maker chief contract to (un)pause any market on Spark mainnet. This should use the EMERGENCY_ADMIN role to silo the access out of an abundance of caution. This type of contract is called a "Mom" contract in Maker-lingo. You can see an example of the structure here: https://github.com/makerdao/clipper-mom/blob/master/src/ClipperMom.sol I would also like to consider making this forward-looking. If, for example, we could have another Spark Proxy deployed with EMERGENCY_ADMIN access which can only be triggered by this "Mom" to bypass the delay we could use this generalized structure instead of having to build a Mom contract for every specific function call on Spark. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I would use the exact structure of the ClipperMom
including all naming. There is a single owner
(pause proxy), the authority
is the chief and the modifiers are onlyOwner
(https://github.com/makerdao/clipper-mom/blob/master/src/ClipperMom.sol#L53) and auth
(https://github.com/makerdao/clipper-mom/blob/master/src/ClipperMom.sol#L58) which calls the isAuthorized
function which allows being called by the owner
as well as the hat.
Keeping this consistent will reduce the cognitive overhead with emergency spells.
Also, add this to the bottom of the README:
***
*The IP in this repository was assigned to Mars SPC Limited in respect of the MarsOne SP*
I will continue review after these changes are made.
Getting full coverage in next PR |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Some minor changes/fixes. Unit tests are also missing event tests.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM (HMU for approve after implementing Sam's comments)
Coverage after merging sc-204-build-gsm-bypass into master will be
Coverage Report
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM
NOTE: Interface is incomplete, will complete in a separate PR with natspec and overrides once everything is solidified.
Also left events and events testing out, but can add those as part of this PR if we want.