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: Build initial implementation with unit testing (SC-204) #1

Merged
merged 21 commits into from
Nov 14, 2023

Conversation

lucas-manuel
Copy link
Contributor

@lucas-manuel lucas-manuel commented Nov 3, 2023

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.

Copy link

linear bot commented Nov 3, 2023

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.

Copy link
Contributor

@hexonaut hexonaut left a 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.

@lucas-manuel
Copy link
Contributor Author

Getting full coverage in next PR

@hexonaut hexonaut requested a review from barrutko November 10, 2023 10:29
Copy link
Contributor

@hexonaut hexonaut left a 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.

src/SparkLendFreezerMom.sol Outdated Show resolved Hide resolved
src/SparkLendFreezerMom.sol Outdated Show resolved Hide resolved
src/SparkLendFreezerMom.sol Outdated Show resolved Hide resolved
src/interfaces/ISparkLendFreezerMom.sol Outdated Show resolved Hide resolved
src/interfaces/ISparkLendFreezerMom.sol Outdated Show resolved Hide resolved
test/SparkLendFreezerMom.t.sol Outdated Show resolved Hide resolved
test/SparkLendFreezerMom.t.sol Show resolved Hide resolved
test/SparkLendFreezerMom.t.sol Outdated Show resolved Hide resolved
Copy link

@barrutko barrutko left a 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)

test/SparkLendFreezerMom.t.sol Show resolved Hide resolved
src/SparkLendFreezerMom.sol Outdated Show resolved Hide resolved
src/SparkLendFreezerMom.sol Outdated Show resolved Hide resolved
src/interfaces/ISparkLendFreezerMom.sol Outdated Show resolved Hide resolved
Copy link

Coverage after merging sc-204-build-gsm-bypass into master will be

83.33%

Coverage Report
FileStmtsBranchesFuncsLinesUncovered Lines
src
   SparkLendFreezerMom.sol79.31%50%100%83.33%91–96

Copy link
Contributor

@hexonaut hexonaut left a comment

Choose a reason for hiding this comment

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

LGTM

@lucas-manuel lucas-manuel merged commit 226ca07 into master Nov 14, 2023
2 of 3 checks passed
@lucas-manuel lucas-manuel deleted the sc-204-build-gsm-bypass branch November 14, 2023 15:07
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants