-
Notifications
You must be signed in to change notification settings - Fork 608
SIP-135 Refactor Collateral contracts for L2 shorts #1448
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
Conversation
d1cffb6
to
baa4b40
Compare
…to meb-short-refactor
d5f108d
to
46e510d
Compare
* Add repayAndClose * Remove extra getLoanAndAccrueInterest * SIP-149 Removing Binary Options from the protocol (#1494) * Fixes for imports around VirtualSynth to prevent issues with IERC20 (#1497) * Update short tests * Fix shorts tests on l1 fork * Fix shorts tests on l1 fork * Update ovm-ignore.json * Update ovm-ignore.json * Rename closeLoanWithCollateral Co-authored-by: Mark Barrasso <mark@synthetix.io> Co-authored-by: jj <justin@synthetix.io>
…to meb-short-refactor
Codecov Report
@@ Coverage Diff @@
## develop #1448 +/- ##
===========================================
+ Coverage 95.21% 95.47% +0.25%
===========================================
Files 76 75 -1
Lines 1733 1744 +11
Branches 557 545 -12
===========================================
+ Hits 1650 1665 +15
+ Misses 83 79 -4
Continue to review full report at Codecov.
|
For
For
|
publish/releases.json
Outdated
@@ -293,6 +293,21 @@ | |||
"layer": "base", | |||
"released": "base" | |||
}, | |||
{ | |||
"sip": 135, | |||
"layer": "both", |
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.
Instead of this, I suggest targeting layer: ovm
and for the SystemSettings configs that are loans specific, put them in a conditional that checks if the SystemSettings contract interface has the function you expect (loaded up from the ABI). We don't want to tackle a multilayer deployment just for system setting configuration.
Some gas usage statistics for L2 shorts:
|
* Remove CollateralState contract * Add check to avoid zero transfer * Use SafeERC20 * Remove unnecessary isLoanOpen check * Minor fix to comments * Resolve audit fixes for CollateralManager * Add short coverage * Add more coverage to manager * Add service fee for collapsing loans * Update ovm params * Add interaction delay override * Add collapse fee rate to defaults * Update sip-135 release layer * Update configure-loans.js * Update l2 integration tests
"MixinSystemSettings", | ||
"ShortingRewards", | ||
"SystemSettings" | ||
] |
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.
you will also need to add SynthsUSD
, ETH
and BTC
here as you want them to be instances of MultiCollateralShort
.
* Enforce skew limit max * Fix manager deployment * Fix unit tests * Add synths to releases * checkout develop publish/deployed * Sync ovm params * Sync ovm synths * Update ovm config * Update local config * Update ovm params * Revert json crap Co-authored-by: Mark Barrasso <barrassomark@gmail.com>
return | ||
flexibleStorage().getUIntValue( | ||
SETTING_CONTRACT_NAME, | ||
keccak256(abi.encodePacked(SETTING_MIN_CRATIO, collateral)) |
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.
[np] would it be worth using a private function to do the keccak
? It might reduce the contract size impact of all these.
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.
nvm - did some testing and seems like it doesn't improve.
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.
💯
Tracking tasks here: Synthetixio/issues#264