-
Notifications
You must be signed in to change notification settings - Fork 12.4k
perf: optimize ReentrancyGuardTransient with pre-computed BooleanSlot #5865
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
perf: optimize ReentrancyGuardTransient with pre-computed BooleanSlot #5865
Conversation
🦋 Changeset detectedLatest commit: 2f1f537 The changes in this PR will be included in the next version bump. This PR includes changesets to release 1 package
Not sure what this means? Click here to learn what changesets are. Click here if you're a maintainer who wants to add another changeset to this PR |
5b77380 to
c07b8d2
Compare
WalkthroughAdds a pre-computed TransientSlot.BooleanSlot constant (REENTRANCY_SLOT) in ReentrancyGuardTransient.sol to wrap REENTRANCY_GUARD_STORAGE. Replaces repeated _reentrancyGuardStorageSlot().asBoolean().tload()/tstore(...) calls with REENTRANCY_SLOT.tload()/tstore(...) in _nonReentrantBefore, _nonReentrantAfter, and _reentrancyGuardEntered. Updates related inline comments. Retains _reentrancyGuardStorageSlot() unchanged. Functional behavior remains unchanged. Also adds a changeset entry marking an openzeppelin-solidity patch release documenting this optimization. Possibly related issues
Possibly related PRs
✨ Finishing touches🧪 Generate unit tests
Tip 👮 Agentic pre-merge checks are now available in preview!Pro plan users can now enable pre-merge checks in their settings to enforce checklists before merging PRs.
Please see the documentation for more information. Example: reviews:
pre_merge_checks:
custom_checks:
- name: "Undocumented Breaking Changes"
mode: "warning"
instructions: |
Pass/fail criteria: All breaking changes to public APIs, CLI flags, environment variables, configuration keys, database schemas, or HTTP/GraphQL endpoints must be documented in the "Breaking Change" section of the PR description and in CHANGELOG.md. Exclude purely internal or private changes (e.g., code not exported from package entry points or explicitly marked as internal).Please share your feedback with us on this Discord post. Comment Pre-merge checks✅ Passed checks (3 passed)
|
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.
Actionable comments posted: 0
🧹 Nitpick comments (1)
.changeset/thick-goats-travel.md (1)
5-5: Minor discrepancy in gas savings documentation.The changeset mentions "30 gas per call" while the PR objectives specify "19 gas saved per nonReentrant call." Consider aligning these numbers for consistency across documentation.
-Optimize ReentrancyGuardTransient by pre-computing BooleanSlot wrapper to save 30 gas per call +Optimize ReentrancyGuardTransient by pre-computing BooleanSlot wrapper to save 19 gas per call
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
.changeset/thick-goats-travel.md(1 hunks)contracts/utils/ReentrancyGuardTransient.sol(2 hunks)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (6)
- GitHub Check: slither
- GitHub Check: coverage
- GitHub Check: tests-foundry
- GitHub Check: tests
- GitHub Check: halmos
- GitHub Check: tests-upgradeable
🔇 Additional comments (5)
contracts/utils/ReentrancyGuardTransient.sol (5)
22-30: Excellent gas optimization with clear documentation.The pre-computed BooleanSlot constant is a smart optimization that eliminates redundant type casting operations. The comprehensive comment clearly explains the rationale and quantifies the gas savings, making it easy for future maintainers to understand the purpose.
70-70: LGTM! Clean replacement of redundant operations.The direct usage of
REENTRANCY_SLOT.tstore(true)eliminates the need for the previous_reentrancyGuardStorageSlot().asBoolean().tstore(true)pattern, reducing gas consumption while maintaining identical functionality.Also applies to: 74-74
78-78: LGTM! Consistent optimization applied.The direct usage of
REENTRANCY_SLOT.tstore(false)follows the same optimization pattern as in_nonReentrantBefore(), maintaining consistency across the codebase.
86-86: LGTM! Optimization maintains function semantics.The direct usage of
REENTRANCY_SLOT.tload()eliminates the redundant_reentrancyGuardStorageSlot().asBoolean().tload()pattern while preserving the exact same return value and behavior.
89-91: Good design choice preserving backward compatibility.Maintaining the
_reentrancyGuardStorageSlot()function unchanged ensures that any external contracts or libraries that depend on this function for storage slot access continue to work without modification. This demonstrates excellent backward compatibility considerations.
| * reducing gas consumption by 19 gas. | ||
| */ | ||
| TransientSlot.BooleanSlot private constant REENTRANCY_SLOT = | ||
| TransientSlot.BooleanSlot.wrap(REENTRANCY_GUARD_STORAGE); |
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.
We need to account for _reentrancyGuardStorageSlot being virtual.
|
This PR uses a constant object that correspond to the default location ( Doing that change would be breaking, and would provide no alternative workflow to override the slot used. |
|
For full context on this optimization’s evolution: This PR was originally opened against the version that repeatedly called After the first refactor (introduction of The recent introduction of the virtual Under this new architecture, my proposed version hardcodes the slot to the default value, silently breaking any contract that overrides it. I agree that’s an unacceptable trade-off for a utility designed to be extended. While the gas savings were meaningful in earlier versions, I fully respect the decision to prioritize correctness and composability over micro-optimizations, especially in foundational code. Made sense when the slot was static. Doesn’t anymore. Appreciate the review. |
Description
Eliminates redundant type casting in
ReentrancyGuardTransientby pre-computing theBooleanSlotwrapper at compile-time.Edit: Updated to support the new
nonReentrantViewmodifier introduced in the latest version.Current implementation
Performs
asBoolean()→BooleanSlot.wrap()three times pernonReentrantexecution.Optimization
Pre-computed constant:
Gas savings
19 gas per
nonReentrantcall (verified via test suite).callback()guardedCheckEntered()Eliminates 30 gas in wrapping operations, reduced to 19 gas net due to function call overhead from the refactored architecture.
Deployment overhead: 1,728 gas (amortized after ~91 transactions).
Testing
Impact
nonReentrantandnonReentrantViewmodifiers.