-
Notifications
You must be signed in to change notification settings - Fork 170
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
[VEN-1146]: Comptroller Diamond Proxy #224
Conversation
4d972ec
to
d22570a
Compare
5d1c6a5
to
b382fb0
Compare
43f5825
to
eb2a5c4
Compare
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'm doing another pass, leaving some notes here for now
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.
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
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.
We have to consider always how each change can be deployed to the main net
I will undo the changes as they make no sense then. |
* 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
[VEN-1887]: Quantstamp audit fix for comptroller diamond proxy
…mond [VEN-1929]: force liquidation functionality in diamond implementation of comptroller
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, left some comments that are not a blocker for merging & releasing
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 !
BREAKING CHANGE: Removal of comptroller.sol
|
Description
Facets:
Other Contracts:
Extensive testing on comptroller changes.
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