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

fix: remove int64 limitations #719

Merged
merged 10 commits into from
Apr 4, 2023

Conversation

krogla
Copy link
Member

@krogla krogla commented Mar 30, 2023

List of changes:

  • Use the uint256 type instead of uint64 in NodeOperatorsRegestry to work with node operator validators' data. Validate that the resulting value doesn't exceed the UINT64_MAX in the Packed64x4.set() method.
  • Replace calculations on signed variables with equivalent on unsigned int type
  • Prevent math operation overflows using SafeMath or asserts before/after calculations

NodeOperatorsRegistry contract size reduced from 23.971 to 23.169 KB

Gase changes for most often used methods based on unit tests

Method Name Prev Version New Version
obtainDepositData() 194,414 189,988
onExitedAndStuckValidatorsCountsUpdated() 8,300,043 8,148,381
updateExitedValidatorsCount() 118,450 115,006

@TheDZhon
Copy link
Contributor

TheDZhon commented Apr 2, 2023

@Psirex please consider resolving the conflicts by integrating the changes from the upstream branch (have divergent changes of the lastly pushed commits) 🙏

Copy link
Contributor

@TheDZhon TheDZhon left a comment

Choose a reason for hiding this comment

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

LGTM 👍

Please consider addressing the minor outdated naming issue

contracts/0.4.24/nos/NodeOperatorsRegistry.sol Outdated Show resolved Hide resolved
@TheDZhon TheDZhon merged commit 847d23c into fix/shapella-upgrade-from-rc0-to-rc1 Apr 4, 2023
@TheDZhon TheDZhon deleted the fix/nor-int64-math branch April 4, 2023 12:22
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants