feat: mark setAuthorizer + _setAuthorizer virtual#306
Merged
Conversation
Extension hook for downstream vaults that need pairing-time invariants on the authorizer (e.g. verifying role-admin hierarchies for application- specific roles before accepting the install). Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Contributor
WalkthroughThis PR marks the internal ChangesAuthorizer method virtualization
🎯 1 (Trivial) | ⏱️ ~2 minutes 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/concrete/vault/OffchainAssetReceiptVault.sol (1)
363-370: 🛠️ Refactor suggestion | 🟠 Major | ⚡ Quick winEnhance documentation for virtual override guidance.
Now that
_setAuthorizeris markedvirtual, derived contracts can override this function to add custom validation (e.g., role-admin hierarchy checks). The existing documentation should be expanded to guide safe override patterns, specifically:
- Derived contracts should either call
super._setAuthorizer(newAuthorizer)to preserve the ERC165 interface check andAuthorizerSetevent emission, OR- Implement their own complete validation and event emission if custom behavior is needed
This will help prevent derived contracts from accidentally bypassing the compatibility check or breaking off-chain indexing that depends on the event.
📝 Suggested documentation enhancement
- /// Internal function to set the authorizer contract. This has no access - /// control so it MUST only be externally accessible by functions with an - /// access check. + /// Internal function to set the authorizer contract. This has no access + /// control so it MUST only be externally accessible by functions with an + /// access check. + /// + /// This function is virtual to allow derived contracts to enforce additional + /// invariants on the authorizer (e.g., verifying role-admin hierarchies). + /// Derived contracts that override SHOULD either: + /// - Call `super._setAuthorizer(newAuthorizer)` to preserve the ERC165 check + /// and event emission, OR + /// - Implement complete validation and emit AuthorizerSet themselves. /// `@param` newAuthorizer The new authorizer contract. function _setAuthorizer(IAuthorizeV1 newAuthorizer) internal virtual {🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/concrete/vault/OffchainAssetReceiptVault.sol` around lines 363 - 370, Update the NatSpec/comment for the internal virtual function _setAuthorizer(IAuthorizeV1 newAuthorizer) in OffchainAssetReceiptVault so it instructs derived contracts to either call super._setAuthorizer(newAuthorizer) (to preserve the IERC165 supportsInterface check and the AuthorizerSet event emission and to keep storage updates via OffchainAssetReceiptVault7201Storage/getStorageOffchainAssetReceiptVault) or, if overriding, to perform equivalent compatibility validation and emit the AuthorizerSet event themselves; mention the risks of bypassing the check or event (breaking off-chain indexing) and reference the IERC165 supportsInterface, OffchainAssetReceiptVault7201Storage, getStorageOffchainAssetReceiptVault, and AuthorizerSet symbols.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@src/concrete/vault/OffchainAssetReceiptVault.sol`:
- Around line 375-377: Add NatSpec documentation to the
setAuthorizer(IAuthorizeV1) function explaining that it is declared virtual to
allow derived contracts to extend or customize authorizer setup, and state
explicit override expectations: any override must preserve the onlyOwner access
control (or re-enforce equivalent checks), must maintain core invariants
enforced by _setAuthorizer, and should call super.setAuthorizer(newAuthorizer)
or otherwise invoke _setAuthorizer(newAuthorizer) to ensure base behavior is
executed; reference the _setAuthorizer internal function and the IAuthorizeV1
parameter so implementers know what to call and what to preserve.
---
Outside diff comments:
In `@src/concrete/vault/OffchainAssetReceiptVault.sol`:
- Around line 363-370: Update the NatSpec/comment for the internal virtual
function _setAuthorizer(IAuthorizeV1 newAuthorizer) in OffchainAssetReceiptVault
so it instructs derived contracts to either call
super._setAuthorizer(newAuthorizer) (to preserve the IERC165 supportsInterface
check and the AuthorizerSet event emission and to keep storage updates via
OffchainAssetReceiptVault7201Storage/getStorageOffchainAssetReceiptVault) or, if
overriding, to perform equivalent compatibility validation and emit the
AuthorizerSet event themselves; mention the risks of bypassing the check or
event (breaking off-chain indexing) and reference the IERC165 supportsInterface,
OffchainAssetReceiptVault7201Storage, getStorageOffchainAssetReceiptVault, and
AuthorizerSet symbols.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro
Run ID: a4a1b21e-1788-4c47-943d-72a88701803c
📒 Files selected for processing (1)
src/concrete/vault/OffchainAssetReceiptVault.sol
Comment on lines
+375
to
377
| function setAuthorizer(IAuthorizeV1 newAuthorizer) external virtual onlyOwner { | ||
| _setAuthorizer(newAuthorizer); | ||
| } |
Contributor
There was a problem hiding this comment.
🛠️ Refactor suggestion | 🟠 Major | ⚡ Quick win
Document the virtual nature and override expectations.
The setAuthorizer function is now virtual, allowing derived contracts to customize authorizer-setting behavior while preserving the onlyOwner access control. The documentation should clarify override expectations to ensure derived contracts maintain the core invariants.
📝 Suggested documentation enhancement
- /// Sets the authorizer contract. This is a critical operation and should be
- /// done with extreme care by the owner.
+ /// Sets the authorizer contract. This is a critical operation and should be
+ /// done with extreme care by the owner.
+ ///
+ /// This function is virtual to allow derived contracts to enforce pairing-time
+ /// invariants on the authorizer. Derived contracts that override SHOULD call
+ /// `_setAuthorizer(newAuthorizer)` or `super.setAuthorizer(newAuthorizer)` to
+ /// maintain the ERC165 compatibility check and event emission.
/// `@param` newAuthorizer The new authorizer contract.
function setAuthorizer(IAuthorizeV1 newAuthorizer) external virtual onlyOwner {📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| function setAuthorizer(IAuthorizeV1 newAuthorizer) external virtual onlyOwner { | |
| _setAuthorizer(newAuthorizer); | |
| } | |
| /// Sets the authorizer contract. This is a critical operation and should be | |
| /// done with extreme care by the owner. | |
| /// | |
| /// This function is virtual to allow derived contracts to enforce pairing-time | |
| /// invariants on the authorizer. Derived contracts that override SHOULD call | |
| /// `_setAuthorizer(newAuthorizer)` or `super.setAuthorizer(newAuthorizer)` to | |
| /// maintain the ERC165 compatibility check and event emission. | |
| /// `@param` newAuthorizer The new authorizer contract. | |
| function setAuthorizer(IAuthorizeV1 newAuthorizer) external virtual onlyOwner { | |
| _setAuthorizer(newAuthorizer); | |
| } |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@src/concrete/vault/OffchainAssetReceiptVault.sol` around lines 375 - 377, Add
NatSpec documentation to the setAuthorizer(IAuthorizeV1) function explaining
that it is declared virtual to allow derived contracts to extend or customize
authorizer setup, and state explicit override expectations: any override must
preserve the onlyOwner access control (or re-enforce equivalent checks), must
maintain core invariants enforced by _setAuthorizer, and should call
super.setAuthorizer(newAuthorizer) or otherwise invoke
_setAuthorizer(newAuthorizer) to ensure base behavior is executed; reference the
_setAuthorizer internal function and the IAuthorizeV1 parameter so implementers
know what to call and what to preserve.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Extension hook for downstream vaults that need pairing-time invariants on the authorizer (e.g. verifying role-admin hierarchies for application-specific roles before accepting the install).
Summary by CodeRabbit
Note: This release includes technical infrastructure updates with no direct impact on end-user functionality or behavior.