-
Notifications
You must be signed in to change notification settings - Fork 1.7k
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
[RFC]use bitmap to save space #11910
Conversation
I see that you haven't updated any CHANGELOG files. Would it make sense to do so? |
@@ -300,17 +299,19 @@ contract AutomationRegistry2_2 is AutomationRegistryBase2_2, OCR2Abstract, Chain | |||
s_signersList = signers; | |||
s_transmittersList = transmitters; | |||
|
|||
// TODO bitmap |
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.
will remove this comment.
To confirm, is this the place where we build the hotvars? if so, seems we are using some existing values to build the hotvar object, e.g. reentrancyGuard: s_hotVars.reentrancyGuard. want to make sure this is correct.
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.
Updated.
Go solidity wrappers are out-of-date, regenerate them via the |
Go solidity wrappers are out-of-date, regenerate them via the |
Go solidity wrappers are out-of-date, regenerate them via the |
totalPremium: totalPremium, | ||
latestEpoch: 0, // DON restarts epoch | ||
reorgProtectionEnabled: onchainConfig.reorgProtectionEnabled | ||
boolFlags: (_isRegistryPaused() ? (1 << 0) : 0) | | ||
(_isReentrancyGuarded() ? (1 << 1) : 0) | |
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.
_isReentrancyGuarded() is not necessary, we can always return 0, and no function needed.
function _isRegistryPaused() internal view returns (bool) { | ||
return (s_hotVars.boolFlags & (1 << 0)) != 0; | ||
} | ||
|
||
/** | ||
* @notice Returns true if the registry is reentrancy guarded, false otherwise | ||
*/ | ||
function _isReentrancyGuarded() internal view returns (bool) { | ||
return (s_hotVars.boolFlags & (1 << 1)) != 0; | ||
} | ||
|
||
/** | ||
* @notice Returns true if the registry enabled reorg protection, false otherwise | ||
*/ | ||
function _isReorgProtectionEnabled() internal view returns (bool) { | ||
return (s_hotVars.boolFlags & (1 << 2)) != 0; | ||
} |
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.
all of these functions do a state read (this is why we use the s_
prefix for state variables, to make state reads obvious). Meaning if we call these three functions in the same tx, they will all pay 2000 gas to read the same variable. Not a good use of gas! This will actually increase the total gas needed for a tx, and we wanted to combine these booleans into one in order to save gas!
A simple way to fix this is to accept the boolFlags
param as an argument. This way we can read it once from state in transmit()
and then pass it to each function when needed.
315e107
to
fa5e766
Compare
fa5e766
to
8c53e48
Compare
Quality Gate passedIssues Measures |
This PR is stale because it has been open 60 days with no activity. Remove stale label or comment or this will be closed in 7 days. |
Update on Feb 28, this PR is deprioritized. Will do the optimization for v2.3 if time allows.
I came across this Bitmap lib from OpenZepplin, could be a good fit, could also be an overkill since we need just one bucket.
Request for comment. Will generate wrappers etc after alignment.
Test plan: