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.

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

Add rollupManager initializer versioning #314

Merged
merged 4 commits into from
Aug 29, 2024

Conversation

ignasirv
Copy link
Contributor

No description provided.

* Initializer function to set new rollup manager version
*/
function initialize() external virtual reinitializer(3) {
emit UpdateRollupManagerVersion("banana");
Copy link
Collaborator

Choose a reason for hiding this comment

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

Add constant instead ^^

@@ -231,6 +231,8 @@ contract PolygonRollupManager is
bytes32 internal constant _EMERGENCY_COUNCIL_ADMIN =
keccak256("EMERGENCY_COUNCIL_ADMIN");

string internal constant _ROLLUP_MANAGER_VERSION_PESSIMISTIC = "pessimistic";
Copy link
Collaborator

Choose a reason for hiding this comment

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

maybe it's worth it to have a constnat that always have the same name, and public, to be able to easy retrieval from the node 🤔 something like currentVersion!?¿

Copy link
Contributor

Choose a reason for hiding this comment

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

yep, agree. See comment above xD

@@ -231,6 +231,8 @@ contract PolygonRollupManager is
bytes32 internal constant _EMERGENCY_COUNCIL_ADMIN =
keccak256("EMERGENCY_COUNCIL_ADMIN");

string internal constant _ROLLUP_MANAGER_VERSION_PESSIMISTIC = "pessimistic";
Copy link
Contributor

Choose a reason for hiding this comment

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

I would make this public, so it is easy to check from the SC directly.
Also, _ROLLUP_MANAGER_VERSION_PESSIMISTIC should have a generic naming like _ROLLUP_MANAGER_VERSION and it value reflects the version.

@@ -231,6 +231,8 @@ contract PolygonRollupManager is
bytes32 internal constant _EMERGENCY_COUNCIL_ADMIN =
keccak256("EMERGENCY_COUNCIL_ADMIN");

string internal constant _ROLLUP_MANAGER_VERSION_PESSIMISTIC = "pessimistic";
Copy link
Contributor

Choose a reason for hiding this comment

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

yep, agree. See comment above xD

@@ -270,6 +270,9 @@ contract PolygonRollupManager is
// Last timestamp when an aggregation happen
uint64 public lastAggregationTimestamp;

// Current rollup manager version
string public constant ROLLUP_MANAGER_VERSION = "pessimistic";
Copy link
Collaborator

Choose a reason for hiding this comment

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

move it to constants part of the file^^

Copy link

sonarcloud bot commented Aug 29, 2024

Copy link
Contributor

@krlosMata krlosMata left a comment

Choose a reason for hiding this comment

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

😸

@ignasirv ignasirv merged commit c263786 into feature/ongoingPP Aug 29, 2024
6 checks passed
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.

3 participants