Conversation
…guidance This commit updates the TypeScript SDK documentation to include the new `FUNCTION_SELECTORS` export and clarifies the initialization process for account-style contracts using OpenZeppelin's **Initializable** semantics. It emphasizes the importance of treating initialization as part of deployment to avoid uninitialized proxies and outlines best practices for atomic proxy creation and initialization. Additionally, it enhances the documentation for the `RuntimeRBAC` integration, detailing the handling of function permissions and the implications of protected schemas. These changes aim to improve clarity and usability for developers working with the SDK and related contracts.
This commit updates the GuardControllerDefinitions to rename the constant `CONTROLLER_CONFIG_OPERATION` to `CONTROLLER_CONFIG_BATCH` for improved clarity and consistency. The change is reflected across the ABI files, Solidity definitions, and TypeScript SDK, ensuring that all references are updated accordingly. Additionally, documentation has been modified to reflect this new naming convention, enhancing the understanding of operation types within the GuardController context. These changes aim to streamline the codebase and improve developer experience.
Contributor
📝 WalkthroughWalkthroughThe PR renames the guard controller operation type constant from Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~22 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 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 |
![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 (1)
test/foundry/helpers/PaymentTestHelper.sol (1)
414-419: Use the repo-standard custom errors/helpers in this new test helper.The new helper reintroduces string-based
requirereverts. Reusing the existingSharedValidation/ custom-error path here would keep revert decoding and test expectations consistent with the rest of the state machine.As per coding guidelines, "Prefer custom errors over string-based errors in Solidity; use existing custom errors from SharedValidation and related libraries"
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@test/foundry/helpers/PaymentTestHelper.sol` around lines 414 - 419, Replace the string-based require checks in removeTargetFromWhitelistForTesting with the repo-standard SharedValidation/custom-error helpers: remove the require(msg.sender == owner(), ...) and instead call the SharedValidation owner-check helper (e.g., SharedValidation.validateOwner or equivalent helper that verifies msg.sender is owner()), replace require(selector != bytes4(0), ...) with the SharedValidation helper that validates non-zero selectors (e.g., SharedValidation.validateNotZeroSelector(selector)), and keep SharedValidation.validateNotZeroAddress(target) and the EngineBlox.removeTargetFromWhitelist call unchanged so the function uses custom errors consistent with the rest of the codebase.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@sdk/typescript/types/core.execution.index.tsx`:
- Around line 7-11: GUARD_CONTROLLER_OPERATION_TYPES has renamed the key
CONTROLLER_CONFIG_OPERATION to CONTROLLER_CONFIG_BATCH which breaks existing
TypeScript users; add back the old key as a deprecated alias that points to the
same keccak256 value (i.e., set CONTROLLER_CONFIG_OPERATION:
GUARD_CONTROLLER_OPERATION_TYPES.CONTROLLER_CONFIG_BATCH or the same keccak256
call) so existing code still works, and annotate the alias with a JSDoc
`@deprecated` comment so consumers see the migration notice; update references in
the module to use CONTROLLER_CONFIG_BATCH going forward.
---
Nitpick comments:
In `@test/foundry/helpers/PaymentTestHelper.sol`:
- Around line 414-419: Replace the string-based require checks in
removeTargetFromWhitelistForTesting with the repo-standard
SharedValidation/custom-error helpers: remove the require(msg.sender == owner(),
...) and instead call the SharedValidation owner-check helper (e.g.,
SharedValidation.validateOwner or equivalent helper that verifies msg.sender is
owner()), replace require(selector != bytes4(0), ...) with the SharedValidation
helper that validates non-zero selectors (e.g.,
SharedValidation.validateNotZeroSelector(selector)), and keep
SharedValidation.validateNotZeroAddress(target) and the
EngineBlox.removeTargetFromWhitelist call unchanged so the function uses custom
errors consistent with the rest of the codebase.
🪄 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: Repository UI
Review profile: CHILL
Plan: Pro
Run ID: 7c120162-d8b1-4515-8a37-01f72804550d
📒 Files selected for processing (19)
abi/GuardControllerDefinitions.abi.jsoncontracts/core/execution/lib/definitions/GuardControllerDefinitions.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/getting-started.mdsdk/typescript/docs/guard-controller.mdsdk/typescript/docs/runtime-rbac.mdsdk/typescript/docs/secure-ownable.mdsdk/typescript/docs/state-machine-engine.mdsdk/typescript/index.tsxsdk/typescript/types/core.execution.index.tsxsdk/typescript/utils/contract-errors.tstest/foundry/fuzz/AuditDerivedAttackVectorsFuzz.t.soltest/foundry/fuzz/ComprehensiveCompositeFuzz.t.soltest/foundry/fuzz/ComprehensiveGasExhaustionFuzz.t.soltest/foundry/fuzz/ComprehensiveStateMachineFuzz.t.soltest/foundry/helpers/PaymentTestHelper.sol
Comment on lines
7
to
11
| export const GUARD_CONTROLLER_OPERATION_TYPES = { | ||
| CONTROLLER_OPERATION: keccak256(new TextEncoder().encode("CONTROLLER_OPERATION")), | ||
| CONTROLLER_CONFIG_OPERATION: keccak256(new TextEncoder().encode("CONTROLLER_CONFIG_OPERATION")), | ||
| CONTROLLER_CONFIG_BATCH: keccak256(new TextEncoder().encode("CONTROLLER_CONFIG_BATCH")), | ||
| NATIVE_TRANSFER: keccak256(new TextEncoder().encode("NATIVE_TRANSFER")) | ||
| } as const; |
Contributor
There was a problem hiding this comment.
Keep a deprecated alias for the old SDK constant name.
Replacing CONTROLLER_CONFIG_OPERATION with CONTROLLER_CONFIG_BATCH is a source-breaking change for existing TypeScript integrations that read the old property. Unless this is intentionally a breaking SDK release, keep the old key as a deprecated alias for at least one cycle or call out the migration explicitly.
💡 Backward-compatible option
export const GUARD_CONTROLLER_OPERATION_TYPES = {
CONTROLLER_OPERATION: keccak256(new TextEncoder().encode("CONTROLLER_OPERATION")),
+ /** `@deprecated` Use CONTROLLER_CONFIG_BATCH */
+ CONTROLLER_CONFIG_OPERATION: keccak256(new TextEncoder().encode("CONTROLLER_CONFIG_BATCH")),
CONTROLLER_CONFIG_BATCH: keccak256(new TextEncoder().encode("CONTROLLER_CONFIG_BATCH")),
NATIVE_TRANSFER: keccak256(new TextEncoder().encode("NATIVE_TRANSFER"))
} as const;📝 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
| export const GUARD_CONTROLLER_OPERATION_TYPES = { | |
| CONTROLLER_OPERATION: keccak256(new TextEncoder().encode("CONTROLLER_OPERATION")), | |
| CONTROLLER_CONFIG_OPERATION: keccak256(new TextEncoder().encode("CONTROLLER_CONFIG_OPERATION")), | |
| CONTROLLER_CONFIG_BATCH: keccak256(new TextEncoder().encode("CONTROLLER_CONFIG_BATCH")), | |
| NATIVE_TRANSFER: keccak256(new TextEncoder().encode("NATIVE_TRANSFER")) | |
| } as const; | |
| export const GUARD_CONTROLLER_OPERATION_TYPES = { | |
| CONTROLLER_OPERATION: keccak256(new TextEncoder().encode("CONTROLLER_OPERATION")), | |
| /** `@deprecated` Use CONTROLLER_CONFIG_BATCH */ | |
| CONTROLLER_CONFIG_OPERATION: keccak256(new TextEncoder().encode("CONTROLLER_CONFIG_BATCH")), | |
| CONTROLLER_CONFIG_BATCH: keccak256(new TextEncoder().encode("CONTROLLER_CONFIG_BATCH")), | |
| NATIVE_TRANSFER: keccak256(new TextEncoder().encode("NATIVE_TRANSFER")) | |
| } as const; |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@sdk/typescript/types/core.execution.index.tsx` around lines 7 - 11,
GUARD_CONTROLLER_OPERATION_TYPES has renamed the key CONTROLLER_CONFIG_OPERATION
to CONTROLLER_CONFIG_BATCH which breaks existing TypeScript users; add back the
old key as a deprecated alias that points to the same keccak256 value (i.e., set
CONTROLLER_CONFIG_OPERATION:
GUARD_CONTROLLER_OPERATION_TYPES.CONTROLLER_CONFIG_BATCH or the same keccak256
call) so existing code still works, and annotate the alias with a JSDoc
`@deprecated` comment so consumers see the migration notice; update references in
the module to use CONTROLLER_CONFIG_BATCH going forward.
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
Release Notes
New Features
Documentation
TxRecorddocumentation to clarify result preview truncation behavior.TransactionEventfield naming and filtering guidance.Tests