Skip to content

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

Merged
merged 135 commits into from
Sep 22, 2021

Conversation

barrasso
Copy link
Member

@barrasso barrasso commented Aug 5, 2021

Tracking tasks here: Synthetixio/issues#264

@barrasso barrasso self-assigned this Aug 6, 2021
@barrasso barrasso added this to the West 4 milestone Aug 6, 2021
barrasso and others added 5 commits September 6, 2021 00:40
* 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>
@codecov
Copy link

codecov bot commented Sep 10, 2021

Codecov Report

Merging #1448 (e830cfc) into develop (2a3cd7e) will increase coverage by 0.25%.
The diff coverage is 96.15%.

Impacted file tree graph

@@             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     
Impacted Files Coverage Δ
contracts/CollateralManager.sol 89.55% <81.81%> (+5.42%) ⬆️
contracts/Collateral.sol 93.33% <96.87%> (-0.65%) ⬇️
contracts/CollateralErc20.sol 100.00% <100.00%> (ø)
contracts/CollateralEth.sol 100.00% <100.00%> (ø)
contracts/CollateralShort.sol 100.00% <100.00%> (+9.09%) ⬆️
contracts/MixinSystemSettings.sol 100.00% <100.00%> (ø)
contracts/SystemSettings.sol 100.00% <100.00%> (ø)

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 2a3cd7e...e830cfc. Read the comment docs.

@barrasso
Copy link
Member Author

barrasso commented Sep 15, 2021

For ovm deployment:

  1. Make sure interactionDelay is set to 0.

  2. Make sure minCollateral is set to 0.

  3. Make sure collapseFeeRate is set to 15 bps 0 by default.

  4. Ensure the CollateralManager has all Collateral contracts added. Only CollateralShort should be added to L2.

  5. Deploy shorting rewards process

  6. Is it necessary to filter shortable synths using getSynths?

For kovan-ovm:

  1. Make sure to upgrade sUNI and sAAVE

@@ -293,6 +293,21 @@
"layer": "base",
"released": "base"
},
{
"sip": 135,
"layer": "both",
Copy link
Contributor

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.

@barrasso
Copy link
Member Author

Some gas usage statistics for L2 shorts:

  • short.open: 9366958

  • short.withdraw: 4044849

  • short.draw: 5553515

  • short.close: 4548746

barrasso and others added 3 commits September 16, 2021 11:47
* 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"
]
Copy link
Contributor

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.

jj and others added 8 commits September 21, 2021 14:15
* 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>
@jjgonecrypto jjgonecrypto dismissed jacko125’s stale review September 22, 2021 15:05

No longer relevant

return
flexibleStorage().getUIntValue(
SETTING_CONTRACT_NAME,
keccak256(abi.encodePacked(SETTING_MIN_CRATIO, collateral))
Copy link
Contributor

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.

Copy link
Contributor

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.

Copy link
Contributor

@jjgonecrypto jjgonecrypto left a comment

Choose a reason for hiding this comment

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

💯

@jjgonecrypto jjgonecrypto merged commit 96be7e1 into develop Sep 22, 2021
@jjgonecrypto jjgonecrypto deleted the meb-short-refactor branch September 22, 2021 16:04
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.

4 participants