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.

Sign up for GitHub

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

Jump to bottom

[VEN-1146]: Comptroller Diamond Proxy #224

Merged
merged 164 commits into from
Sep 25, 2023
Merged

[VEN-1146]: Comptroller Diamond Proxy #224

merged 164 commits into from
Sep 25, 2023

Conversation

Debugger022
Copy link
Contributor

@Debugger022 Debugger022 commented Mar 6, 2023 •
edited
Loading

Description

  1. Unitroller changes for a custom slot and diamond support
  • Added Storage Structure which contains function selector and facet Address.
  • Maps function selector to the facet address and the position of the selector in the facetFunctionSelectors.selectors array.
  1. The diamond proxy initial structure - EIP2535
  • Implements Diamond which contains a fallback function and more functions to set, and update facets and their function selectors.
  • Implements Lens functions standard functions used to show what functions and facets a diamond has.

Facets:

  1. PolicyFacet
  • This facet contains all the external pre-hook functions related to vToken.
  1. SetterFacet
  • This facet contains all the configurational setter functions.
  1. MarketFacet
  • Market Facet contains the function about the asset in the market and the function for entering and exiting the market.
  1. RewardFacet
  • Reward Facet provides the external functions related to all claims and rewards of the protocol.

Other Contracts:

  1. XVSRewardsHelper
  • It contains the methods related to the XVS rewards which are used in the PolicyFacet and RewardFacet.
  1. FacetBase
  • It contains the common methods which are used in each facet.

Extensive testing on comptroller changes.

  • Test all the facets and function selectors of the diamond.
  • Test add/replace/remove functions.
  • Test All facets functions.
  • Test for collision-free Storage.

Removed old comptroller contract (Comptroller.sol) and it's dependencies. [VEN_1580]

Fixed FairyProof audit suggestions [VEN-1619]

Fixed Peckshield audit suggestions [VEN-1699]

FIxed VEN-1686

@kkirka kkirka force-pushed the feat/delegated-borrows branch 2 times, most recently from 4d972ec to d22570a Compare March 6, 2023 11:47
@Debugger022 Debugger022 changed the base branch from feat/delegated-borrows to develop March 6, 2023 13:47
@Debugger022 Debugger022 changed the base branch from develop to feat/delegated-borrows March 6, 2023 13:47
@Debugger022 Debugger022 changed the base branch from feat/delegated-borrows to develop March 6, 2023 14:20
kkirka
kkirka reviewed Mar 23, 2023
View reviewed changes
Copy link
Collaborator

@kkirka kkirka left a comment

Choose a reason for hiding this comment

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

I'm doing another pass, leaving some notes here for now

contracts/Comptroller/ComptrollerG2.sol Outdated Show resolved Hide resolved
contracts/Diamond/libraries/LibHelper.sol Outdated Show resolved Hide resolved
contracts/Comptroller/Comptroller.sol Outdated Show resolved Hide resolved
chechu
chechu reviewed Mar 25, 2023
View reviewed changes
Copy link
Member

@chechu chechu left a comment

Choose a reason for hiding this comment

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

GovernorAlpha.sol and GovernorAlpha2.sol are old contracts. Their ABI may be needed for subgraph, but other than that these contracts are not used.

Moreover, the following contracts are being moved to a different package (see it here), so I wouldn't upgrade them here, and I would remove from this repo after merging and releasing the other PR

  • GovernorBravoDelegate.sol
  • GovernorBravoDelegator.sol
  • GovernorBravoInterfaces.sol
  • IAccessControlManager.sol
  • Timelock.sol

chechu
chechu reviewed Mar 25, 2023
View reviewed changes
Copy link
Member

@chechu chechu left a comment

Choose a reason for hiding this comment

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

We have to consider always how each change can be deployed to the main net

contracts/Diamond/upgradeInitialize/DiamondInit.sol Outdated Show resolved Hide resolved
contracts/Governance/VTreasury.sol Outdated Show resolved Hide resolved
contracts/InterestRateModels/InterestRateModel.sol Outdated Show resolved Hide resolved
contracts/Lens/ComptrollerLens.sol Outdated Show resolved Hide resolved
contracts/Oracle/VenusChainlinkOracle.sol Outdated Show resolved Hide resolved
contracts/Tokens/VRT/VRT.sol Outdated Show resolved Hide resolved
contracts/Tokens/VTokens/VBNB.sol Outdated Show resolved Hide resolved
contracts/Tokens/VTokens/VBep20Delegator.sol Outdated Show resolved Hide resolved
contracts/Tokens/XVS/XVS.sol Outdated Show resolved Hide resolved
contracts/VRTVault/VRTVaultProxy.sol Outdated Show resolved Hide resolved
@Debugger022
Copy link
Contributor Author

GovernorAlpha.sol and GovernorAlpha2.sol are old contracts. Their ABI may be needed for subgraph, but other than that these contracts are not used.

Moreover, the following contracts are being moved to a different package (see it here), so I wouldn't upgrade them here, and I would remove from this repo after merging and releasing the other PR

  • GovernorBravoDelegate.sol
  • GovernorBravoDelegator.sol
  • GovernorBravoInterfaces.sol
  • IAccessControlManager.sol
  • Timelock.sol

I will undo the changes as they make no sense then.

@Debugger022 Debugger022 marked this pull request as ready for review March 30, 2023 12:35
@Debugger022 Debugger022 added the don't merge Need to merge after passing test label Mar 30, 2023
@Debugger022 Debugger022 changed the title [WIP] Comptroller Diamond Proxy Comptroller Diamond Proxy Mar 30, 2023
Debugger022 and others added 10 commits September 8, 2023 16:14
@Debugger022
[VEN-1887]: Quantstamp audit fix for comptroller diamond proxy (#328)
df08df4 
* fix: ven-1887 ven-08

* fix: ven-1887 ven-12

* fix: ven-1887 ven-04

* fix: lint

* fix: replaced And operator with OR while checking cutoff

* refactor: netspec comments

* Revert "fix: ven-1887 ven-12"

This reverts commit 0e24ded.

* fix: lint
@Debugger022
Revert "[VEN-1887]: Quantstamp audit fix for comptroller diamond proxy (
9af2d4a 
#328)" (#337)

This reverts commit df08df4.
@Debugger022
Merge pull request #338 from VenusProtocol/VEN-1887
3586e05 
[VEN-1887]: Quantstamp audit fix for comptroller diamond proxy
@Debugger022
refactor: optimized code
0af25d4
@Debugger022
fix: resolved conflicts
b4ddc09
@Debugger022
fix: resolved conflicts
1f1b0f9
@Debugger022
fix: pr comments
35b5517
@Debugger022
fix: merge branch 'develop' into feat/diamond-proxy
e6e9ca5
@Debugger022
fix: updated yarn.lock
032cc8e
@Debugger022
Merge pull request #335 from VenusProtocol/feat/force-liquidation-dia…
3313948 
…mond

[VEN-1929]: force liquidation functionality in diamond implementation of comptroller
kkirka
kkirka reviewed Sep 18, 2023
View reviewed changes
Copy link
Collaborator

@kkirka kkirka left a comment

Choose a reason for hiding this comment

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

LGTM, left some comments that are not a blocker for merging & releasing

contracts/Comptroller/ComptrollerInterface.sol Outdated Show resolved Hide resolved
contracts/Lens/InterestRateModelLens.sol Show resolved Hide resolved
contracts/test/ComptrollerMock.sol Show resolved Hide resolved
script/deploy/comptroller/facet-cut-params-generator.ts Show resolved Hide resolved
script/hardhat/fork/vip-framework/utils.ts Show resolved Hide resolved
tests/hardhat/Comptroller/Diamond/accessControl.ts Outdated Show resolved Hide resolved
tests/hardhat/Comptroller/Diamond/assetListTest.ts Show resolved Hide resolved
GitGuru7
GitGuru7 approved these changes Sep 18, 2023
View reviewed changes
Copy link
Contributor

@GitGuru7 GitGuru7 left a comment

Choose a reason for hiding this comment

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

LGTM !

chechu
chechu reviewed Sep 18, 2023
View reviewed changes
tests/hardhat/Fork/diamondTest.ts Outdated Show resolved Hide resolved
tests/hardhat/Fork/diamondTest.ts Outdated Show resolved Hide resolved
tests/hardhat/Fork/diamondTest.ts Outdated Show resolved Hide resolved
tests/hardhat/Fork/diamondTest.ts Outdated Show resolved Hide resolved
tests/hardhat/Fork/diamondTest.ts Outdated Show resolved Hide resolved
tests/hardhat/Fork/diamondTest.ts Show resolved Hide resolved
tests/hardhat/Fork/diamondTest.ts Outdated Show resolved Hide resolved
tests/hardhat/Fork/diamondTest.ts Outdated Show resolved Hide resolved
tests/hardhat/Fork/diamondTest.ts Show resolved Hide resolved
@github-actions
Copy link

Code Coverage

Package Line Rate Branch Rate Health
Comptroller 100% 90% ✔
Comptroller.Diamond 95% 59% ✔
Comptroller.Diamond.facets 73% 62% ➖
Comptroller.Diamond.interfaces 100% 100% ✔
DelegateBorrowers 100% 86% ✔
Governance 26% 25% ❌
InterestRateModels 0% 0% ❌
Lens 41% 37% ❌
Liquidator 99% 89% ✔
Oracle 100% 100% ✔
PegStability 88% 84% ✔
Swap 91% 58% ✔
Swap.interfaces 100% 100% ✔
Swap.lib 81% 53% ✔
Tokens 100% 100% ✔
Tokens.VAI 71% 46% ➖
Tokens.VRT 20% 9% ❌
Tokens.VTokens 35% 20% ❌
Tokens.XVS 19% 8% ❌
Utils 53% 35% ➖
VRTVault 49% 36% ❌
Vault 50% 45% ➖
XVSVault 65% 52% ➖
Summary 55% (2243 / 4054) 40% (925 / 2304) ➖

@chechu chechu removed the don't merge Need to merge after passing test label Sep 25, 2023
@Debugger022 Debugger022 merged commit 3733de6 into develop Sep 25, 2023
3 checks passed
@kkirka kkirka deleted the feat/diamond-proxy branch November 29, 2023 15:44
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@kkirka kkirka kkirka left review comments

@chechu chechu chechu approved these changes

@web3rover web3rover web3rover approved these changes

@0xlucian 0xlucian 0xlucian approved these changes

@GitGuru7 GitGuru7 GitGuru7 approved these changes

@therealemjy therealemjy Awaiting requested review from therealemjy

Assignees

@Debugger022 Debugger022

Labels
audit This content is being audited
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
7 participants
@Debugger022 @chechu @web3rover @kkirka @therealemjy @0xlucian @GitGuru7