Skip to content

test: fix execution delay edge cases in AccessManaged tests #5972

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Open
wants to merge 1 commit into
base: master
Choose a base branch
from
+37 −1
Open

test: fix execution delay edge cases in AccessManaged tests #5972

Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
38 changes: 37 additions & 1 deletion test/access/manager/AccessManaged.test.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -65,7 +65,7 @@

describe('when role is granted with execution delay', function () {
beforeEach(async function () {
const executionDelay = 911n;
const executionDelay = time.duration.hours(1); // 1 hour - standard test value
await this.authority.$_grantRole(this.role, this.roleMember, 0, executionDelay);
});

Expand Down Expand Up @@ -97,6 +97,42 @@
// Shouldn't revert
await this.managed.connect(this.roleMember)[this.selector]();
});

it('succeeds when role is granted with zero execution delay', async function () {
// Grant role with zero execution delay
await this.authority.$_grantRole(this.role, this.roleMember, 0, 0n);

// Should work without scheduling
await this.managed.connect(this.roleMember)[this.selector]();
});

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');
});
Comment on lines +109 to +116
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ Potential issue | 🟡 Minor

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.

Suggested change
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);

Check failure on line 126 in test/access/manager/AccessManaged.test.js

View workflow job for this annotation

GitHub Actions / codespell 

pastTime ==> pastime

// Should automatically adjust time to minimum required
const { operationId } = await this.authority.connect(this.roleMember).schedule(this.managed, calldata, pastTime);

Check failure on line 129 in test/access/manager/AccessManaged.test.js

View workflow job for this annotation

GitHub Actions / codespell 

pastTime ==> pastime
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);
});
Comment on lines +118 to +135
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ Potential issue | 🟠 Major

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.

Suggested change
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.

});
});

Expand Down
Loading