NoBey/openzeppelin-contracts
1
0
Fork 0
Code Issues Pull Requests Actions Packages Projects Releases Wiki Activity

Replace revert strings with custom errors (#4261)

Browse Source 
Co-authored-by: Hadrien Croubois <hadrien.croubois@gmail.com>
Co-authored-by: Francisco <fg@frang.io>
This commit is contained in:
Ernesto García
2023-06-12 17:41:52 -06:00
committed by GitHub
parent 08fd777f6d
commit b425a72240
138 changed files with 3220 additions and 1287 deletions
Download Patch File Download Diff File Expand all files Collapse all files

126
test/access/AccessControl.behavior.js
View File

@ -1,4 +1,5 @@
const { expectEvent, expectRevert, constants, BN } = require('@openzeppelin/test-helpers');
const { expectEvent, constants, BN } = require('@openzeppelin/test-helpers');
const { expectRevertCustomError } = require('../helpers/customError');
const { expect } = require('chai');
const { time } = require('@nomicfoundation/hardhat-network-helpers');
@ -12,7 +13,7 @@ const ROLE = web3.utils.soliditySha3('ROLE');
const OTHER_ROLE = web3.utils.soliditySha3('OTHER_ROLE');
const ZERO = web3.utils.toBN(0);
function shouldBehaveLikeAccessControl(errorPrefix, admin, authorized, other, otherAdmin) {
function shouldBehaveLikeAccessControl(admin, authorized, other, otherAdmin) {
shouldSupportInterfaces(['AccessControl']);
describe('default admin', function () {
@ -35,9 +36,10 @@ function shouldBehaveLikeAccessControl(errorPrefix, admin, authorized, other, ot
});
it('non-admin cannot grant role to other accounts', async function () {
await expectRevert(
await expectRevertCustomError(
this.accessControl.grantRole(ROLE, authorized, { from: other }),
`${errorPrefix}: account ${other.toLowerCase()} is missing role ${DEFAULT_ADMIN_ROLE}`,
'AccessControlUnauthorizedAccount',
[other, DEFAULT_ADMIN_ROLE],
);
});
@ -69,9 +71,10 @@ function shouldBehaveLikeAccessControl(errorPrefix, admin, authorized, other, ot
});
it('non-admin cannot revoke role', async function () {
await expectRevert(
await expectRevertCustomError(
this.accessControl.revokeRole(ROLE, authorized, { from: other }),
`${errorPrefix}: account ${other.toLowerCase()} is missing role ${DEFAULT_ADMIN_ROLE}`,
'AccessControlUnauthorizedAccount',
[other, DEFAULT_ADMIN_ROLE],
);
});
@ -103,9 +106,10 @@ function shouldBehaveLikeAccessControl(errorPrefix, admin, authorized, other, ot
});
it('only the sender can renounce their roles', async function () {
await expectRevert(
await expectRevertCustomError(
this.accessControl.renounceRole(ROLE, authorized, { from: admin }),
`${errorPrefix}: can only renounce roles for self`,
'AccessControlBadConfirmation',
[],
);
});
@ -146,16 +150,18 @@ function shouldBehaveLikeAccessControl(errorPrefix, admin, authorized, other, ot
});
it("a role's previous admins no longer grant roles", async function () {
await expectRevert(
await expectRevertCustomError(
this.accessControl.grantRole(ROLE, authorized, { from: admin }),
`${errorPrefix}: account ${admin.toLowerCase()} is missing role ${OTHER_ROLE}`,
'AccessControlUnauthorizedAccount',
[admin.toLowerCase(), OTHER_ROLE],
);
});
it("a role's previous admins no longer revoke roles", async function () {
await expectRevert(
await expectRevertCustomError(
this.accessControl.revokeRole(ROLE, authorized, { from: admin }),
`${errorPrefix}: account ${admin.toLowerCase()} is missing role ${OTHER_ROLE}`,
'AccessControlUnauthorizedAccount',
[admin.toLowerCase(), OTHER_ROLE],
);
});
});
@ -170,22 +176,24 @@ function shouldBehaveLikeAccessControl(errorPrefix, admin, authorized, other, ot
});
it("revert if sender doesn't have role #1", async function () {
await expectRevert(
await expectRevertCustomError(
this.accessControl.methods['$_checkRole(bytes32)'](ROLE, { from: other }),
`${errorPrefix}: account ${other.toLowerCase()} is missing role ${ROLE}`,
'AccessControlUnauthorizedAccount',
[other, ROLE],
);
});
it("revert if sender doesn't have role #2", async function () {
await expectRevert(
await expectRevertCustomError(
this.accessControl.methods['$_checkRole(bytes32)'](OTHER_ROLE, { from: authorized }),
`${errorPrefix}: account ${authorized.toLowerCase()} is missing role ${OTHER_ROLE}`,
'AccessControlUnauthorizedAccount',
[authorized.toLowerCase(), OTHER_ROLE],
);
});
});
}
function shouldBehaveLikeAccessControlEnumerable(errorPrefix, admin, authorized, other, otherAdmin, otherAuthorized) {
function shouldBehaveLikeAccessControlEnumerable(admin, authorized, other, otherAdmin, otherAuthorized) {
shouldSupportInterfaces(['AccessControlEnumerable']);
describe('enumerating', function () {
@ -215,18 +223,9 @@ function shouldBehaveLikeAccessControlEnumerable(errorPrefix, admin, authorized,
});
}
function shouldBehaveLikeAccessControlDefaultAdminRules(errorPrefix, delay, defaultAdmin, newDefaultAdmin, other) {
function shouldBehaveLikeAccessControlDefaultAdminRules(delay, defaultAdmin, newDefaultAdmin, other) {
shouldSupportInterfaces(['AccessControlDefaultAdminRules']);
function expectNoEvent(receipt, eventName) {
try {
expectEvent(receipt, eventName);
throw new Error(`${eventName} event found`);
} catch (err) {
expect(err.message).to.eq(`No '${eventName}' events found: expected false to equal true`);
}
}
for (const getter of ['owner', 'defaultAdmin']) {
describe(`${getter}()`, function () {
it('has a default set to the initial default admin', async function () {
@ -366,30 +365,34 @@ function shouldBehaveLikeAccessControlDefaultAdminRules(errorPrefix, delay, defa
});
it('should revert if granting default admin role', async function () {
await expectRevert(
await expectRevertCustomError(
this.accessControl.grantRole(DEFAULT_ADMIN_ROLE, defaultAdmin, { from: defaultAdmin }),
`${errorPrefix}: can't directly grant default admin role`,
'AccessControlEnforcedDefaultAdminRules',
[],
);
});
it('should revert if revoking default admin role', async function () {
await expectRevert(
await expectRevertCustomError(
this.accessControl.revokeRole(DEFAULT_ADMIN_ROLE, defaultAdmin, { from: defaultAdmin }),
`${errorPrefix}: can't directly revoke default admin role`,
'AccessControlEnforcedDefaultAdminRules',
[],
);
});
it("should revert if defaultAdmin's admin is changed", async function () {
await expectRevert(
await expectRevertCustomError(
this.accessControl.$_setRoleAdmin(DEFAULT_ADMIN_ROLE, defaultAdmin),
`${errorPrefix}: can't violate default admin rules`,
'AccessControlEnforcedDefaultAdminRules',
[],
);
});
it('should not grant the default admin role twice', async function () {
await expectRevert(
await expectRevertCustomError(
this.accessControl.$_grantRole(DEFAULT_ADMIN_ROLE, defaultAdmin),
`${errorPrefix}: default admin already granted`,
'AccessControlEnforcedDefaultAdminRules',
[],
);
});
@ -398,9 +401,10 @@ function shouldBehaveLikeAccessControlDefaultAdminRules(errorPrefix, delay, defa
let acceptSchedule;
it('reverts if called by non default admin accounts', async function () {
await expectRevert(
await expectRevertCustomError(
this.accessControl.beginDefaultAdminTransfer(newDefaultAdmin, { from: other }),
`${errorPrefix}: account ${other.toLowerCase()} is missing role ${DEFAULT_ADMIN_ROLE}`,
'AccessControlUnauthorizedAccount',
[other, DEFAULT_ADMIN_ROLE],
);
});
@ -456,7 +460,7 @@ function shouldBehaveLikeAccessControlDefaultAdminRules(errorPrefix, delay, defa
await this.accessControl.acceptDefaultAdminTransfer({ from: newDefaultAdmin });
const receipt = await this.accessControl.beginDefaultAdminTransfer(other, { from: newDefaultAdmin });
expectNoEvent(receipt, 'DefaultAdminTransferCanceled');
expectEvent.notEmitted(receipt, 'DefaultAdminTransferCanceled');
});
});
@ -506,9 +510,10 @@ function shouldBehaveLikeAccessControlDefaultAdminRules(errorPrefix, delay, defa
it('should revert if caller is not pending default admin', async function () {
await time.setNextBlockTimestamp(acceptSchedule.addn(1));
await expectRevert(
await expectRevertCustomError(
this.accessControl.acceptDefaultAdminTransfer({ from: other }),
`${errorPrefix}: pending admin must accept`,
'AccessControlInvalidDefaultAdmin',
[other],
);
});
@ -549,9 +554,10 @@ function shouldBehaveLikeAccessControlDefaultAdminRules(errorPrefix, delay, defa
]) {
it(`should revert if block.timestamp is ${tag} to schedule`, async function () {
await time.setNextBlockTimestamp(acceptSchedule.toNumber() + fromSchedule);
await expectRevert(
await expectRevertCustomError(
this.accessControl.acceptDefaultAdminTransfer({ from: newDefaultAdmin }),
`${errorPrefix}: transfer delay not passed`,
'AccessControlEnforcedDefaultAdminDelay',
[acceptSchedule],
);
});
}
@ -560,9 +566,10 @@ function shouldBehaveLikeAccessControlDefaultAdminRules(errorPrefix, delay, defa
describe('cancels a default admin transfer', function () {
it('reverts if called by non default admin accounts', async function () {
await expectRevert(
await expectRevertCustomError(
this.accessControl.cancelDefaultAdminTransfer({ from: other }),
`${errorPrefix}: account ${other.toLowerCase()} is missing role ${DEFAULT_ADMIN_ROLE}`,
'AccessControlUnauthorizedAccount',
[other, DEFAULT_ADMIN_ROLE],
);
});
@ -600,9 +607,10 @@ function shouldBehaveLikeAccessControlDefaultAdminRules(errorPrefix, delay, defa
await time.setNextBlockTimestamp(acceptSchedule.addn(1));
// Previous pending default admin should not be able to accept after cancellation.
await expectRevert(
await expectRevertCustomError(
this.accessControl.acceptDefaultAdminTransfer({ from: newDefaultAdmin }),
`${errorPrefix}: pending admin must accept`,
'AccessControlInvalidDefaultAdmin',
[newDefaultAdmin],
);
});
});
@ -615,7 +623,7 @@ function shouldBehaveLikeAccessControlDefaultAdminRules(errorPrefix, delay, defa
expect(newAdmin).to.equal(constants.ZERO_ADDRESS);
expect(schedule).to.be.bignumber.equal(ZERO);
expectNoEvent(receipt, 'DefaultAdminTransferCanceled');
expectEvent.notEmitted(receipt, 'DefaultAdminTransferCanceled');
});
});
});
@ -634,9 +642,10 @@ function shouldBehaveLikeAccessControlDefaultAdminRules(errorPrefix, delay, defa
it('reverts if caller is not default admin', async function () {
await time.setNextBlockTimestamp(delayPassed);
await expectRevert(
await expectRevertCustomError(
this.accessControl.renounceRole(DEFAULT_ADMIN_ROLE, other, { from: defaultAdmin }),
`${errorPrefix}: can only renounce roles for self`,
'AccessControlBadConfirmation',
[],
);
});
@ -693,9 +702,10 @@ function shouldBehaveLikeAccessControlDefaultAdminRules(errorPrefix, delay, defa
]) {
it(`reverts if block.timestamp is ${tag} to schedule`, async function () {
await time.setNextBlockTimestamp(delayNotPassed.toNumber() + fromSchedule);
await expectRevert(
await expectRevertCustomError(
this.accessControl.renounceRole(DEFAULT_ADMIN_ROLE, defaultAdmin, { from: defaultAdmin }),
`${errorPrefix}: only can renounce in two delayed steps`,
'AccessControlEnforcedDefaultAdminDelay',
[expectedSchedule],
);
});
}
@ -704,11 +714,12 @@ function shouldBehaveLikeAccessControlDefaultAdminRules(errorPrefix, delay, defa
describe('changes delay', function () {
it('reverts if called by non default admin accounts', async function () {
await expectRevert(
await expectRevertCustomError(
this.accessControl.changeDefaultAdminDelay(time.duration.hours(4), {
from: other,
}),
`${errorPrefix}: account ${other.toLowerCase()} is missing role ${DEFAULT_ADMIN_ROLE}`,
'AccessControlUnauthorizedAccount',
[other, DEFAULT_ADMIN_ROLE],
);
});
@ -792,7 +803,7 @@ function shouldBehaveLikeAccessControlDefaultAdminRules(errorPrefix, delay, defa
from: defaultAdmin,
});
const eventMatcher = passed ? expectNoEvent : expectEvent;
const eventMatcher = passed ? expectEvent.notEmitted : expectEvent;
eventMatcher(receipt, 'DefaultAdminDelayChangeCanceled');
});
}
@ -803,9 +814,10 @@ function shouldBehaveLikeAccessControlDefaultAdminRules(errorPrefix, delay, defa
describe('rollbacks a delay change', function () {
it('reverts if called by non default admin accounts', async function () {
await expectRevert(
await expectRevertCustomError(
this.accessControl.rollbackDefaultAdminDelay({ from: other }),
`${errorPrefix}: account ${other.toLowerCase()} is missing role ${DEFAULT_ADMIN_ROLE}`,
'AccessControlUnauthorizedAccount',
[other, DEFAULT_ADMIN_ROLE],
);
});
@ -841,7 +853,7 @@ function shouldBehaveLikeAccessControlDefaultAdminRules(errorPrefix, delay, defa
const receipt = await this.accessControl.rollbackDefaultAdminDelay({ from: defaultAdmin });
const eventMatcher = passed ? expectNoEvent : expectEvent;
const eventMatcher = passed ? expectEvent.notEmitted : expectEvent;
eventMatcher(receipt, 'DefaultAdminDelayChangeCanceled');
});
}

2
test/access/AccessControl.test.js
View File

@ -8,5 +8,5 @@ contract('AccessControl', function (accounts) {
await this.accessControl.$_grantRole(DEFAULT_ADMIN_ROLE, accounts[0]);
});
shouldBehaveLikeAccessControl('AccessControl', ...accounts);
shouldBehaveLikeAccessControl(...accounts);
});

7
test/access/AccessControlDefaultAdminRules.test.js
View File

@ -16,10 +16,11 @@ contract('AccessControlDefaultAdminRules', function (accounts) {
it('initial admin not zero', async function () {
await expectRevert(
AccessControlDefaultAdminRules.new(delay, constants.ZERO_ADDRESS),
'AccessControl: 0 default admin',
'AccessControlInvalidDefaultAdmin',
[constants.ZERO_ADDRESS],
);
});
shouldBehaveLikeAccessControl('AccessControl', ...accounts);
shouldBehaveLikeAccessControlDefaultAdminRules('AccessControl', delay, ...accounts);
shouldBehaveLikeAccessControl(...accounts);
shouldBehaveLikeAccessControlDefaultAdminRules(delay, ...accounts);
});

4
test/access/AccessControlEnumerable.test.js
View File

@ -12,6 +12,6 @@ contract('AccessControl', function (accounts) {
await this.accessControl.$_grantRole(DEFAULT_ADMIN_ROLE, accounts[0]);
});
shouldBehaveLikeAccessControl('AccessControl', ...accounts);
shouldBehaveLikeAccessControlEnumerable('AccessControl', ...accounts);
shouldBehaveLikeAccessControl(...accounts);
shouldBehaveLikeAccessControlEnumerable(...accounts);
});

19
test/access/Ownable.test.js
View File

@ -1,4 +1,6 @@
const { constants, expectEvent, expectRevert } = require('@openzeppelin/test-helpers');
const { constants, expectEvent } = require('@openzeppelin/test-helpers');
const { expectRevertCustomError } = require('../helpers/customError');
const { ZERO_ADDRESS } = constants;
const { expect } = require('chai');
@ -25,13 +27,18 @@ contract('Ownable', function (accounts) {
});
it('prevents non-owners from transferring', async function () {
await expectRevert(this.ownable.transferOwnership(other, { from: other }), 'Ownable: caller is not the owner');
await expectRevertCustomError(
this.ownable.transferOwnership(other, { from: other }),
'OwnableUnauthorizedAccount',
[other],
);
});
it('guards ownership against stuck state', async function () {
await expectRevert(
await expectRevertCustomError(
this.ownable.transferOwnership(ZERO_ADDRESS, { from: owner }),
'Ownable: new owner is the zero address',
'OwnableInvalidOwner',
[ZERO_ADDRESS],
);
});
});
@ -45,7 +52,9 @@ contract('Ownable', function (accounts) {
});
it('prevents non-owners from renouncement', async function () {
await expectRevert(this.ownable.renounceOwnership({ from: other }), 'Ownable: caller is not the owner');
await expectRevertCustomError(this.ownable.renounceOwnership({ from: other }), 'OwnableUnauthorizedAccount', [
other,
]);
});
it('allows to recover access using the internal _transferOwnership', async function () {

21
test/access/Ownable2Step.test.js
View File

@ -1,6 +1,7 @@
const { constants, expectEvent, expectRevert } = require('@openzeppelin/test-helpers');
const { constants, expectEvent } = require('@openzeppelin/test-helpers');
const { ZERO_ADDRESS } = constants;
const { expect } = require('chai');
const { expectRevertCustomError } = require('../helpers/customError');
const Ownable2Step = artifacts.require('$Ownable2Step');
@ -29,14 +30,15 @@ contract('Ownable2Step', function (accounts) {
it('guards transfer against invalid user', async function () {
await this.ownable2Step.transferOwnership(accountA, { from: owner });
await expectRevert(
await expectRevertCustomError(
this.ownable2Step.acceptOwnership({ from: accountB }),
'Ownable2Step: caller is not the new owner',
'OwnableUnauthorizedAccount',
[accountB],
);
});
});
it('renouncing ownership', async function () {
describe('renouncing ownership', async function () {
it('changes owner after renouncing ownership', async function () {
await this.ownable2Step.renounceOwnership({ from: owner });
// If renounceOwnership is removed from parent an alternative is needed ...
@ -50,18 +52,19 @@ contract('Ownable2Step', function (accounts) {
expect(await this.ownable2Step.pendingOwner()).to.equal(accountA);
await this.ownable2Step.renounceOwnership({ from: owner });
expect(await this.ownable2Step.pendingOwner()).to.equal(ZERO_ADDRESS);
await expectRevert(
await expectRevertCustomError(
this.ownable2Step.acceptOwnership({ from: accountA }),
'Ownable2Step: caller is not the new owner',
'OwnableUnauthorizedAccount',
[accountA],
);
});
it('allows to recover access using the internal _transferOwnership', async function () {
await this.ownable.renounceOwnership({ from: owner });
const receipt = await this.ownable.$_transferOwnership(accountA);
await this.ownable2Step.renounceOwnership({ from: owner });
const receipt = await this.ownable2Step.$_transferOwnership(accountA);
expectEvent(receipt, 'OwnershipTransferred');
expect(await this.ownable.owner()).to.equal(accountA);
expect(await this.ownable2Step.owner()).to.equal(accountA);
});
});
});