-
Notifications
You must be signed in to change notification settings - Fork 11.8k
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.
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
Migrate AccessManager
tests to ethers
#4710
Merged
ernestognw
merged 10 commits into
OpenZeppelin:master
from
ernestognw:tests/access-manager-migration
Nov 9, 2023
Merged
Changes from all commits
Commits
Show all changes
10 commits
Select commit
Hold shift + click to select a range
3b15c69
Migrate `AcessManager` tests to ethers
ernestognw 3e7b508
Remove unnecessary BigInt()s
ernestognw 4a6401c
Remove `mine()` calls
ernestognw a6d363d
Apply review suggestions
ernestognw 90cf0e1
Avoid unnecessary `selector` use
ernestognw 863e6aa
sendTransaction to AddressLike object without dereferencing address
Amxx c2635a0
use Addressable directly in calls
Amxx 6c6c8f4
use AddressLike object where possible
Amxx 5e4784a
Apply review suggestion
ernestognw 67d5b41
Lint
ernestognw File filter
Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
There are no files selected for viewing
This file contains 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
This file contains 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
This file contains 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
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -1,142 +1,145 @@ | ||
const { expectEvent, time, expectRevert } = require('@openzeppelin/test-helpers'); | ||
const { selector } = require('../../helpers/methods'); | ||
const { expectRevertCustomError } = require('../../helpers/customError'); | ||
const { | ||
time: { setNextBlockTimestamp }, | ||
} = require('@nomicfoundation/hardhat-network-helpers'); | ||
const { bigint: time } = require('../../helpers/time'); | ||
const { loadFixture } = require('@nomicfoundation/hardhat-network-helpers'); | ||
const { impersonate } = require('../../helpers/account'); | ||
const { ethers } = require('hardhat'); | ||
|
||
const AccessManaged = artifacts.require('$AccessManagedTarget'); | ||
const AccessManager = artifacts.require('$AccessManager'); | ||
async function fixture() { | ||
const [admin, roleMember, other] = await ethers.getSigners(); | ||
|
||
const AuthoritiyObserveIsConsuming = artifacts.require('$AuthoritiyObserveIsConsuming'); | ||
const authority = await ethers.deployContract('$AccessManager', [admin]); | ||
const managed = await ethers.deployContract('$AccessManagedTarget', [authority]); | ||
|
||
contract('AccessManaged', function (accounts) { | ||
const [admin, roleMember, other] = accounts; | ||
const anotherAuthority = await ethers.deployContract('$AccessManager', [admin]); | ||
const authorityObserveIsConsuming = await ethers.deployContract('$AuthorityObserveIsConsuming'); | ||
|
||
await impersonate(authority.target); | ||
const authorityAsSigner = await ethers.getSigner(authority.target); | ||
|
||
return { | ||
roleMember, | ||
other, | ||
authorityAsSigner, | ||
authority, | ||
managed, | ||
authorityObserveIsConsuming, | ||
anotherAuthority, | ||
}; | ||
} | ||
|
||
describe('AccessManaged', function () { | ||
beforeEach(async function () { | ||
this.authority = await AccessManager.new(admin); | ||
this.managed = await AccessManaged.new(this.authority.address); | ||
Object.assign(this, await loadFixture(fixture)); | ||
}); | ||
|
||
it('sets authority and emits AuthorityUpdated event during construction', async function () { | ||
await expectEvent.inConstruction(this.managed, 'AuthorityUpdated', { | ||
authority: this.authority.address, | ||
}); | ||
expect(await this.managed.authority()).to.eq(this.authority.address); | ||
await expect(await this.managed.deploymentTransaction()) | ||
.to.emit(this.managed, 'AuthorityUpdated') | ||
.withArgs(this.authority.target); | ||
}); | ||
|
||
describe('restricted modifier', function () { | ||
const method = 'fnRestricted()'; | ||
|
||
beforeEach(async function () { | ||
this.selector = selector(method); | ||
this.role = web3.utils.toBN(42); | ||
await this.authority.$_setTargetFunctionRole(this.managed.address, this.selector, this.role); | ||
await this.authority.$_grantRole(this.role, roleMember, 0, 0); | ||
this.selector = this.managed.fnRestricted.getFragment().selector; | ||
this.role = 42n; | ||
await this.authority.$_setTargetFunctionRole(this.managed, this.selector, this.role); | ||
await this.authority.$_grantRole(this.role, this.roleMember, 0, 0); | ||
}); | ||
|
||
it('succeeds when role is granted without execution delay', async function () { | ||
await this.managed.methods[method]({ from: roleMember }); | ||
await this.managed.connect(this.roleMember)[this.selector](); | ||
}); | ||
|
||
it('reverts when role is not granted', async function () { | ||
await expectRevertCustomError(this.managed.methods[method]({ from: other }), 'AccessManagedUnauthorized', [ | ||
other, | ||
]); | ||
await expect(this.managed.connect(this.other)[this.selector]()) | ||
.to.be.revertedWithCustomError(this.managed, 'AccessManagedUnauthorized') | ||
.withArgs(this.other.address); | ||
}); | ||
|
||
it('panics in short calldata', async function () { | ||
// We avoid adding the `restricted` modifier to the fallback function because other tests may depend on it | ||
// being accessible without restrictions. We check for the internal `_checkCanCall` instead. | ||
await expectRevert.unspecified(this.managed.$_checkCanCall(other, '0x1234')); | ||
await expect(this.managed.$_checkCanCall(this.roleMember, '0x1234')).to.be.reverted; | ||
}); | ||
|
||
describe('when role is granted with execution delay', function () { | ||
beforeEach(async function () { | ||
const executionDelay = web3.utils.toBN(911); | ||
await this.authority.$_grantRole(this.role, roleMember, 0, executionDelay); | ||
const executionDelay = 911n; | ||
await this.authority.$_grantRole(this.role, this.roleMember, 0, executionDelay); | ||
}); | ||
|
||
it('reverts if the operation is not scheduled', async function () { | ||
const calldata = this.managed.contract.methods[method]().encodeABI(); | ||
const opId = await this.authority.hashOperation(roleMember, this.managed.address, calldata); | ||
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); | ||
|
||
await expectRevertCustomError(this.managed.methods[method]({ from: roleMember }), 'AccessManagerNotScheduled', [ | ||
opId, | ||
]); | ||
await expect(this.managed.connect(this.roleMember)[this.selector]()) | ||
.to.be.revertedWithCustomError(this.authority, 'AccessManagerNotScheduled') | ||
.withArgs(opId); | ||
}); | ||
|
||
it('succeeds if the operation is scheduled', async function () { | ||
// Arguments | ||
const delay = time.duration.hours(12); | ||
const calldata = this.managed.contract.methods[method]().encodeABI(); | ||
const fn = this.managed.interface.getFunction(this.selector); | ||
const calldata = this.managed.interface.encodeFunctionData(fn, []); | ||
|
||
// Schedule | ||
const timestamp = await time.latest(); | ||
const scheduledAt = timestamp.addn(1); | ||
const when = scheduledAt.add(delay); | ||
await setNextBlockTimestamp(scheduledAt); | ||
await this.authority.schedule(this.managed.address, calldata, when, { | ||
from: roleMember, | ||
}); | ||
const timestamp = await time.clock.timestamp(); | ||
const scheduledAt = timestamp + 1n; | ||
const when = scheduledAt + delay; | ||
await time.forward.timestamp(scheduledAt, false); | ||
await this.authority.connect(this.roleMember).schedule(this.managed, calldata, when); | ||
|
||
// Set execution date | ||
await setNextBlockTimestamp(when); | ||
await time.forward.timestamp(when, false); | ||
|
||
// Shouldn't revert | ||
await this.managed.methods[method]({ from: roleMember }); | ||
await this.managed.connect(this.roleMember)[this.selector](); | ||
}); | ||
}); | ||
}); | ||
|
||
describe('setAuthority', function () { | ||
beforeEach(async function () { | ||
this.newAuthority = await AccessManager.new(admin); | ||
}); | ||
|
||
it('reverts if the caller is not the authority', async function () { | ||
await expectRevertCustomError(this.managed.setAuthority(other, { from: other }), 'AccessManagedUnauthorized', [ | ||
other, | ||
]); | ||
await expect(this.managed.connect(this.other).setAuthority(this.other)) | ||
.to.be.revertedWithCustomError(this.managed, 'AccessManagedUnauthorized') | ||
.withArgs(this.other.address); | ||
}); | ||
|
||
it('reverts if the new authority is not a valid authority', async function () { | ||
await impersonate(this.authority.address); | ||
await expectRevertCustomError( | ||
this.managed.setAuthority(other, { from: this.authority.address }), | ||
'AccessManagedInvalidAuthority', | ||
[other], | ||
); | ||
await expect(this.managed.connect(this.authorityAsSigner).setAuthority(this.other)) | ||
.to.be.revertedWithCustomError(this.managed, 'AccessManagedInvalidAuthority') | ||
.withArgs(this.other.address); | ||
}); | ||
|
||
it('sets authority and emits AuthorityUpdated event', async function () { | ||
await impersonate(this.authority.address); | ||
const { receipt } = await this.managed.setAuthority(this.newAuthority.address, { from: this.authority.address }); | ||
await expectEvent(receipt, 'AuthorityUpdated', { | ||
authority: this.newAuthority.address, | ||
}); | ||
expect(await this.managed.authority()).to.eq(this.newAuthority.address); | ||
await expect(this.managed.connect(this.authorityAsSigner).setAuthority(this.anotherAuthority)) | ||
.to.emit(this.managed, 'AuthorityUpdated') | ||
.withArgs(this.anotherAuthority.target); | ||
|
||
expect(await this.managed.authority()).to.equal(this.anotherAuthority.target); | ||
}); | ||
}); | ||
|
||
describe('isConsumingScheduledOp', function () { | ||
beforeEach(async function () { | ||
this.authority = await AuthoritiyObserveIsConsuming.new(); | ||
this.managed = await AccessManaged.new(this.authority.address); | ||
await this.managed.connect(this.authorityAsSigner).setAuthority(this.authorityObserveIsConsuming); | ||
}); | ||
|
||
it('returns bytes4(0) when not consuming operation', async function () { | ||
expect(await this.managed.isConsumingScheduledOp()).to.eq('0x00000000'); | ||
expect(await this.managed.isConsumingScheduledOp()).to.equal('0x00000000'); | ||
}); | ||
|
||
it('returns isConsumingScheduledOp selector when consuming operation', async function () { | ||
const receipt = await this.managed.fnRestricted({ from: other }); | ||
await expectEvent.inTransaction(receipt.tx, this.authority, 'ConsumeScheduledOpCalled', { | ||
caller: other, | ||
data: this.managed.contract.methods.fnRestricted().encodeABI(), | ||
isConsuming: selector('isConsumingScheduledOp()'), | ||
}); | ||
const isConsumingScheduledOp = this.managed.interface.getFunction('isConsumingScheduledOp()'); | ||
const fnRestricted = this.managed.fnRestricted.getFragment(); | ||
await expect(this.managed.connect(this.other).fnRestricted()) | ||
.to.emit(this.authorityObserveIsConsuming, 'ConsumeScheduledOpCalled') | ||
.withArgs( | ||
this.other.address, | ||
this.managed.interface.encodeFunctionData(fnRestricted, []), | ||
isConsumingScheduledOp.selector, | ||
); | ||
}); | ||
}); | ||
}); |
Oops, something went wrong.
Oops, something went wrong.
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.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think there is a way to check that the revert was a panic. We should do that.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Note, if we don't specify the panic code, it will work just like we want. Sources: https://github.com/NomicFoundation/hardhat/blob/main/packages/hardhat-chai-matchers/src/internal/reverted/revertedWithPanic.ts
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This doesn't work but I'm not sure why. If done like this, we get an error saying that it reverted without a reason:
Array slices don't specify if they panic in the docs, but the section with the panic codes does mention it panics for "... an array slice at an out-of-bounds or negative index"
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Interresting. Lets leave it like that for now. I'll try to come up with a minimum reproductive example and report it somewhere.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I opened this ticket. Seems to be a vague interpretation of what the panic code means. Seems to be referring to only accessing an array out of bounds but it does mention array slices in the docs.
Let's see what they say