-
Notifications
You must be signed in to change notification settings - Fork 15
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
Fix public immutable state #449
Conversation
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 believe merge target should be v2.3-fix-review
instead of 2.3
.
Note that PE-1682 adds additional keep configs. I will move those from ctor to initializer in my [ed/pe-1682](https://github.com/equilibria-xyz/perennial-v2/tree/ed/pe-1682)
branch. We'll need to resolve a merge conflict there when this comes together.
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. Happy to help merge this with PR #451 .
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.
one nit but lgtm overall
@@ -36,30 +36,31 @@ abstract contract Controller_Incentivized is Controller, IRelayer, Kept { | |||
KeepConfig public keepConfig; | |||
|
|||
/// @dev Handles relayed messages for nonce cancellation | |||
IVerifierBase public nonceManager; | |||
IVerifierBase public immutable nonceManager; |
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.
can we move this above the mutable keepConfig just to make the storage layout clearer?
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.
Fixed in 7faab93
[Periphery] Unit Test Coverage ReportCoverage after merging prateek/fix-immutable-state into v2.3-fix-review will be
Coverage Report
|
[Core] Integration Test Coverage ReportCoverage after merging prateek/fix-immutable-state into v2.3-fix-review will be
Coverage Report
|
[Periphery] Integration Test Coverage ReportCoverage after merging prateek/fix-immutable-state into v2.3-fix-review will be
Coverage Report
|
[Periphery] Combined Test Coverage ReportCoverage after merging prateek/fix-immutable-state into v2.3-fix-review will be
Coverage Report
|
[Core] Unit Test Coverage ReportCoverage after merging prateek/fix-immutable-state into v2.3-fix-review will be
Coverage Report
|
[Core] Combined Test Coverage ReportCoverage after merging prateek/fix-immutable-state into v2.3-fix-review will be
Coverage Report
|
Issue: https://linear.app/perennial/issue/PE-1789/remove-public-states-set-in-constructor-from-upgradeable-contracts