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.
feat: introduce CONTROLLER_CONFIG_OPERATION and enhance GuardControll…
Contributor
📝 WalkthroughWalkthroughAdds a new operation type Changes
Estimated Code Review Effort🎯 3 (Moderate) | ⏱️ ~25 minutes Possibly Related PRs
Poem
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 inconclusive)
✅ Passed checks (2 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
🧹 Nitpick comments (4)
contracts/core/security/SecureOwnable.sol (1)
326-348: Prefer custom errors over barerevert()for unknown operation types.Lines 332 and 346 use bare
revert()for unrecognized operation types. Per coding guidelines, custom errors are preferred for gas efficiency and clarity.♻️ Suggested fix
function _requireNoPendingRequest(bytes32 requestOperationType) internal view { if (requestOperationType == SecureOwnableDefinitions.OWNERSHIP_TRANSFER) { if (_hasOpenOwnershipRequest) revert SharedValidation.PendingSecureRequest(); } else if (requestOperationType == SecureOwnableDefinitions.BROADCASTER_UPDATE) { if (_hasOpenBroadcasterRequest) revert SharedValidation.PendingSecureRequest(); } else { - revert(); + revert SharedValidation.NotSupported(); } } function _clearPendingFlagForOperation(bytes32 operationType) private { if (operationType == SecureOwnableDefinitions.OWNERSHIP_TRANSFER) { _hasOpenOwnershipRequest = false; } 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 326 - 348, Replace the bare revert() calls in _requireNoPendingRequest and _clearPendingFlagForOperation with a custom error that includes the unknown operationType for clarity and gas efficiency; define a new error like UnknownSecureOperation(bytes32 operationType) (or reuse an existing SharedValidation error if available) and throw it with the operationType when neither SecureOwnableDefinitions.OWNERSHIP_TRANSFER nor SecureOwnableDefinitions.BROADCASTER_UPDATE matches, updating both functions to revert with that custom error instead of calling revert().contracts/core/lib/EngineBlox.sol (1)
379-387: Extract duplicated payment request validation into one helper.The new checks are good, but duplicating them in two paths increases drift risk.
♻️ Suggested DRY refactor
function txRequestWithPayment( SecureOperationState storage self, @@ - if (paymentDetails.nativeTokenAmount > 0 || paymentDetails.erc20TokenAmount > 0) { - SharedValidation.validateNotZeroAddress(paymentDetails.recipient); - } - if (paymentDetails.erc20TokenAmount > 0) { - SharedValidation.validateNotZeroAddress(paymentDetails.erc20TokenAddress); - } + _validatePaymentDetailsForRequest(paymentDetails); @@ function requestAndApprove( SecureOperationState storage self, MetaTransaction memory metaTx ) public returns (TxRecord memory) { @@ - if (metaTx.txRecord.payment.nativeTokenAmount > 0 || metaTx.txRecord.payment.erc20TokenAmount > 0) { - SharedValidation.validateNotZeroAddress(metaTx.txRecord.payment.recipient); - } - if (metaTx.txRecord.payment.erc20TokenAmount > 0) { - SharedValidation.validateNotZeroAddress(metaTx.txRecord.payment.erc20TokenAddress); - } + _validatePaymentDetailsForRequest(metaTx.txRecord.payment); @@ + function _validatePaymentDetailsForRequest(PaymentDetails memory payment) private pure { + if (payment.nativeTokenAmount > 0 || payment.erc20TokenAmount > 0) { + SharedValidation.validateNotZeroAddress(payment.recipient); + } + if (payment.erc20TokenAmount > 0) { + SharedValidation.validateNotZeroAddress(payment.erc20TokenAddress); + } + }Also applies to: 587-595
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@contracts/core/lib/EngineBlox.sol` around lines 379 - 387, Extract the duplicated payment validation into a single helper (e.g., validatePaymentDetails or validateAttachedPaymentDetails) and call it from both locations where the current checks appear; move the logic that checks paymentDetails.nativeTokenAmount, paymentDetails.erc20TokenAmount and calls SharedValidation.validateNotZeroAddress(paymentDetails.recipient) and SharedValidation.validateNotZeroAddress(paymentDetails.erc20TokenAddress) into that helper so the two code paths simply call validatePaymentDetails(paymentDetails) instead of repeating the conditions and validateNotZeroAddress calls.test/foundry/helpers/AccountPatternTest.sol (1)
12-20: Add function-level NatSpec for the initializer override.Behavior is fine, but this new Solidity 0.8.34 helper should include canonical machine-readable function docs.
📝 Suggested NatSpec addition
contract AccountPatternTest is Account { + /** + * `@notice` Initializes the account test fixture. + * `@param` initialOwner Owner wallet assigned during initialization. + * `@param` broadcaster Broadcaster wallet assigned during initialization. + * `@param` recovery Recovery wallet assigned during initialization. + * `@param` timeLockPeriodSec Timelock duration in seconds. + * `@param` eventForwarder Optional event forwarder contract address. + */ function initialize( address initialOwner, address broadcaster, address recovery, uint256 timeLockPeriodSec, address eventForwarder ) public virtual override initializer { Account.initialize(initialOwner, broadcaster, recovery, timeLockPeriodSec, eventForwarder); } }As per coding guidelines: In Solidity 0.8.34 contracts, maintain or improve NatSpec documentation as the canonical machine-readable spec.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@test/foundry/helpers/AccountPatternTest.sol` around lines 12 - 20, Add a function-level NatSpec block above the initialize(...) override: include a brief `@dev` describing that this is the test helper initializer which delegates to Account.initialize, add `@param` tags for initialOwner, broadcaster, recovery, timeLockPeriodSec, and eventForwarder describing their purpose/expected values, and include `@inheritdoc` Account.initialize (or an `@notice` if you prefer explicit docs) so the override is machine-readable and follows the Solidity 0.8.34 canonical NatSpec style.test/foundry/fuzz/ComprehensivePaymentSecurityFuzz.t.sol (1)
534-534: Unused parameter_timeAdvance.The parameter was renamed to
_timeAdvance(with underscore prefix indicating intentional non-use), but the variable is never used in the function body. The test function description mentions "Payment Update Timing" but the fuzzed time advance value isn't applied anywhere.Consider either:
- Using the parameter to vary timing scenarios (e.g.,
advanceTime(bound(_timeAdvance, timeLockPeriod + 1, timeLockPeriod * 2)))- Removing the parameter entirely if timing variation isn't needed for this test
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@test/foundry/fuzz/ComprehensivePaymentSecurityFuzz.t.sol` at line 534, The parameter `_timeAdvance` in the fuzz test signature is unused; either remove it or apply it to vary the timing as intended. If you want timing variation, use the fuzzed value (e.g., call your test helper to advance block timestamp using a bounded version of `_timeAdvance` such as bound(_timeAdvance, timeLockPeriod + 1, timeLockPeriod * 2) and then advanceTime/warp before performing the payment update assertions), or simply delete `_timeAdvance` from the function signature and test description if timing variation is not required; update references in the test name/comments accordingly.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@contracts/core/execution/lib/definitions/GuardControllerDefinitions.sol`:
- Line 19: Update the NatSpec comment that currently says "Registers all 7
GuardController public execution functions" to match the actual number of
schemas returned by getFunctionSchemas() (which is 9); locate the
contract/contract comment referencing GuardController and modify the sentence to
read the correct count (e.g., "Registers all 9 GuardController public execution
functions") so the documentation and the getFunctionSchemas() implementation are
consistent.
---
Nitpick comments:
In `@contracts/core/lib/EngineBlox.sol`:
- Around line 379-387: Extract the duplicated payment validation into a single
helper (e.g., validatePaymentDetails or validateAttachedPaymentDetails) and call
it from both locations where the current checks appear; move the logic that
checks paymentDetails.nativeTokenAmount, paymentDetails.erc20TokenAmount and
calls SharedValidation.validateNotZeroAddress(paymentDetails.recipient) and
SharedValidation.validateNotZeroAddress(paymentDetails.erc20TokenAddress) into
that helper so the two code paths simply call
validatePaymentDetails(paymentDetails) instead of repeating the conditions and
validateNotZeroAddress calls.
In `@contracts/core/security/SecureOwnable.sol`:
- Around line 326-348: Replace the bare revert() calls in
_requireNoPendingRequest and _clearPendingFlagForOperation with a custom error
that includes the unknown operationType for clarity and gas efficiency; define a
new error like UnknownSecureOperation(bytes32 operationType) (or reuse an
existing SharedValidation error if available) and throw it with the
operationType when neither SecureOwnableDefinitions.OWNERSHIP_TRANSFER nor
SecureOwnableDefinitions.BROADCASTER_UPDATE matches, updating both functions to
revert with that custom error instead of calling revert().
In `@test/foundry/fuzz/ComprehensivePaymentSecurityFuzz.t.sol`:
- Line 534: The parameter `_timeAdvance` in the fuzz test signature is unused;
either remove it or apply it to vary the timing as intended. If you want timing
variation, use the fuzzed value (e.g., call your test helper to advance block
timestamp using a bounded version of `_timeAdvance` such as bound(_timeAdvance,
timeLockPeriod + 1, timeLockPeriod * 2) and then advanceTime/warp before
performing the payment update assertions), or simply delete `_timeAdvance` from
the function signature and test description if timing variation is not required;
update references in the test name/comments accordingly.
In `@test/foundry/helpers/AccountPatternTest.sol`:
- Around line 12-20: Add a function-level NatSpec block above the
initialize(...) override: include a brief `@dev` describing that this is the test
helper initializer which delegates to Account.initialize, add `@param` tags for
initialOwner, broadcaster, recovery, timeLockPeriodSec, and eventForwarder
describing their purpose/expected values, and include `@inheritdoc`
Account.initialize (or an `@notice` if you prefer explicit docs) so the override
is machine-readable and follows the Solidity 0.8.34 canonical NatSpec style.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro
Run ID: 49a3d153-a773-4012-a4c4-e6f12784eece
📒 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/ComprehensiveWhitelistSchemaFuzz.t.sol
- test/foundry/fuzz/ComprehensiveHookSystemFuzz.t.sol
…execution functions This commit updates the documentation in GuardControllerDefinitions to indicate the registration of 9 public execution functions instead of 7. This change enhances clarity regarding the capabilities of the GuardController and ensures consistency with the current implementation.
This commit updates the version number to 1.0.0-alpha.19 in the package.json files for the main project, the @bloxchain/contracts package, and the @bloxchain/sdk package. This version bump reflects ongoing development and prepares for upcoming features and improvements.
Contributor
There was a problem hiding this comment.
🧹 Nitpick comments (1)
contracts/core/execution/lib/definitions/GuardControllerDefinitions.sol (1)
257-269: Schema 8 registered without default OWNER_ROLE permission—verify this is intentional design.Schema 8 for
executeWithPaymentis registered and protected, but has no correspondingOWNER_ROLEpermission entry ingetRolePermissions(). The comment indicates permissions must be added manually to enable this flow. This means the function will be blocked until explicitly granted, making the payment flow opt-in by default.Clarify in the NatSpec for
getRolePermissions()thatexecuteWithPaymentrequires manual permission setup, or document the rationale for this opt-in design in the schema comment.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@contracts/core/execution/lib/definitions/GuardControllerDefinitions.sol` around lines 257 - 269, Schema 8 registers executeWithPayment (EXECUTE_WITH_PAYMENT_SELECTOR) as protected but lacks a default OWNER_ROLE grant, making the payment flow opt-in; update the NatSpec for getRolePermissions() to explicitly state that the executeWithPayment schema requires a manual OWNER_ROLE permission grant (or, if intentional alternative, document the rationale for leaving it opt-in) and mention the schema identifier (schemas[8]) and handler mapping (executeWithPaymentHandlerForSelectors) so callers know to add the OWNER_ROLE entry when enabling this flow.
🤖 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/execution/lib/definitions/GuardControllerDefinitions.sol`:
- Around line 257-269: Schema 8 registers executeWithPayment
(EXECUTE_WITH_PAYMENT_SELECTOR) as protected but lacks a default OWNER_ROLE
grant, making the payment flow opt-in; update the NatSpec for
getRolePermissions() to explicitly state that the executeWithPayment schema
requires a manual OWNER_ROLE permission grant (or, if intentional alternative,
document the rationale for leaving it opt-in) and mention the schema identifier
(schemas[8]) and handler mapping (executeWithPaymentHandlerForSelectors) so
callers know to add the OWNER_ROLE entry when enabling this flow.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro
Run ID: 41bc9d33-f647-464d-bd7e-78202f8b5172
📒 Files selected for processing (4)
contracts/core/execution/lib/definitions/GuardControllerDefinitions.solpackage.jsonpackage/package.jsonsdk/typescript/package.json
✅ Files skipped from review due to trivial changes (3)
- package.json
- package/package.json
- sdk/typescript/package.json
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.
Summary by CodeRabbit
New Features
Improvements
Tests & Docs