Add AccessControlBatch extension for batch role assignment #6274
Conversation
🦋 Changeset detectedLatest commit: 8b34161 The changes in this PR will be included in the next version bump. This PR includes changesets to release 1 package
Not sure what this means? Click here to learn what changesets are. Click here if you're a maintainer who wants to add another changeset to this PR |
WalkthroughAdds an abstract 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing touches
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}
There was a problem hiding this comment.
Actionable comments posted: 3
🤖 Fix all issues with AI agents
In @contracts/access/extensions/AccessControlBatch.sol:
- Around line 19-27: The grantRoles function allows granting roles to addresses
without checking for the zero address; add a non-zero validation inside the loop
(or a separate pre-check) to ensure accounts[i] != address(0) before calling
_grantRole and revert with a clear message like "AccessControlBatch: zero
address" when violated; update the grantRoles function (referencing grantRoles,
roles, accounts, and _grantRole) to perform this check for each account to
prevent accidental grants to address(0).
- Around line 36-44: The revokeRoles function lacks a zero-address check for
accounts, causing inconsistency with grantRoles; before calling _revokeRole in
revokeRoles(bytes32[] calldata roles, address[] calldata accounts) add a
validation that accounts[i] != address(0) (revert with a clear message like
"AccessControlBatch: zero address") for each entry (do the check inside the for
loop prior to _revokeRole) so accidental operations on the zero address are
prevented and behavior matches grantRoles.
- Around line 19-44: Add validation in grantRoles and revokeRoles to reject the
zero address: inside the for-loop that iterates over roles/accounts in
grantRoles(bytes32[] calldata roles, address[] calldata accounts) and
revokeRoles(...) check if accounts[i] == address(0) and revert with a clear
message (e.g., "AccessControlBatch: invalid account") before calling
_grantRole(roles[i], accounts[i]) or _revokeRole(roles[i], accounts[i]); ensure
both functions enforce the same check and keep the existing length mismatch
check and onlyRole modifier.
🧹 Nitpick comments (3)
test/access/AccessControlBatch.test.js (1)
40-42: Consider checking the specific revert reason.The test validates that non-admin access is blocked, but using a more specific assertion would improve test clarity. Consider verifying the exact AccessControl error message.
📝 More specific assertion
- it('reverts when caller is not admin', async function () { - await expect(contract.connect(user1).grantRoles([ROLE_A], [user1.address])).to.be.reverted; - }); + it('reverts when caller is not admin', async function () { + await expect(contract.connect(user1).grantRoles([ROLE_A], [user1.address])) + .to.be.revertedWithCustomError(contract, 'AccessControlUnauthorizedAccount') + .withArgs(user1.address, await contract.DEFAULT_ADMIN_ROLE()); + });contracts/access/extensions/AccessControlBatch.sol (2)
20-22: Consider using custom errors for gas efficiency.String reverts consume more gas than custom errors. OpenZeppelin contracts typically use custom errors for better gas optimization.
⚡ Custom error implementation
Add this error declaration near the top of the contract:
error AccessControlBatchLengthMismatch(uint256 rolesLength, uint256 accountsLength);Then update the revert:
- if (roles.length != accounts.length) { - revert("AccessControlBatch: length mismatch"); - } + if (roles.length != accounts.length) { + revert AccessControlBatchLengthMismatch(roles.length, accounts.length); + }
37-39: Consider using custom errors for gas efficiency.Same recommendation as
grantRoles: using custom errors would improve gas efficiency. The same custom error can be reused for both functions.
📜 Review details
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
contracts/access/extensions/AccessControlBatch.solcontracts/mocks/AccessControlBatchMock.soltest/access/AccessControlBatch.test.js
🧰 Additional context used
🧠 Learnings (1)
📚 Learning: 2025-08-29T13:16:08.640Z
Learnt from: Amxx
Repo: OpenZeppelin/openzeppelin-contracts PR: 5904
File: contracts/mocks/crosschain/ERC7786RecipientMock.sol:12-14
Timestamp: 2025-08-29T13:16:08.640Z
Learning: In OpenZeppelin contracts, mock contracts (like ERC7786RecipientMock) don't require input validation such as zero-address checks in constructors, as they are only used for testing purposes in controlled environments.
Applied to files:
contracts/mocks/AccessControlBatchMock.sol
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (9)
- GitHub Check: Redirect rules - solidity-contracts
- GitHub Check: Header rules - solidity-contracts
- GitHub Check: Pages changed - solidity-contracts
- GitHub Check: slither
- GitHub Check: tests-foundry
- GitHub Check: tests
- GitHub Check: coverage
- GitHub Check: tests-upgradeable
- GitHub Check: halmos
🔇 Additional comments (5)
contracts/mocks/AccessControlBatchMock.sol (1)
1-10: LGTM! Clean mock implementation.The mock correctly extends
AccessControlBatchand initializes the admin role. The absence of zero-address validation is acceptable for mock contracts used in testing.test/access/AccessControlBatch.test.js (3)
4-16: LGTM! Test setup is well-structured.The setup correctly initializes test roles using
ethers.idand deploys the mock contract with proper admin configuration.
18-23: LGTM! Batch grant test is comprehensive.The test validates that multiple roles can be granted in a single transaction and correctly asserts both role assignments.
25-32: LGTM! Batch revoke test is thorough.The test properly validates the batch revocation flow by first granting roles and then verifying their removal.
contracts/access/extensions/AccessControlBatch.sol (1)
1-11: LGTM! Well-documented extension contract.The contract header properly documents that this is an optional extension and correctly extends
AccessControl.
|
@coderabbitai please re-review |
|
@harshkumarrai: I'll re-review the PR for you. ✅ Actions performedReview triggered.
|
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (4)
test/access/AccessControlBatch.test.js (4)
18-23: Consider using more idiomatic chai assertions.The test correctly verifies batch role grants. For boolean assertions, consider using
.to.be.trueinstead of.to.equal(true)for more idiomatic chai syntax.✨ Optional style improvement
- expect(await contract.hasRole(ROLE_A, user1.address)).to.equal(true); - expect(await contract.hasRole(ROLE_B, user2.address)).to.equal(true); + expect(await contract.hasRole(ROLE_A, user1.address)).to.be.true; + expect(await contract.hasRole(ROLE_B, user2.address)).to.be.true;
25-32: Consider using more idiomatic chai assertions.The test correctly verifies batch role revocation. For consistency with chai best practices, consider using
.to.be.falseinstead of.to.equal(false).✨ Optional style improvement
- expect(await contract.hasRole(ROLE_A, user1.address)).to.equal(false); - expect(await contract.hasRole(ROLE_B, user2.address)).to.equal(false); + expect(await contract.hasRole(ROLE_A, user1.address)).to.be.false; + expect(await contract.hasRole(ROLE_B, user2.address)).to.be.false;
57-59: Consider checking specific revert reason for more precise test validation.The test correctly verifies that non-admin callers cannot grant roles, but using a generic
.to.be.revertedis less precise. Consider verifying the specific AccessControl error message to ensure the correct validation is being triggered.♻️ More precise assertion
it('reverts when caller is not admin', async function () { - await expect(contract.connect(user1).grantRoles([ROLE_A], [user1.address])).to.be.reverted; + await expect(contract.connect(user1).grantRoles([ROLE_A], [user1.address])) + .to.be.revertedWithCustomError(contract, 'AccessControlUnauthorizedAccount') + .withArgs(user1.address, await contract.DEFAULT_ADMIN_ROLE()); });Note: Adjust the error name/format based on the actual AccessControl implementation in use.
4-65: Consider adding event emission tests and additional edge case coverage.The current test suite covers the main success and failure paths well. To improve robustness, consider adding:
Recommended: Event emission verification - AccessControl typically emits
RoleGrantedandRoleRevokedevents. Verify these are emitted correctly for each role in the batch.Optional: Additional edge cases:
- Empty arrays (both roles and accounts)
- Granting the same role to multiple accounts
- Granting multiple roles to the same account
- Re-granting an already granted role
- Revoking a role that was never granted
📝 Example event verification test
it('emits RoleGranted events for each role', async function () { await expect(contract.connect(admin).grantRoles([ROLE_A, ROLE_B], [user1.address, user2.address])) .to.emit(contract, 'RoleGranted') .withArgs(ROLE_A, user1.address, admin.address) .to.emit(contract, 'RoleGranted') .withArgs(ROLE_B, user2.address, admin.address); }); it('emits RoleRevoked events for each role', async function () { await contract.connect(admin).grantRoles([ROLE_A, ROLE_B], [user1.address, user2.address]); await expect(contract.connect(admin).revokeRoles([ROLE_A, ROLE_B], [user1.address, user2.address])) .to.emit(contract, 'RoleRevoked') .withArgs(ROLE_A, user1.address, admin.address) .to.emit(contract, 'RoleRevoked') .withArgs(ROLE_B, user2.address, admin.address); });
📜 Review details
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
.changeset/upset-teeth-play.mdcontracts/access/extensions/AccessControlBatch.soltest/access/AccessControlBatch.test.js
🚧 Files skipped from review as they are similar to previous changes (1)
- contracts/access/extensions/AccessControlBatch.sol
🔇 Additional comments (2)
.changeset/upset-teeth-play.md (1)
1-5: LGTM!The changeset entry correctly specifies a minor version bump for adding a new optional extension, and the description accurately reflects the changes.
test/access/AccessControlBatch.test.js (1)
1-16: LGTM!The test setup is well-structured with appropriate imports, role definitions using
ethers.id(), and clean contract deployment in thebeforeEachhook.
|
@coderabbitai please re-review the changes |
|
@harshkumarrai: I'll re-review the changes in the PR for you. ✅ Actions performedReview triggered.
|
1 participant
Fixes #6143
This PR adds an optional
AccessControlBatchextension that allowsgranting and revoking multiple roles to multiple accounts in a single
transaction, without modifying the core
AccessControlcontract.The change addresses the batch role assignment use case discussed in #6143.