test: fix execution delay edge cases in AccessManaged tests #5972
Conversation
|
WalkthroughUpdates tests in test/access/manager/AccessManaged.test.js. Replaces a hardcoded executionDelay of 911n with a 1-hour delay for the “when role is granted with execution delay” setup. Adds three test cases: zero execution delay (no scheduling required), maximum uint32 execution delay (scheduling required, expects AccessManagerNotScheduled on unscheduled execution), and scheduling with a past timestamp adjusts to at least now + executionDelay. Retains existing assertions distinguishing scheduled vs. unscheduled operations and corresponding AccessManagerNotScheduled reverts where applicable. No changes to exported or public entities. Pre-merge checks and finishing touches✅ Passed checks (3 passed)
✨ 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}
There was a problem hiding this comment.
Actionable comments posted: 2
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
test/access/manager/AccessManaged.test.js(2 hunks)
🧰 Additional context used
🧬 Code graph analysis (1)
test/access/manager/AccessManaged.test.js (3)
test/governance/extensions/GovernorTimelockAccess.test.js (1)
time(11-11)test/access/manager/AccessManager.test.js (1)
time(8-8)test/helpers/access-manager.js (1)
time(4-4)
🪛 GitHub Check: codespell
test/access/manager/AccessManaged.test.js
[failure] 129-129:
pastTime ==> pastime
[failure] 126-126:
pastTime ==> pastime
⏰ 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: halmos
- GitHub Check: slither
- GitHub Check: coverage
- GitHub Check: tests-foundry
- GitHub Check: tests-upgradeable
- GitHub Check: tests
🔇 Additional comments (2)
test/access/manager/AccessManaged.test.js (2)
68-68: LGTM! Improved readability.Replacing the magic number
911nwithtime.duration.hours(1)makes the test more readable and consistent with the codebase standards.
101-107: LGTM! Zero delay edge case covered.The test correctly validates that when a role is granted with zero execution delay, no scheduling is required for immediate execution.
| it('handles maximum execution delay correctly', async function () { | ||
| const maxExecutionDelay = 2n**32n - 1n; // uint32 max | ||
| await this.authority.$_grantRole(this.role, this.roleMember, 0, maxExecutionDelay); | ||
|
|
||
| // Should require scheduling | ||
| await expect(this.managed.connect(this.roleMember)[this.selector]()) | ||
| .to.be.revertedWithCustomError(this.authority, 'AccessManagerNotScheduled'); | ||
| }); |
There was a problem hiding this comment.
Add opId to error assertion for consistency.
The test should calculate the opId and include it in .withArgs(opId) for a more precise assertion, consistent with the similar test at lines 72-79.
Apply this diff to add the missing opId:
it('handles maximum execution delay correctly', async function () {
const maxExecutionDelay = 2n**32n - 1n; // uint32 max
await this.authority.$_grantRole(this.role, this.roleMember, 0, maxExecutionDelay);
+ const fn = this.managed.interface.getFunction(this.selector);
+ const calldata = this.managed.interface.encodeFunctionData(fn, []);
+ const opId = await this.authority.hashOperation(this.roleMember, this.managed, calldata);
+
// Should require scheduling
await expect(this.managed.connect(this.roleMember)[this.selector]())
- .to.be.revertedWithCustomError(this.authority, 'AccessManagerNotScheduled');
+ .to.be.revertedWithCustomError(this.authority, 'AccessManagerNotScheduled')
+ .withArgs(opId);
});📝 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.
| it('handles maximum execution delay correctly', async function () { | |
| const maxExecutionDelay = 2n**32n - 1n; // uint32 max | |
| await this.authority.$_grantRole(this.role, this.roleMember, 0, maxExecutionDelay); | |
| // Should require scheduling | |
| await expect(this.managed.connect(this.roleMember)[this.selector]()) | |
| .to.be.revertedWithCustomError(this.authority, 'AccessManagerNotScheduled'); | |
| }); | |
| it('handles maximum execution delay correctly', async function () { | |
| const maxExecutionDelay = 2n**32n - 1n; // uint32 max | |
| await this.authority.$_grantRole(this.role, this.roleMember, 0, maxExecutionDelay); | |
| const fn = this.managed.interface.getFunction(this.selector); | |
| const calldata = this.managed.interface.encodeFunctionData(fn, []); | |
| const opId = await this.authority.hashOperation(this.roleMember, this.managed, calldata); | |
| // Should require scheduling | |
| await expect(this.managed.connect(this.roleMember)[this.selector]()) | |
| .to.be.revertedWithCustomError(this.authority, 'AccessManagerNotScheduled') | |
| .withArgs(opId); | |
| }); |
🤖 Prompt for AI Agents
In test/access/manager/AccessManaged.test.js around lines 109 to 116, the
assertion for the revert should include the computed opId like the earlier test;
compute opId the same way as in the test at lines 72-79 (using the role,
roleMember, selector, and delay values used) and change the expect to assert
.to.be.revertedWithCustomError(this.authority,
'AccessManagerNotScheduled').withArgs(opId) so the test verifies the specific
operation id.
| it('adjusts when time to minimum required time if too early', async function () { | ||
| const executionDelay = time.duration.hours(1); | ||
| await this.authority.$_grantRole(this.role, this.roleMember, 0, executionDelay); | ||
|
|
||
| const fn = this.managed.interface.getFunction(this.selector); | ||
| const calldata = this.managed.interface.encodeFunctionData(fn, []); | ||
|
|
||
| // Try to schedule in the past | ||
| const pastTime = (await time.clock.timestamp()) - time.duration.hours(1); | ||
|
|
||
| // Should automatically adjust time to minimum required | ||
| const { operationId } = await this.authority.connect(this.roleMember).schedule(this.managed, calldata, pastTime); | ||
| const scheduledTime = await this.authority.getSchedule(operationId); | ||
|
|
||
| // Verify time was adjusted to minimum required | ||
| const minTime = (await time.clock.timestamp()) + executionDelay; | ||
| expect(scheduledTime).to.be.at.least(minTime); | ||
| }); |
There was a problem hiding this comment.
Fix timing inconsistency in minTime calculation.
The test calculates minTime at line 133 using the current timestamp, but this occurs after the schedule() call at line 129. The timestamp used for minTime should be captured before or at the time of scheduling to ensure the assertion accurately validates the behavior.
Apply this diff to fix the timing issue:
it('adjusts when time to minimum required time if too early', async function () {
const executionDelay = time.duration.hours(1);
await this.authority.$_grantRole(this.role, this.roleMember, 0, executionDelay);
const fn = this.managed.interface.getFunction(this.selector);
const calldata = this.managed.interface.encodeFunctionData(fn, []);
// Try to schedule in the past
- const pastTime = (await time.clock.timestamp()) - time.duration.hours(1);
+ const currentTime = await time.clock.timestamp();
+ const pastTime = currentTime - time.duration.hours(1);
// Should automatically adjust time to minimum required
const { operationId } = await this.authority.connect(this.roleMember).schedule(this.managed, calldata, pastTime);
const scheduledTime = await this.authority.getSchedule(operationId);
// Verify time was adjusted to minimum required
- const minTime = (await time.clock.timestamp()) + executionDelay;
+ const minTime = currentTime + executionDelay;
expect(scheduledTime).to.be.at.least(minTime);
});📝 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.
| it('adjusts when time to minimum required time if too early', async function () { | |
| const executionDelay = time.duration.hours(1); | |
| await this.authority.$_grantRole(this.role, this.roleMember, 0, executionDelay); | |
| const fn = this.managed.interface.getFunction(this.selector); | |
| const calldata = this.managed.interface.encodeFunctionData(fn, []); | |
| // Try to schedule in the past | |
| const pastTime = (await time.clock.timestamp()) - time.duration.hours(1); | |
| // Should automatically adjust time to minimum required | |
| const { operationId } = await this.authority.connect(this.roleMember).schedule(this.managed, calldata, pastTime); | |
| const scheduledTime = await this.authority.getSchedule(operationId); | |
| // Verify time was adjusted to minimum required | |
| const minTime = (await time.clock.timestamp()) + executionDelay; | |
| expect(scheduledTime).to.be.at.least(minTime); | |
| }); | |
| it('adjusts when time to minimum required time if too early', async function () { | |
| const executionDelay = time.duration.hours(1); | |
| await this.authority.$_grantRole(this.role, this.roleMember, 0, executionDelay); | |
| const fn = this.managed.interface.getFunction(this.selector); | |
| const calldata = this.managed.interface.encodeFunctionData(fn, []); | |
| // Try to schedule in the past | |
| const currentTime = await time.clock.timestamp(); | |
| const pastTime = currentTime - time.duration.hours(1); | |
| // Should automatically adjust time to minimum required | |
| const { operationId } = await this.authority.connect(this.roleMember).schedule(this.managed, calldata, pastTime); | |
| const scheduledTime = await this.authority.getSchedule(operationId); | |
| // Verify time was adjusted to minimum required | |
| const minTime = currentTime + executionDelay; | |
| expect(scheduledTime).to.be.at.least(minTime); | |
| }); |
🧰 Tools
🪛 GitHub Check: codespell
[failure] 129-129:
pastTime ==> pastime
[failure] 126-126:
pastTime ==> pastime
🤖 Prompt for AI Agents
In test/access/manager/AccessManaged.test.js around lines 118 to 135, the
calculation of minTime happens after calling schedule which introduces a timing
race; capture the current timestamp immediately before calling schedule (e.g.,
const now = await time.clock.timestamp()), compute minTime = now +
executionDelay using that captured value, then call schedule and use the
computed minTime in the assertion so the test consistently validates that
scheduledTime >= minTime.
Fix ctest coverage gaps in AccessManaged execution delay handling
Replace magic number 911n with standard time.duration.hours(1) for better
readability and consistency with other tests in the codebase
Add missing edge case tests for execution delay:
is set to a past timestamp by automatically setting it to minimum required time
These changes are necessary because:
boundary conditions that could be exploited
not being tested, potentially hiding bugs in the automatic time adjustment feature
and must be thoroughly tested to prevent privilege escalation or denial of service