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

[RFC]use bitmap to save space #11910

Closed
wants to merge 4 commits into from
Closed

[RFC]use bitmap to save space #11910

wants to merge 4 commits into from

Conversation

shileiwill
Copy link
Contributor

@shileiwill shileiwill commented Jan 27, 2024

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:

  • existing hardhat tests pass
  • existing foundry test pass.
pnpm prettier:check
yarn hardhat size-contracts
pnpm hardhat test test/v0.8/automation/AutomationRegistry2_2.test.ts all tests passed.
pnpm ts-node ./scripts/generate-automation-master-interface.ts nothing changed.
go generate core/gethwrappers/go_generate.go no wrappers regenerated.
forge test --match-path "*/AutomationRegistry2_2.t.sol"  all foundry tests pass.

Copy link
Contributor

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
Copy link
Contributor Author

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.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Updated.

Copy link
Contributor

Go solidity wrappers are out-of-date, regenerate them via the make wrappers-all command

Copy link
Contributor

github-actions bot commented Feb 5, 2024

Go solidity wrappers are out-of-date, regenerate them via the make wrappers-all command

Copy link
Contributor

github-actions bot commented Feb 5, 2024

Go solidity wrappers are out-of-date, regenerate them via the make wrappers-all command

@shileiwill shileiwill changed the title [RFC][No Merge] use bitmap to save space [RFC]use bitmap to save space Feb 5, 2024
totalPremium: totalPremium,
latestEpoch: 0, // DON restarts epoch
reorgProtectionEnabled: onchainConfig.reorgProtectionEnabled
boolFlags: (_isRegistryPaused() ? (1 << 0) : 0) |
(_isReentrancyGuarded() ? (1 << 1) : 0) |
Copy link
Contributor Author

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.

Comment on lines 957 to 955
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;
}
Copy link
Contributor

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.

@cl-sonarqube-production
Copy link

Quality Gate passed Quality Gate passed

Issues
0 New issues
0 Fixed issues
0 Accepted issues

Measures
0 Security Hotspots
No data about Coverage
No data about Duplication

See analysis details on SonarQube

@shileiwill shileiwill marked this pull request as draft February 28, 2024 16:56
Copy link
Contributor

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.

@github-actions github-actions bot added the Stale label Apr 29, 2024
@github-actions github-actions bot closed this May 6, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants