Conversation
…er functionality This commit adds a new operation type, CONTROLLER_CONFIG_OPERATION, to the GuardControllerDefinitions, allowing for distinct handling of guard configuration batch operations. It updates the relevant function schemas and modifies the associated logic in the GuardController to support this new operation type. Additionally, it refines the documentation to clarify the differentiation between CONTROLLER_OPERATION and CONTROLLER_CONFIG_OPERATION, improving the overall structure and usability of the GuardController's execution functions.
Contributor
📝 WalkthroughWalkthroughThis PR introduces a new Changes
Estimated code review effort🎯 4 (Complex) | ⏱️ ~55 minutes Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 3✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 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 Tip You can make CodeRabbit's review stricter and more nitpicky using the `assertive` profile, if that's what you prefer.Change the |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
🧹 Nitpick comments (4)
contracts/core/security/SecureOwnable.sol (2)
326-334: Consider using a custom error instead of barerevert().The bare
revert()at lines 332-333 provides no diagnostic information. While this path should only be hit by internal misuse (passing an unsupportedrequestOperationType), using a custom error likeSharedValidation.NotSupported()would improve debuggability.🔧 Optional: Use custom error for clarity
} else if (requestOperationType == SecureOwnableDefinitions.BROADCASTER_UPDATE) { if (_hasOpenBroadcasterRequest) revert SharedValidation.PendingSecureRequest(); } else { - revert(); + revert SharedValidation.NotSupported(); }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@contracts/core/security/SecureOwnable.sol` around lines 326 - 334, In _requireNoPendingRequest(bytes32 requestOperationType) replace the bare revert() in the final else branch with a descriptive custom error to aid debugging — e.g. revert SharedValidation.NotSupported() (or add SharedValidation.NotSupported() if it doesn't exist) so that invalid requestOperationType paths in _requireNoPendingRequest are clearly reported instead of silently reverting.
340-348: Same suggestion: use custom error in_clearPendingFlagForOperation.The bare
revert()at line 346-347 should also use a custom error for consistency and debuggability.🔧 Optional: Use custom error
} else if (operationType == SecureOwnableDefinitions.BROADCASTER_UPDATE) { _hasOpenBroadcasterRequest = false; } else { - revert(); + revert SharedValidation.NotSupported(); }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@contracts/core/security/SecureOwnable.sol` around lines 340 - 348, Replace the bare revert() in _clearPendingFlagForOperation with a custom error to match other error handling: define (or reuse) a custom error such as InvalidOperationType(bytes32 operationType) and throw it with the incoming operationType in the final else branch of _clearPendingFlagForOperation; reference SecureOwnableDefinitions and the _clearPendingFlagForOperation function when making the change so logs/stack traces include the failing operation value.test/foundry/fuzz/ComprehensiveInitializationFuzz.t.sol (1)
282-289: Use the explicitinitialized()flag in_isInitialized.
AccountPatternTestalready exposes the authoritative initialization state, so probingowner()here couples the helper to accessor revert behavior instead of the actual initialization flag.♻️ Suggested cleanup
function _isInitialized(AccountPatternTest contractInstance) internal view returns (bool) { - // Try to get owner - if initialized, this will return an address - // If not initialized, it will revert - try contractInstance.owner() returns (address) { - return true; - } catch { - return false; - } + return contractInstance.initialized(); }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@test/foundry/fuzz/ComprehensiveInitializationFuzz.t.sol` around lines 282 - 289, Replace the try/catch probing of contractInstance.owner() in the helper function _isInitialized with a direct call to the contract's authoritative initialization flag by calling AccountPatternTest.initialized() (or the exact boolean accessor exposed, e.g., initialized()). Update _isInitialized to return the boolean from contractInstance.initialized() instead of catching owner() reverts so it relies on the explicit initialization state.test/foundry/fuzz/ComprehensivePaymentSecurityFuzz.t.sol (1)
530-535: Use or remove_timeAdvanceto avoid no-op fuzz permutations.
_timeAdvanceis currently unused, so it increases fuzz input space without increasing behavioral coverage in this test.♻️ Minimal cleanup diff
function testFuzz_PaymentUpdateTiming( address originalRecipient, address newRecipient, - uint256 paymentAmount, - uint256 _timeAdvance + uint256 paymentAmount ) public {🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@test/foundry/fuzz/ComprehensivePaymentSecurityFuzz.t.sol` around lines 530 - 535, The fuzz parameter _timeAdvance in testFuzz_PaymentUpdateTiming is unused, bloating the fuzz space—either remove the parameter from the function signature or consume it to advance time; to fix, update the testFuzz_PaymentUpdateTiming signature to drop _timeAdvance, or use _timeAdvance (e.g., normalize it to a safe range and call the test harness time-travel helper such as vm.warp or the project’s equivalent) before exercising payment update logic so the fuzz input affects behavior.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@contracts/core/security/SecureOwnable.sol`:
- Around line 326-334: In _requireNoPendingRequest(bytes32 requestOperationType)
replace the bare revert() in the final else branch with a descriptive custom
error to aid debugging — e.g. revert SharedValidation.NotSupported() (or add
SharedValidation.NotSupported() if it doesn't exist) so that invalid
requestOperationType paths in _requireNoPendingRequest are clearly reported
instead of silently reverting.
- Around line 340-348: Replace the bare revert() in
_clearPendingFlagForOperation with a custom error to match other error handling:
define (or reuse) a custom error such as InvalidOperationType(bytes32
operationType) and throw it with the incoming operationType in the final else
branch of _clearPendingFlagForOperation; reference SecureOwnableDefinitions and
the _clearPendingFlagForOperation function when making the change so logs/stack
traces include the failing operation value.
In `@test/foundry/fuzz/ComprehensiveInitializationFuzz.t.sol`:
- Around line 282-289: Replace the try/catch probing of contractInstance.owner()
in the helper function _isInitialized with a direct call to the contract's
authoritative initialization flag by calling AccountPatternTest.initialized()
(or the exact boolean accessor exposed, e.g., initialized()). Update
_isInitialized to return the boolean from contractInstance.initialized() instead
of catching owner() reverts so it relies on the explicit initialization state.
In `@test/foundry/fuzz/ComprehensivePaymentSecurityFuzz.t.sol`:
- Around line 530-535: The fuzz parameter _timeAdvance in
testFuzz_PaymentUpdateTiming is unused, bloating the fuzz space—either remove
the parameter from the function signature or consume it to advance time; to fix,
update the testFuzz_PaymentUpdateTiming signature to drop _timeAdvance, or use
_timeAdvance (e.g., normalize it to a safe range and call the test harness
time-travel helper such as vm.warp or the project’s equivalent) before
exercising payment update logic so the fuzz input affects behavior.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro
Run ID: fc8ac5a4-7d06-4729-9fdf-ca266710ba2d
📒 Files selected for processing (27)
abi/GuardControllerDefinitions.abi.jsoncontracts/core/access/RuntimeRBAC.solcontracts/core/execution/lib/definitions/GuardControllerDefinitions.solcontracts/core/lib/EngineBlox.solcontracts/core/pattern/Account.solcontracts/core/security/SecureOwnable.soldocs/core/execution/lib/definitions/GuardControllerDefinitions.mdscripts/sanity-sdk/guard-controller/base-test.tsscripts/sanity/guard-controller/base-test.cjssdk/typescript/abi/GuardControllerDefinitions.abi.jsonsdk/typescript/docs/guard-controller.mdsdk/typescript/types/core.execution.index.tsxtest/foundry/CommonBase.soltest/foundry/fuzz/ComprehensiveCompositeFuzz.t.soltest/foundry/fuzz/ComprehensiveDefinitionSecurityFuzz.t.soltest/foundry/fuzz/ComprehensiveEventForwardingFuzz.t.soltest/foundry/fuzz/ComprehensiveGasExhaustionFuzz.t.soltest/foundry/fuzz/ComprehensiveHookSystemFuzz.t.soltest/foundry/fuzz/ComprehensiveInitializationFuzz.t.soltest/foundry/fuzz/ComprehensivePaymentSecurityFuzz.t.soltest/foundry/fuzz/ComprehensiveStateMachineFuzz.t.soltest/foundry/fuzz/ComprehensiveWhitelistSchemaFuzz.t.soltest/foundry/fuzz/SecureOwnableFuzz.t.soltest/foundry/helpers/AccountPatternTest.soltest/foundry/security/EdgeCases.t.soltest/foundry/unit/BaseStateMachine.t.soltest/foundry/unit/SecureOwnable.t.sol
💤 Files with no reviewable changes (2)
- test/foundry/fuzz/ComprehensiveHookSystemFuzz.t.sol
- test/foundry/fuzz/ComprehensiveWhitelistSchemaFuzz.t.sol
Merged
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.
…er functionality
This commit adds a new operation type, CONTROLLER_CONFIG_OPERATION, to the GuardControllerDefinitions, allowing for distinct handling of guard configuration batch operations. It updates the relevant function schemas and modifies the associated logic in the GuardController to support this new operation type. Additionally, it refines the documentation to clarify the differentiation between CONTROLLER_OPERATION and CONTROLLER_CONFIG_OPERATION, improving the overall structure and usability of the GuardController's execution functions.
Summary by CodeRabbit
Release Notes
New Features
CONTROLLER_CONFIG_OPERATIONfor guard configuration batch actions, distinct from guard execution operations.Tests
Documentation