Migrate Ownable tests #4657
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,72 +1,75 @@ | ||
| const { ethers } = require('hardhat'); | ||
| const { expect } = require('chai'); | ||
| const { loadFixture } = require('@nomicfoundation/hardhat-network-helpers'); | ||
|
|
||
| async function fixture() { | ||
| const accounts = await ethers.getSigners(); // this is slow :/ | ||
| const owner = accounts.shift(); | ||
| const other = accounts.shift(); | ||
| const ownable = await ethers.deployContract('$Ownable', [owner]); | ||
| return { accounts, owner, other, ownable }; | ||
| } | ||
|
|
||
| describe('Ownable', function () { | ||
| beforeEach(async function () { | ||
| await loadFixture(fixture).then(results => Object.assign(this, results)); | ||
| }); | ||
Amxx marked this conversation as resolved.
Show resolved
Hide resolved
|
||
|
|
||
| it('rejects zero address for initialOwner', async function () { | ||
| await expect(ethers.deployContract('$Ownable', [ethers.ZeroAddress])) | ||
| .to.be.revertedWithCustomError(this.ownable, 'OwnableInvalidOwner') | ||
| .withArgs(ethers.ZeroAddress); | ||
Amxx marked this conversation as resolved.
Show resolved
Hide resolved
|
||
| }); | ||
|
|
||
| it('has an owner', async function () { | ||
| expect(await this.ownable.owner()).to.equal(this.owner.address); | ||
| }); | ||
|
|
||
| describe('transfer ownership', function () { | ||
| it('changes owner after transfer', async function () { | ||
| await expect(this.ownable.connect(this.owner).transferOwnership(this.other)) | ||
| .to.emit(this.ownable, 'OwnershipTransferred') | ||
| .withArgs(this.owner.address, this.other.address); | ||
|
|
||
| expect(await this.ownable.owner()).to.equal(this.other.address); | ||
| }); | ||
|
There was a problem hiding this comment. Using Are there any workarounds? Is it worth it to contribute to Hardhat to improve this? There was a problem hiding this comment. I once wrote an helper that is const getAddress = (account : string | { address: string }) : string => account.address ?? account;but calling it everywhere is even worst. Maybe it could be integrated by default ... There was a problem hiding this comment.
There was a problem hiding this comment. Note: I'd love to simplify the code by removing There was a problem hiding this comment. Agree with migrating using There was a problem hiding this comment. For anyone curious, this PR NomicFoundation/hardhat#4449 resolves the issue in hardhat-chai-matchers |
||
|
|
||
| it('prevents non-owners from transferring', async function () { | ||
| await expect(this.ownable.connect(this.other).transferOwnership(this.other)) | ||
| .to.be.revertedWithCustomError(this.ownable, 'OwnableUnauthorizedAccount') | ||
| .withArgs(this.other.address); | ||
| }); | ||
|
|
||
| it('guards ownership against stuck state', async function () { | ||
| await expect(this.ownable.connect(this.owner).transferOwnership(ethers.ZeroAddress)) | ||
| .to.be.revertedWithCustomError(this.ownable, 'OwnableInvalidOwner') | ||
| .withArgs(ethers.ZeroAddress); | ||
| }); | ||
| }); | ||
|
|
||
| describe('renounce ownership', function () { | ||
| it('loses ownership after renouncement', async function () { | ||
| await expect(this.ownable.connect(this.owner).renounceOwnership()) | ||
| .to.emit(this.ownable, 'OwnershipTransferred') | ||
| .withArgs(this.owner.address, ethers.ZeroAddress); | ||
|
|
||
| expect(await this.ownable.owner()).to.equal(ethers.ZeroAddress); | ||
| }); | ||
|
|
||
| it('prevents non-owners from renouncement', async function () { | ||
| await expect(this.ownable.connect(this.other).renounceOwnership()) | ||
| .to.be.revertedWithCustomError(this.ownable, 'OwnableUnauthorizedAccount') | ||
| .withArgs(this.other.address); | ||
| }); | ||
|
|
||
| it('allows to recover access using the internal _transferOwnership', async function () { | ||
| await this.ownable.connect(this.owner).renounceOwnership(); | ||
|
|
||
| await expect(this.ownable.$_transferOwnership(this.other)) | ||
| .to.emit(this.ownable, 'OwnershipTransferred') | ||
| .withArgs(ethers.ZeroAddress, this.other.address); | ||
|
|
||
| expect(await this.ownable.owner()).to.equal(this.other.address); | ||
| }); | ||
| }); | ||
| }); | ||
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,70 +1,83 @@ | ||
| const { ethers } = require('hardhat'); | ||
| const { expect } = require('chai'); | ||
| const { loadFixture } = require('@nomicfoundation/hardhat-network-helpers'); | ||
|
|
||
| async function fixture() { | ||
| const accounts = await ethers.getSigners(); | ||
| const owner = accounts.shift(); | ||
| const accountA = accounts.shift(); | ||
| const accountB = accounts.shift(); | ||
| const ownable2Step = await ethers.deployContract('$Ownable2Step', [owner]); | ||
| return { accounts, owner, accountA, accountB, ownable2Step }; | ||
| } | ||
|
|
||
| describe('Ownable2Step', function () { | ||
| beforeEach(async function () { | ||
| await loadFixture(fixture).then(results => Object.assign(this, results)); | ||
| }); | ||
|
|
||
| describe('transfer ownership', function () { | ||
| it('starting a transfer does not change owner', async function () { | ||
| await expect(this.ownable2Step.connect(this.owner).transferOwnership(this.accountA)) | ||
| .to.emit(this.ownable2Step, 'OwnershipTransferStarted') | ||
| .withArgs(this.owner.address, this.accountA.address); | ||
|
|
||
| expect(await this.ownable2Step.owner()).to.equal(this.owner.address); | ||
| expect(await this.ownable2Step.pendingOwner()).to.equal(this.accountA.address); | ||
| }); | ||
|
|
||
| it('changes owner after transfer', async function () { | ||
| await this.ownable2Step.connect(this.owner).transferOwnership(this.accountA); | ||
|
|
||
| await expect(this.ownable2Step.connect(this.accountA).acceptOwnership()) | ||
| .to.emit(this.ownable2Step, 'OwnershipTransferred') | ||
| .withArgs(this.owner.address, this.accountA.address); | ||
|
|
||
| expect(await this.ownable2Step.owner()).to.equal(this.accountA.address); | ||
| expect(await this.ownable2Step.pendingOwner()).to.equal(ethers.ZeroAddress); | ||
| }); | ||
|
|
||
| it('guards transfer against invalid user', async function () { | ||
| await this.ownable2Step.connect(this.owner).transferOwnership(this.accountA); | ||
|
|
||
| await expect(this.ownable2Step.connect(this.accountB).acceptOwnership()) | ||
| .to.be.revertedWithCustomError(this.ownable2Step, 'OwnableUnauthorizedAccount') | ||
| .withArgs(this.accountB.address); | ||
| }); | ||
| }); | ||
|
|
||
| describe('renouncing ownership', async function () { | ||
| it('changes owner after renouncing ownership', async function () { | ||
| await expect(this.ownable2Step.connect(this.owner).renounceOwnership()) | ||
| .to.emit(this.ownable2Step, 'OwnershipTransferred') | ||
| .withArgs(this.owner.address, ethers.ZeroAddress); | ||
|
|
||
| // If renounceOwnership is removed from parent an alternative is needed ... | ||
| // without it is difficult to cleanly renounce with the two step process | ||
| // see: https://github.com/OpenZeppelin/openzeppelin-contracts/pull/3620#discussion_r957930388 | ||
| expect(await this.ownable2Step.owner()).to.equal(ethers.ZeroAddress); | ||
| }); | ||
|
|
||
| it('pending owner resets after renouncing ownership', async function () { | ||
| await this.ownable2Step.connect(this.owner).transferOwnership(this.accountA); | ||
| expect(await this.ownable2Step.pendingOwner()).to.equal(this.accountA.address); | ||
|
|
||
| await this.ownable2Step.connect(this.owner).renounceOwnership(); | ||
| expect(await this.ownable2Step.pendingOwner()).to.equal(ethers.ZeroAddress); | ||
|
|
||
| await expect(this.ownable2Step.connect(this.accountA).acceptOwnership()) | ||
| .to.be.revertedWithCustomError(this.ownable2Step, 'OwnableUnauthorizedAccount') | ||
| .withArgs(this.accountA.address); | ||
| }); | ||
|
|
||
| it('allows to recover access using the internal _transferOwnership', async function () { | ||
| await this.ownable2Step.connect(this.owner).renounceOwnership(); | ||
|
|
||
| await expect(this.ownable2Step.$_transferOwnership(this.accountA)) | ||
| .to.emit(this.ownable2Step, 'OwnershipTransferred') | ||
| .withArgs(ethers.ZeroAddress, this.accountA.address); | ||
|
|
||
| expect(await this.ownable2Step.owner()).to.equal(this.accountA.address); | ||
| }); | ||
| }); | ||
| }); |
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,22 +1,6 @@ | ||
| const { ethers } = require('hardhat'); | ||
|
|
||
| module.exports = { | ||
| computeCreateAddress: (from, nonce) => ethers.getCreateAddress({ from, nonce }), | ||
| computeCreate2Address: (salt, bytecode, from) => ethers.getCreate2Address(from, salt, ethers.keccak256(bytecode)), | ||
| }; |
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
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.
We could cache it in a global helper. But it should run only once per contract test suite anyway.
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 is easily cacheable in
env.contract.jsThere 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.
Do you know how this will behave with ethers?
Would it work if in the tests we do
this.accounts.shift()?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 was thinking in something like:
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.
And we would get the signers as an argument of
contract("...", accounts => { ... });?
That makes sens, but it will also break the existing truffle tests, which we don't want.
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.
Too maintenance heavy (and error prone) IMO.
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.
If loadFixture does a "per contract suite" snapshot, I don't see why we would have to also do a "cross contract suite" snapshot. That is making everything slower, and I really don't see what it brings in exchange. The only usecase I see for that, is if we wanted our test to all start with
blockNumber = 0... and I don't think that is an assumption we should make. IMO our tests (each test.js file) should work regardless of the initial state of the chain.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.
we could do
And then do
and
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.
Keeping the high-level snapshot makes sense if not all the contract suites implement a fixture. Let's try removing the high-level snapshot once we finish with the migration as well.
I like this, let's implement it that way 😄
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.
Side note, I also added a comment in #4658 to keep track of things we should do to consolidate the migration once done