Conversation
…dress pair logging This commit refactors the SecureOwnable contract by replacing instances of _logComponentEvent with a new internal function _logAddressPairEvent. This change reduces contract size and improves clarity by standardizing the logging of address pairs. The BaseStateMachine contract also removes unnecessary logging calls, further streamlining the code.
This commit refactors the RuntimeRBAC and GuardController contracts by consolidating role management actions into dedicated internal functions. The changes include the introduction of _executeCreateRole, _executeRemoveRole, _executeAddWallet, _executeRevokeWallet, _executeAddFunctionToRole, and _executeRemoveFunctionFromRole methods, which enhance code clarity and maintainability. Additionally, the initialization logic in BaseStateMachine and SecureOwnable has been simplified to ensure consistent behavior across components.
This commit modifies the sanity test scripts and configuration files to standardize the use of the AccountBlox contract. The ACCOUNTBLOX_ADDRESS environment variable is now required for all sanity tests, replacing previous references to SecureBlox and RoleBlox. Additionally, the truffle-config has been updated to accept dynamic network IDs, enhancing compatibility with various development environments. Documentation has also been updated to reflect these changes.
This commit updates the version numbers in package.json and related files to 1.0.0-alpha.12, reflecting the latest changes in the project. The release scripts have also been adjusted to ensure proper version synchronization for publishing, enhancing the overall release process.
Contributor
|
Warning Rate limit exceeded
⌛ How to resolve this issue?After the wait time has elapsed, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans have higher rate limits than the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout. Please see our FAQ for further information. 📝 WalkthroughWalkthroughUnifies batch-action handling into internal executor functions and centralized logging across core contracts, makes base-state initialization unconditional in some initializers, consolidates sanity tests to use a single AccountBlox contract (ACCOUNTBLOX_ADDRESS), hardens tests and tx/status handling, and bumps package versions to 1.0.0-alpha.12. Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 3 | ❌ 1❌ Failed checks (1 inconclusive)
✅ Passed checks (3 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: 4
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
contracts/core/base/BaseStateMachine.sol (1)
298-310:⚠️ Potential issue | 🔴 CriticalAdd event emissions for hook configuration changes.
Hook additions and removals in
_setHook/_clearHook(lines 298-310) do not emit events, making these critical state changes invisible to off-chain indexers and monitoring systems. Per the coding guidelines requiring events for all critical state changes, the functions should emitHookSetandHookClearedevents (or similar). This aligns with the event-logging pattern used elsewhere in the codebase for state modifications (e.g.,_logComponentEventin GuardController, RuntimeRBAC, and SecureOwnable) and ensures hook configuration changes are observable off-chain.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@contracts/core/base/BaseStateMachine.sol` around lines 298 - 310, The _setHook and _clearHook functions call EngineBlox.addTargetToFunctionHooks and EngineBlox.removeTargetFromFunctionHooks but do not emit events, so add event declarations (e.g., event HookSet(bytes4 indexed functionSelector, address indexed hook, address indexed setter) and event HookCleared(bytes4 indexed functionSelector, address indexed hook, address indexed clearer)) to the contract and emit HookSet(functionSelector, hook, msg.sender) at the end of _setHook and emit HookCleared(functionSelector, hook, msg.sender) at the end of _clearHook so off-chain indexers can observe hook configuration changes.
🤖 Fix all issues with AI agents
Verify each finding against the current code and only fix it if needed.
In `@contracts/core/access/RuntimeRBAC.sol`:
- Around line 158-174: The ADD_WALLET/REVOKE_WALLET handlers (_executeAddWallet
and _executeRevokeWallet) currently call _logRoleConfigEvent with bytes4(0),
omitting the wallet address; update logging so the wallet is emitted: either
extend _logRoleConfigEvent to accept an extra address parameter and pass the
decoded wallet, or add a new helper (e.g., _logRoleConfigWalletEvent) that emits
(actionType, roleHash, wallet) and call it from
_executeAddWallet/_executeRevokeWallet for
IRuntimeRBAC.RoleConfigActionType.ADD_WALLET and REVOKE_WALLET respectively, and
update any event signatures/NatSpec accordingly.
In `@contracts/core/base/BaseStateMachine.sol`:
- Around line 298-310: The _setHook and _clearHook functions call
EngineBlox.addTargetToFunctionHooks and EngineBlox.removeTargetFromFunctionHooks
but do not emit events, so add event declarations (e.g., event HookSet(bytes4
indexed functionSelector, address indexed hook, address indexed setter) and
event HookCleared(bytes4 indexed functionSelector, address indexed hook, address
indexed clearer)) to the contract and emit HookSet(functionSelector, hook,
msg.sender) at the end of _setHook and emit HookCleared(functionSelector, hook,
msg.sender) at the end of _clearHook so off-chain indexers can observe hook
configuration changes.
In `@contracts/core/execution/GuardController.sol`:
- Around line 358-362: The _executeRemoveTargetFromWhitelist function currently
omits zero-address validation; update it to assert target != address(0) before
calling _removeTargetFromFunctionWhitelist by invoking the shared validation
helper (e.g., SharedValidation.validateNotZeroAddress(target)) or adding an
equivalent require, then proceed to call
_removeTargetFromFunctionWhitelist(functionSelector, target) and
_logGuardConfigEvent as before; this mirrors the protection used in
_executeAddTargetToWhitelist and prevents removing the zero address.
In `@package.json`:
- Around line 47-48: The two publish scripts (publish:contracts and publish:sdk)
hardcode "--tag alpha.12"; replace that with a tag derived from package.json's
prerelease identifier so it automatically follows version bumps. Update each
script to compute the tag from require('./package.json').version (e.g., extract
the part after the "-" or fallback to "latest") and use that value for npm
publish --tag; you can perform this inline in the script or delegate the value
from existing tasks like release:sync-versions or release:prepare so the publish
commands use the computed tag instead of the hardcoded "alpha.12".
In `@scripts/sanity-sdk/guard-controller/erc20-mint-controller-tests.ts`:
- Around line 344-372: The catch block for requestAndApproveExecution currently
masks real failures by always calling this.assertTest(true); change it to record
soft-failures (e.g., set a boolean/array like this.softFailures or
this.testResults.softFailures and push execError/details) and still allow the
individual test to continue, then at the end of the test run (or test file)
check that collection and fail the suite or call process.exit(1) /
this.assertTest(false, ...) if any soft-failures occurred; update the catch to
include execError metadata (execError?.message) in the recorded entry so CI can
surface actionable failures while keeping the local best-effort behavior during
the test body.
In `@scripts/sanity-sdk/runtime-rbac/base-test.ts`:
- Around line 122-123: The ABI override currently mutates (this.runtimeRBAC as
any).abi which bypasses typing and is not applied in
createRuntimeRBACWithWallet; update the code to consistently supply the
AccountBlox ABI to all RuntimeRBAC instances by either (A) passing
AccountBloxABIJson into the RuntimeRBAC constructor/factory (RuntimeRBAC(...))
if the class supports an abi parameter, or (B) centralizing the override inside
createRuntimeRBACWithWallet so it sets the instance ABI immediately after
creation (avoid using `as any` by using the proper typed property or a setter on
RuntimeRBAC); ensure both the existing this.runtimeRBAC initialization and
createRuntimeRBACWithWallet use the same mechanism so no RuntimeRBAC is left
with the incomplete default ABI.
In `@scripts/sanity-sdk/runtime-rbac/rbac-tests.ts`:
- Around line 217-228: The current error-handling in the create block uses
overly broad substring checks on msg (checking for 'revert' and 'Missing or
invalid') which can mask real failures; update the conditional around msg (and
the surrounding create flow that calls ensureRoleHasRequiredPermissions and uses
registryAdminRoleHash) to match only specific known error signatures (e.g.,
exact "ResourceAlreadyExists" or a more specific revert reason/regex returned by
your contract/library), prefer inspecting structured fields like error.code or
error.name if available, and when falling back to any broader match still emit a
distinct warning-level log that includes the full error and stack before
attempting ensureRoleHasRequiredPermissions so genuine issues aren’t silently
treated as "role exists."
In `@scripts/sanity/secure-ownable/broadcaster-update-tests.cjs`:
- Around line 271-272: Replace the brittle fixed 1500ms sleep with a
retry/polling loop that calls getTransaction until the returned transaction
status is no longer EXECUTING or PROCESSING_PAYMENT (use the same pattern as
createBroadcasterRequestAndDeriveTxId's retry logic), retrying with a short
delay and a max timeout; update the code around the await new
Promise(setTimeout...) to perform repeated getTransaction calls, break when the
status is terminal, and throw or fail if the max wait is exceeded to avoid
hanging tests.
- Around line 277-295: Extract the duplicated status parsing and revert-data
logging into a shared helper named extractAndValidateStatus(tx, txId) that
accepts the transaction object and an identifier string; inside it perform the
flexible status extraction (use tx.status or tx[2]), normalize to a Number
(handling BigNumber-like objects with toNumber), if statusNum === 6 extract
rawResult from tx.result or tx[5], format resultHex (handle string vs
Buffer/array via Buffer.from), log the first 200 chars and the 4-byte selector,
then return statusNum (or throw/assert if you prefer); replace the duplicated
blocks in both places (the block around statusVal/statusNum and the similar
block in testTimeDelayApproval) with calls to extractAndValidateStatus(tx, txId)
and use the returned statusNum in the existing this.assertTest checks so
on-chain failures are consistently logged.
In `@scripts/sanity/secure-ownable/ownership-transfer-tests.cjs`:
- Line 443: The transaction status comparisons are inconsistent: some checks use
string equality (e.g., tx.status === '1' or '4') while others use
Number(tx.status) === 5; convert all status checks to numeric coercion for
consistency by wrapping tx.status with Number() and comparing against numeric
literals (e.g., Number(tx.status) === 1, 4, 5) wherever tx.status is tested
(look for assertions using this.assertTest and any direct tx.status
comparisons), and update the fallback history scan logic that reads tx.status
(the scan around the fallback on line ~728) to use Number(tx.status) as well so
all branches use the same numeric comparison pattern.
- Around line 403-410: The metaTxParams assignment is misindented relative to
its surrounding try block; locate the call to
this.callContractMethod(this.contract.methods.createMetaTxParams(...)) that
assigns to metaTxParams in the test and realign its indentation to match the
rest of the block (same nesting as adjacent statements) so the assignment line
and its wrapped arguments (including createMetaTxParams,
getTxAction('SIGN_META_APPROVE'), and getRoleWalletObject('owner').address) have
consistent indentation with the surrounding try block.
🧹 Nitpick comments (6)
🤖 Fix all nitpicks with AI agents
Verify each finding against the current code and only fix it if needed. In `@package.json`: - Around line 47-48: The two publish scripts (publish:contracts and publish:sdk) hardcode "--tag alpha.12"; replace that with a tag derived from package.json's prerelease identifier so it automatically follows version bumps. Update each script to compute the tag from require('./package.json').version (e.g., extract the part after the "-" or fallback to "latest") and use that value for npm publish --tag; you can perform this inline in the script or delegate the value from existing tasks like release:sync-versions or release:prepare so the publish commands use the computed tag instead of the hardcoded "alpha.12". In `@scripts/sanity-sdk/guard-controller/erc20-mint-controller-tests.ts`: - Around line 344-372: The catch block for requestAndApproveExecution currently masks real failures by always calling this.assertTest(true); change it to record soft-failures (e.g., set a boolean/array like this.softFailures or this.testResults.softFailures and push execError/details) and still allow the individual test to continue, then at the end of the test run (or test file) check that collection and fail the suite or call process.exit(1) / this.assertTest(false, ...) if any soft-failures occurred; update the catch to include execError metadata (execError?.message) in the recorded entry so CI can surface actionable failures while keeping the local best-effort behavior during the test body. In `@scripts/sanity-sdk/runtime-rbac/rbac-tests.ts`: - Around line 217-228: The current error-handling in the create block uses overly broad substring checks on msg (checking for 'revert' and 'Missing or invalid') which can mask real failures; update the conditional around msg (and the surrounding create flow that calls ensureRoleHasRequiredPermissions and uses registryAdminRoleHash) to match only specific known error signatures (e.g., exact "ResourceAlreadyExists" or a more specific revert reason/regex returned by your contract/library), prefer inspecting structured fields like error.code or error.name if available, and when falling back to any broader match still emit a distinct warning-level log that includes the full error and stack before attempting ensureRoleHasRequiredPermissions so genuine issues aren’t silently treated as "role exists." In `@scripts/sanity/secure-ownable/broadcaster-update-tests.cjs`: - Around line 271-272: Replace the brittle fixed 1500ms sleep with a retry/polling loop that calls getTransaction until the returned transaction status is no longer EXECUTING or PROCESSING_PAYMENT (use the same pattern as createBroadcasterRequestAndDeriveTxId's retry logic), retrying with a short delay and a max timeout; update the code around the await new Promise(setTimeout...) to perform repeated getTransaction calls, break when the status is terminal, and throw or fail if the max wait is exceeded to avoid hanging tests. - Around line 277-295: Extract the duplicated status parsing and revert-data logging into a shared helper named extractAndValidateStatus(tx, txId) that accepts the transaction object and an identifier string; inside it perform the flexible status extraction (use tx.status or tx[2]), normalize to a Number (handling BigNumber-like objects with toNumber), if statusNum === 6 extract rawResult from tx.result or tx[5], format resultHex (handle string vs Buffer/array via Buffer.from), log the first 200 chars and the 4-byte selector, then return statusNum (or throw/assert if you prefer); replace the duplicated blocks in both places (the block around statusVal/statusNum and the similar block in testTimeDelayApproval) with calls to extractAndValidateStatus(tx, txId) and use the returned statusNum in the existing this.assertTest checks so on-chain failures are consistently logged. In `@scripts/sanity/secure-ownable/ownership-transfer-tests.cjs`: - Around line 403-410: The metaTxParams assignment is misindented relative to its surrounding try block; locate the call to this.callContractMethod(this.contract.methods.createMetaTxParams(...)) that assigns to metaTxParams in the test and realign its indentation to match the rest of the block (same nesting as adjacent statements) so the assignment line and its wrapped arguments (including createMetaTxParams, getTxAction('SIGN_META_APPROVE'), and getRoleWalletObject('owner').address) have consistent indentation with the surrounding try block.package.json (1)
47-48: Consider deriving the publish tag from the version field to avoid manual updates.The
--tag alpha.12strings are hardcoded and must be manually bumped alongside the version on every release. A small helper (or inline shell expansion) that extracts the prerelease identifier fromversionwould eliminate this synchronization chore and prevent accidental mismatches.For example:
- "publish:contracts": "npm run release:prepare && cd package && npm publish --tag alpha.12", - "publish:sdk": "npm run release:sync-versions && npm run extract-abi && npm run build:sdk && cd sdk/typescript && npm publish --tag alpha.12", + "publish:contracts": "npm run release:prepare && cd package && npm publish --tag $(node -p \"require('./package.json').version.split('-')[1]?.split('.')[0] || 'latest'\")", + "publish:sdk": "npm run release:sync-versions && npm run extract-abi && npm run build:sdk && cd sdk/typescript && npm publish --tag $(node -p \"require('../../package.json').version.split('-')[1]?.split('.')[0] || 'latest'\")",Or delegate to
release:sync-versions/release:prepareto also patch publish tags.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@package.json` around lines 47 - 48, The two publish scripts (publish:contracts and publish:sdk) hardcode "--tag alpha.12"; replace that with a tag derived from package.json's prerelease identifier so it automatically follows version bumps. Update each script to compute the tag from require('./package.json').version (e.g., extract the part after the "-" or fallback to "latest") and use that value for npm publish --tag; you can perform this inline in the script or delegate the value from existing tasks like release:sync-versions or release:prepare so the publish commands use the computed tag instead of the hardcoded "alpha.12".scripts/sanity/secure-ownable/ownership-transfer-tests.cjs (1)
403-410: Indentation is misaligned with the surrounding block.The
metaTxParamsassignment (lines 403-410) drops to a shallower indent compared to the rest of thetryblock. Likely a copy-paste artifact.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@scripts/sanity/secure-ownable/ownership-transfer-tests.cjs` around lines 403 - 410, The metaTxParams assignment is misindented relative to its surrounding try block; locate the call to this.callContractMethod(this.contract.methods.createMetaTxParams(...)) that assigns to metaTxParams in the test and realign its indentation to match the rest of the block (same nesting as adjacent statements) so the assignment line and its wrapped arguments (including createMetaTxParams, getTxAction('SIGN_META_APPROVE'), and getRoleWalletObject('owner').address) have consistent indentation with the surrounding try block.scripts/sanity/secure-ownable/broadcaster-update-tests.cjs (2)
271-272: ArbitrarysetTimeoutdelay is fragile.A fixed 1500ms sleep to "allow state to settle" is unreliable — it may be too short on slow nodes or unnecessarily slow elsewhere. Consider polling
getTransactionin a retry loop (similar to the pattern already used increateBroadcasterRequestAndDeriveTxIdat lines 398–405) until the status transitions from EXECUTING/PROCESSING_PAYMENT to a terminal state.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@scripts/sanity/secure-ownable/broadcaster-update-tests.cjs` around lines 271 - 272, Replace the brittle fixed 1500ms sleep with a retry/polling loop that calls getTransaction until the returned transaction status is no longer EXECUTING or PROCESSING_PAYMENT (use the same pattern as createBroadcasterRequestAndDeriveTxId's retry logic), retrying with a short delay and a max timeout; update the code around the await new Promise(setTimeout...) to perform repeated getTransaction calls, break when the status is terminal, and throw or fail if the max wait is exceeded to avoid hanging tests.
277-295: Duplicated status-extraction logic — extract a helper.The status parsing (lines 277–279) is repeated almost identically at lines 357–359, but the revert-data logging (lines 281–289) is only present here. This inconsistency means
testTimeDelayApprovalsilently swallows on-chain failures without any diagnostic output. Extract a shared method (e.g.,extractAndValidateStatus(tx, txId)) that encapsulates both the flexible status extraction and the revert-data logging.♻️ Sketch of a shared helper
+ /** + * Extract numeric status from a transaction record and log revert data if FAILED. + */ + parseTransactionStatus(tx) { + const statusVal = tx.status !== undefined ? tx.status : tx[2]; + const statusNum = typeof statusVal === 'object' && statusVal != null && typeof statusVal.toNumber === 'function' + ? statusVal.toNumber() : Number(statusVal); + if (statusNum === 6) { + const rawResult = tx.result !== undefined ? tx.result : tx[5]; + if (rawResult) { + const resultHex = typeof rawResult === 'string' ? rawResult : (rawResult.length && typeof rawResult.slice === 'function') ? '0x' + Buffer.from(rawResult).toString('hex') : '0x'; + const errorSelector = resultHex.length >= 10 ? resultHex.slice(0, 10) : resultHex; + console.log(` 📋 Revert data (first 200 chars): ${resultHex.slice(0, 200)}`); + console.log(` 📋 Error selector: ${errorSelector}`); + } + } + return statusNum; + }Then in both test methods:
- const statusVal = tx.status !== undefined ? tx.status : tx[2]; - const statusNum = typeof statusVal === 'object' ... + const statusNum = this.parseTransactionStatus(tx);🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@scripts/sanity/secure-ownable/broadcaster-update-tests.cjs` around lines 277 - 295, Extract the duplicated status parsing and revert-data logging into a shared helper named extractAndValidateStatus(tx, txId) that accepts the transaction object and an identifier string; inside it perform the flexible status extraction (use tx.status or tx[2]), normalize to a Number (handling BigNumber-like objects with toNumber), if statusNum === 6 extract rawResult from tx.result or tx[5], format resultHex (handle string vs Buffer/array via Buffer.from), log the first 200 chars and the 4-byte selector, then return statusNum (or throw/assert if you prefer); replace the duplicated blocks in both places (the block around statusVal/statusNum and the similar block in testTimeDelayApproval) with calls to extractAndValidateStatus(tx, txId) and use the returned statusNum in the existing this.assertTest checks so on-chain failures are consistently logged.scripts/sanity-sdk/runtime-rbac/rbac-tests.ts (1)
217-228: Broad error matching may mask genuine failures.The check on line 219 matches
'revert'and'Missing or invalid', which are very generic substrings. Any unrelated revert error would be swallowed and treated as "role already exists," potentially hiding real issues during test development. Consider tightening to more specific patterns, or at minimum logging a warning when falling through the broader matchers.That said, since this is test-harness recovery code and subsequent steps will fail if assumptions are wrong, the risk is limited.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@scripts/sanity-sdk/runtime-rbac/rbac-tests.ts` around lines 217 - 228, The current error-handling in the create block uses overly broad substring checks on msg (checking for 'revert' and 'Missing or invalid') which can mask real failures; update the conditional around msg (and the surrounding create flow that calls ensureRoleHasRequiredPermissions and uses registryAdminRoleHash) to match only specific known error signatures (e.g., exact "ResourceAlreadyExists" or a more specific revert reason/regex returned by your contract/library), prefer inspecting structured fields like error.code or error.name if available, and when falling back to any broader match still emit a distinct warning-level log that includes the full error and stack before attempting ensureRoleHasRequiredPermissions so genuine issues aren’t silently treated as "role exists."scripts/sanity-sdk/guard-controller/erc20-mint-controller-tests.ts (1)
344-372: Soft-failure on mint execution undermines test signal.When
requestAndApproveExecutionreverts, the test logs a warning but callsthis.assertTest(true, ...), effectively masking a real failure. If the SDK wrapper itself has a bug, this test will never catch it. Consider at minimum tracking soft-failures in a summary or emitting a non-zero exit code when they occur, so CI doesn't silently pass broken flows.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@scripts/sanity-sdk/guard-controller/erc20-mint-controller-tests.ts` around lines 344 - 372, The catch block for requestAndApproveExecution currently masks real failures by always calling this.assertTest(true); change it to record soft-failures (e.g., set a boolean/array like this.softFailures or this.testResults.softFailures and push execError/details) and still allow the individual test to continue, then at the end of the test run (or test file) check that collection and fail the suite or call process.exit(1) / this.assertTest(false, ...) if any soft-failures occurred; update the catch to include execError metadata (execError?.message) in the recorded entry so CI can surface actionable failures while keeping the local best-effort behavior during the test body.
Comment on lines
+122
to
+123
| // Override the ABI to use AccountBlox ABI which includes all RuntimeRBAC + GuardController + SecureOwnable functions | ||
| (this.runtimeRBAC as any).abi = AccountBloxABIJson; |
Contributor
There was a problem hiding this comment.
ABI override via as any is fragile and incomplete.
(this.runtimeRBAC as any).abi bypasses type safety and relies on the SDK class's internal property name. More importantly, createRuntimeRBACWithWallet (line 188-200) does not apply this same override, so any RuntimeRBAC instance created through that helper will use the default (incomplete) ABI, potentially causing silent failures when calling AccountBlox-specific functions.
Consider either:
- Applying the override in
createRuntimeRBACWithWalletas well, or - Passing a custom ABI through the
RuntimeRBACconstructor if the SDK supports it.
#!/bin/bash
# Check if RuntimeRBAC class constructor or factory supports an ABI parameter
ast-grep --pattern 'class RuntimeRBAC {
$$$
constructor($$$) {
$$$
}
$$$
}'🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@scripts/sanity-sdk/runtime-rbac/base-test.ts` around lines 122 - 123, The ABI
override currently mutates (this.runtimeRBAC as any).abi which bypasses typing
and is not applied in createRuntimeRBACWithWallet; update the code to
consistently supply the AccountBlox ABI to all RuntimeRBAC instances by either
(A) passing AccountBloxABIJson into the RuntimeRBAC constructor/factory
(RuntimeRBAC(...)) if the class supports an abi parameter, or (B) centralizing
the override inside createRuntimeRBACWithWallet so it sets the instance ABI
immediately after creation (avoid using `as any` by using the proper typed
property or a setter on RuntimeRBAC); ensure both the existing this.runtimeRBAC
initialization and createRuntimeRBACWithWallet use the same mechanism so no
RuntimeRBAC is left with the incomplete default ABI.
| // TxStatus enum: 0=UNDEFINED, 1=PENDING, 2=EXECUTING, 3=PROCESSING_PAYMENT, 4=CANCELLED, 5=COMPLETED, 6=FAILED, 7=REJECTED | ||
| const tx = await this.callContractMethod(this.contract.methods.getTransaction(txRecord.txId), this.getRoleWalletObject('recovery')); | ||
| this.assertTest(tx.status === '5', 'Transaction completed successfully'); | ||
| this.assertTest(Number(tx.status) === 5, 'Transaction completed successfully'); |
Contributor
There was a problem hiding this comment.
Inconsistent status comparison pattern — apply Number() coercion uniformly or not at all.
Lines 443 and 661 now use Number(tx.status) === 5, but every other status check in this file still uses string comparison (e.g., tx.status === '1' on lines 98, 225, 343, 573 and tx.status === '4' on lines 196, 314). If the return type of getTransaction().status can vary (string vs. number/BigInt), the string comparisons elsewhere will silently pass or fail incorrectly under the same conditions that motivated this change.
Pick one approach and apply it consistently. Using Number() everywhere is the safer option:
Proposed fix (representative — apply to all similar lines)
- this.assertTest(tx.status === '1', 'Transaction is pending');
+ this.assertTest(Number(tx.status) === 1, 'Transaction is pending');- this.assertTest(tx.status === '4', 'Transaction cancelled successfully');
+ this.assertTest(Number(tx.status) === 4, 'Transaction cancelled successfully');Also update the fallback history scan on line 728:
- if (rec && rec.status === '1' && rec.params && rec.params.operationType &&
+ if (rec && Number(rec.status) === 1 && rec.params && rec.params.operationType &&Also applies to: 661-661
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@scripts/sanity/secure-ownable/ownership-transfer-tests.cjs` at line 443, The
transaction status comparisons are inconsistent: some checks use string equality
(e.g., tx.status === '1' or '4') while others use Number(tx.status) === 5;
convert all status checks to numeric coercion for consistency by wrapping
tx.status with Number() and comparing against numeric literals (e.g.,
Number(tx.status) === 1, 4, 5) wherever tx.status is tested (look for assertions
using this.assertTest and any direct tx.status comparisons), and update the
fallback history scan logic that reads tx.status (the scan around the fallback
on line ~728) to use Number(tx.status) as well so all branches use the same
numeric comparison pattern.
This commit updates the foundry.toml file to suppress common compiler warnings and disables linting during the build process for cleaner output. Additionally, it modifies the truffle-config to optimize the Solidity compiler settings by increasing the optimization runs to 200. In the RuntimeRBAC contract, the logging functions are updated to include the wallet address in role configuration events, improving traceability and clarity in role management actions.
Contributor
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 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/access/RuntimeRBAC.sol`:
- Around line 148-152: The _executeRemoveRole function is missing the protection
check that prevents removing protected roles; add a call to
_requireRoleNotProtected(roleHash) at the start of _executeRemoveRole (before
calling _removeRole and _logRoleConfigEvent) so removals of
OWNER/BROADCASTER/RECOVERY (and any other protected roles enforced by
_requireRoleNotProtected) are blocked in batch operations; locate
_executeRemoveRole, insert the same _requireRoleNotProtected(roleHash) guard
used in _executeAddWallet and _executeRevokeWallet.
In `@foundry.toml`:
- Around line 32-35: The ignored_error_codes entry currently masks Solidity
shadowing warnings (2519); remove 2519 from the ignored_error_codes array so the
compiler surfaces variable-shadowing issues (update the ignored_error_codes
symbol to exclude 2519), then run the build/tests and fix any shadowing
instances (rename locals, use explicit storage/visibility, or refactor) until no
2519 warnings remain. If you cannot immediately fix all cases, revert the change
but add a TODO comment next to ignored_error_codes referencing a tracking issue
(create an issue and include its ID) that lists all shadowing occurrences to be
resolved and a milestone to re-enable the 2519 check.
---
Duplicate comments:
In `@contracts/core/access/RuntimeRBAC.sol`:
- Around line 158-174: The event logging concern is resolved: both
_executeAddWallet and _executeRevokeWallet now pass the wallet into
_logRoleConfigEvent, so no code change is required here; just verify that the
_logRoleConfigEvent implementation accepts and records the wallet parameter
correctly and add/update unit/integration tests for _executeAddWallet and
_executeRevokeWallet to assert the emitted RoleConfig event contains the
expected wallet value.
Comment on lines
+148
to
+152
| function _executeRemoveRole(bytes calldata data) internal { | ||
| (bytes32 roleHash) = abi.decode(data, (bytes32)); | ||
| _removeRole(roleHash); | ||
| _logRoleConfigEvent(IRuntimeRBAC.RoleConfigActionType.REMOVE_ROLE, roleHash, bytes4(0), address(0)); | ||
| } |
Contributor
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
find . -name "RuntimeRBAC.sol" -type fRepository: PracticalParticle/Bloxchain-Protocol
Length of output: 119
🏁 Script executed:
cat -n contracts/core/access/RuntimeRBAC.sol | head -200Repository: PracticalParticle/Bloxchain-Protocol
Length of output: 10157
🏁 Script executed:
rg -n "_requireRoleNotProtected" -A 5 --type sol -B 2Repository: PracticalParticle/Bloxchain-Protocol
Length of output: 107
🏁 Script executed:
rg -n "_removeRole" -A 10 --type solRepository: PracticalParticle/Bloxchain-Protocol
Length of output: 107
🏁 Script executed:
rg -n "_removeRole" -g "*.sol" -A 10 -B 2Repository: PracticalParticle/Bloxchain-Protocol
Length of output: 2397
🏁 Script executed:
rg -n "isProtected" -g "*.sol" -B 2 -A 2 | head -100Repository: PracticalParticle/Bloxchain-Protocol
Length of output: 8584
🏁 Script executed:
rg -n "OWNER|BROADCASTER|RECOVERY" -g "*.sol" -B 1 -A 1 | head -80Repository: PracticalParticle/Bloxchain-Protocol
Length of output: 5623
Add missing _requireRoleNotProtected check in _executeRemoveRole.
_executeAddWallet and _executeRevokeWallet both guard against modifying protected roles via _requireRoleNotProtected, but _executeRemoveRole is missing this check. This allows removing protected roles (OWNER, BROADCASTER, RECOVERY) through batch operations, bypassing the design intent stated in the function's docstring and creating a critical access-control vulnerability.
Proposed fix
function _executeRemoveRole(bytes calldata data) internal {
(bytes32 roleHash) = abi.decode(data, (bytes32));
+ _requireRoleNotProtected(roleHash);
_removeRole(roleHash);
_logRoleConfigEvent(IRuntimeRBAC.RoleConfigActionType.REMOVE_ROLE, roleHash, bytes4(0), address(0));
}📝 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 _executeRemoveRole(bytes calldata data) internal { | |
| (bytes32 roleHash) = abi.decode(data, (bytes32)); | |
| _removeRole(roleHash); | |
| _logRoleConfigEvent(IRuntimeRBAC.RoleConfigActionType.REMOVE_ROLE, roleHash, bytes4(0), address(0)); | |
| } | |
| function _executeRemoveRole(bytes calldata data) internal { | |
| (bytes32 roleHash) = abi.decode(data, (bytes32)); | |
| _requireRoleNotProtected(roleHash); | |
| _removeRole(roleHash); | |
| _logRoleConfigEvent(IRuntimeRBAC.RoleConfigActionType.REMOVE_ROLE, roleHash, bytes4(0), address(0)); | |
| } |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@contracts/core/access/RuntimeRBAC.sol` around lines 148 - 152, The
_executeRemoveRole function is missing the protection check that prevents
removing protected roles; add a call to _requireRoleNotProtected(roleHash) at
the start of _executeRemoveRole (before calling _removeRole and
_logRoleConfigEvent) so removals of OWNER/BROADCASTER/RECOVERY (and any other
protected roles enforced by _requireRoleNotProtected) are blocked in batch
operations; locate _executeRemoveRole, insert the same
_requireRoleNotProtected(roleHash) guard used in _executeAddWallet and
_executeRevokeWallet.
This commit modifies the foundry.toml file to clarify the handling of compiler warnings by explicitly stating that shadowing (code 2519) is not suppressed, ensuring that variable-shadowing issues are surfaced. The list of ignored error codes has been updated accordingly for better clarity in the build process.
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@foundry.toml`:
- Around line 32-36: The ignored_error_codes array (ignored_error_codes) and
remappings setting are placed under the [invariant] section and therefore won’t
be applied; move both ignored_error_codes and remappings into the
[profile.default] section (i.e., cut the ignored_error_codes block and the
remappings entry from under [invariant] and paste them under [profile.default]
above the [fuzz] section), and then remove their originals from [invariant] so
the TOML parser applies these profile-level settings correctly.
foundry.toml
Outdated
Comment on lines
32
to
36
|
|
||
| # Suppress common compiler warnings (lint-style) so build output is clean | ||
| # Shadowing (2519) is NOT suppressed so the compiler surfaces variable-shadowing issues. | ||
| # Codes: 2072=unused-var, 5667=unused-param, 2018=func-mutability, 1878=license, 2462=constructor-visibility, 3628=missing-receive-ether, 6321=unnamed-return | ||
| ignored_error_codes = [2072, 5667, 2018, 1878, 2462, 3628, 6321] |
Contributor
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# First, check if foundry.toml exists and read it
if [ -f foundry.toml ]; then
echo "=== Full foundry.toml content ==="
cat -n foundry.toml
echo ""
echo "=== Parsing TOML structure ==="
pip install toml -q 2>/dev/null
python3 << 'EOF'
import toml
import json
with open('foundry.toml') as f:
data = toml.load(f)
print("Full parsed structure:")
print(json.dumps(data, indent=2))
print("\n=== Section Analysis ===")
if 'invariant' in data:
print(f"Keys in [invariant]: {list(data['invariant'].keys())}")
if 'profile' in data and 'default' in data['profile']:
profile_default = data['profile']['default']
print(f"Keys in [profile.default]: {list(profile_default.keys())}")
if 'invariant' in profile_default:
print(f" Keys in [profile.default.invariant]: {list(profile_default['invariant'].keys())}")
if 'fuzz' in profile_default:
print(f" Keys in [profile.default.fuzz]: {list(profile_default['fuzz'].keys())}")
if 'lint' in data:
print(f"Keys in [lint]: {list(data['lint'].keys())}")
if 'profile' in data and 'default' in data['profile'] and 'lint' in data['profile']['default']:
print(f"Keys in [profile.default.lint]: {list(data['profile']['default']['lint'].keys())}")
print("\n=== Checking for ignored_error_codes location ===")
for section, content in data.items():
if isinstance(content, dict) and 'ignored_error_codes' in content:
print(f"Found 'ignored_error_codes' in section: [{section}]")
print(f" Value: {content['ignored_error_codes']}")
EOF
else
echo "foundry.toml not found"
fiRepository: PracticalParticle/Bloxchain-Protocol
Length of output: 2846
ignored_error_codes is under the wrong section and will not take effect.
The TOML structure shows ignored_error_codes (line 36) is parsed as part of the [invariant] section, not [profile.default]. Compiler error code suppression is a profile-level setting and must appear under [profile.default] to work correctly. In its current position, it will be ignored.
Move ignored_error_codes and its comments above the [fuzz] section:
Suggested fix
evm_version = "osaka"
+# Suppress common compiler warnings (lint-style) so build output is clean
+# Shadowing (2519) is NOT suppressed so the compiler surfaces variable-shadowing issues.
+# Codes: 2072=unused-var, 5667=unused-param, 2018=func-mutability, 1878=license, 2462=constructor-visibility, 3628=missing-receive-ether, 6321=unnamed-return
+ignored_error_codes = [2072, 5667, 2018, 1878, 2462, 3628, 6321]
+
# Remappings for OpenZeppelin dependencies
remappings = [
"@openzeppelin/=node_modules/@openzeppelin/",
"forge-std/=lib/forge-std/src/",
]
# Fuzzing configuration
[fuzz]Then remove lines 33–36 from their current location under [invariant].
Note: remappings (line 28) has the same placement issue—it is also under [invariant] instead of [profile.default]. Consider relocating it as well.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@foundry.toml` around lines 32 - 36, The ignored_error_codes array
(ignored_error_codes) and remappings setting are placed under the [invariant]
section and therefore won’t be applied; move both ignored_error_codes and
remappings into the [profile.default] section (i.e., cut the ignored_error_codes
block and the remappings entry from under [invariant] and paste them under
[profile.default] above the [fuzz] section), and then remove their originals
from [invariant] so the TOML parser applies these profile-level settings
correctly.
… suppression This commit modifies the foundry.toml file to reintroduce remappings for OpenZeppelin dependencies and suppress common compiler warnings for a cleaner build output. The list of ignored error codes has been clarified, ensuring that variable-shadowing issues are still surfaced while improving overall build clarity.
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.
Summary by CodeRabbit
New Features
Bug Fixes
Refactor
Documentation
Chores