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

Cleanup of Ownership directory (#2120)

Browse Source 
* Remove Ownable.isOwner.

* Remove ownable behavior from tests

* Make Escrow use Ownable instead of Secondary

* Migrate GSNRecipientERC20Fee to Ownable

* Remove Secondary

* Move Ownable to access directory

* Remove mentions of Secondary

* Add changelog entry

* Move Ownable tests to access

* Remove unused mock
This commit is contained in:
Nicolás Venturo
2020-03-16 15:12:29 -03:00
committed by GitHub
parent baaadde3c5
commit 8176a901a9
20 changed files with 144 additions and 315 deletions
Download Patch File Download Diff File Expand all files Collapse all files

54
test/access/Ownable.test.js Normal file
View File

@ -0,0 +1,54 @@
const { accounts, contract } = require('@openzeppelin/test-environment');
const { constants, expectEvent, expectRevert } = require('@openzeppelin/test-helpers');
const { ZERO_ADDRESS } = constants;
const { expect } = require('chai');
const Ownable = contract.fromArtifact('OwnableMock');
describe('Ownable', function () {
const [ owner, other ] = accounts;
beforeEach(async function () {
this.ownable = await Ownable.new({ from: owner });
});
it('should have an owner', async function () {
expect(await this.ownable.owner()).to.equal(owner);
});
it('changes owner after transfer', async function () {
const receipt = await this.ownable.transferOwnership(other, { from: owner });
expectEvent(receipt, 'OwnershipTransferred');
expect(await this.ownable.owner()).to.equal(other);
});
it('should prevent non-owners from transferring', async function () {
await expectRevert(
this.ownable.transferOwnership(other, { from: other }),
'Ownable: caller is not the owner'
);
});
it('should guard ownership against stuck state', async function () {
await expectRevert(
this.ownable.transferOwnership(ZERO_ADDRESS, { from: owner }),
'Ownable: new owner is the zero address'
);
});
it('loses owner after renouncement', async function () {
const receipt = await this.ownable.renounceOwnership({ from: owner });
expectEvent(receipt, 'OwnershipTransferred');
expect(await this.ownable.owner()).to.equal(ZERO_ADDRESS);
});
it('should prevent non-owners from renouncement', async function () {
await expectRevert(
this.ownable.renounceOwnership({ from: other }),
'Ownable: caller is not the owner'
);
});
});

53
test/ownership/Ownable.behavior.js
View File

@ -1,53 +0,0 @@
const { constants, expectEvent, expectRevert } = require('@openzeppelin/test-helpers');
const { ZERO_ADDRESS } = constants;
const { expect } = require('chai');
function shouldBehaveLikeOwnable (owner, [other]) {
describe('as an ownable', function () {
it('should have an owner', async function () {
expect(await this.ownable.owner()).to.equal(owner);
});
it('changes owner after transfer', async function () {
expect(await this.ownable.isOwner({ from: other })).to.equal(false);
const receipt = await this.ownable.transferOwnership(other, { from: owner });
expectEvent(receipt, 'OwnershipTransferred');
expect(await this.ownable.owner()).to.equal(other);
expect(await this.ownable.isOwner({ from: other })).to.equal(true);
});
it('should prevent non-owners from transferring', async function () {
await expectRevert(
this.ownable.transferOwnership(other, { from: other }),
'Ownable: caller is not the owner'
);
});
it('should guard ownership against stuck state', async function () {
await expectRevert(
this.ownable.transferOwnership(ZERO_ADDRESS, { from: owner }),
'Ownable: new owner is the zero address'
);
});
it('loses owner after renouncement', async function () {
const receipt = await this.ownable.renounceOwnership({ from: owner });
expectEvent(receipt, 'OwnershipTransferred');
expect(await this.ownable.owner()).to.equal(ZERO_ADDRESS);
});
it('should prevent non-owners from renouncement', async function () {
await expectRevert(
this.ownable.renounceOwnership({ from: other }),
'Ownable: caller is not the owner'
);
});
});
}
module.exports = {
shouldBehaveLikeOwnable,
};

16
test/ownership/Ownable.test.js
View File

@ -1,16 +0,0 @@
const { accounts, contract } = require('@openzeppelin/test-environment');
require('@openzeppelin/test-helpers');
const { shouldBehaveLikeOwnable } = require('./Ownable.behavior');
const Ownable = contract.fromArtifact('OwnableMock');
describe('Ownable', function () {
const [ owner, ...otherAccounts ] = accounts;
beforeEach(async function () {
this.ownable = await Ownable.new({ from: owner });
});
shouldBehaveLikeOwnable(owner, otherAccounts);
});

68
test/ownership/Secondary.test.js
View File

@ -1,68 +0,0 @@
const { accounts, contract } = require('@openzeppelin/test-environment');
const { constants, expectEvent, expectRevert } = require('@openzeppelin/test-helpers');
const { ZERO_ADDRESS } = constants;
const { expect } = require('chai');
const SecondaryMock = contract.fromArtifact('SecondaryMock');
describe('Secondary', function () {
const [ primary, newPrimary, other ] = accounts;
beforeEach(async function () {
this.secondary = await SecondaryMock.new({ from: primary });
});
it('stores the primary\'s address', async function () {
expect(await this.secondary.primary()).to.equal(primary);
});
describe('onlyPrimary', function () {
it('allows the primary account to call onlyPrimary functions', async function () {
await this.secondary.onlyPrimaryMock({ from: primary });
});
it('reverts when anyone calls onlyPrimary functions', async function () {
await expectRevert(this.secondary.onlyPrimaryMock({ from: other }),
'Secondary: caller is not the primary account'
);
});
});
describe('transferPrimary', function () {
it('makes the recipient the new primary', async function () {
const { logs } = await this.secondary.transferPrimary(newPrimary, { from: primary });
expectEvent.inLogs(logs, 'PrimaryTransferred', { recipient: newPrimary });
expect(await this.secondary.primary()).to.equal(newPrimary);
});
it('reverts when transferring to the null address', async function () {
await expectRevert(this.secondary.transferPrimary(ZERO_ADDRESS, { from: primary }),
'Secondary: new primary is the zero address'
);
});
it('reverts when called by anyone', async function () {
await expectRevert(this.secondary.transferPrimary(newPrimary, { from: other }),
'Secondary: caller is not the primary account'
);
});
context('with new primary', function () {
beforeEach(async function () {
await this.secondary.transferPrimary(newPrimary, { from: primary });
});
it('allows the new primary account to call onlyPrimary functions', async function () {
await this.secondary.onlyPrimaryMock({ from: newPrimary });
});
it('reverts when the old primary account calls onlyPrimary functions', async function () {
await expectRevert(this.secondary.onlyPrimaryMock({ from: primary }),
'Secondary: caller is not the primary account'
);
});
});
});
});

34
test/payment/escrow/Escrow.behavior.js
View File

@ -2,13 +2,13 @@ const { balance, ether, expectEvent, expectRevert } = require('@openzeppelin/tes
const { expect } = require('chai');
function shouldBehaveLikeEscrow (primary, [payee1, payee2]) {
function shouldBehaveLikeEscrow (owner, [payee1, payee2]) {
const amount = ether('42');
describe('as an escrow', function () {
describe('deposits', function () {
it('can accept a single deposit', async function () {
await this.escrow.deposit(payee1, { from: primary, value: amount });
await this.escrow.deposit(payee1, { from: owner, value: amount });
expect(await balance.current(this.escrow.address)).to.be.bignumber.equal(amount);
@ -16,17 +16,17 @@ function shouldBehaveLikeEscrow (primary, [payee1, payee2]) {
});
it('can accept an empty deposit', async function () {
await this.escrow.deposit(payee1, { from: primary, value: 0 });
await this.escrow.deposit(payee1, { from: owner, value: 0 });
});
it('only the primary account can deposit', async function () {
it('only the owner can deposit', async function () {
await expectRevert(this.escrow.deposit(payee1, { from: payee2 }),
'Secondary: caller is not the primary account'
'Ownable: caller is not the owner'
);
});
it('emits a deposited event', async function () {
const { logs } = await this.escrow.deposit(payee1, { from: primary, value: amount });
const { logs } = await this.escrow.deposit(payee1, { from: owner, value: amount });
expectEvent.inLogs(logs, 'Deposited', {
payee: payee1,
weiAmount: amount,
@ -34,8 +34,8 @@ function shouldBehaveLikeEscrow (primary, [payee1, payee2]) {
});
it('can add multiple deposits on a single account', async function () {
await this.escrow.deposit(payee1, { from: primary, value: amount });
await this.escrow.deposit(payee1, { from: primary, value: amount.muln(2) });
await this.escrow.deposit(payee1, { from: owner, value: amount });
await this.escrow.deposit(payee1, { from: owner, value: amount.muln(2) });
expect(await balance.current(this.escrow.address)).to.be.bignumber.equal(amount.muln(3));
@ -43,8 +43,8 @@ function shouldBehaveLikeEscrow (primary, [payee1, payee2]) {
});
it('can track deposits to multiple accounts', async function () {
await this.escrow.deposit(payee1, { from: primary, value: amount });
await this.escrow.deposit(payee2, { from: primary, value: amount.muln(2) });
await this.escrow.deposit(payee1, { from: owner, value: amount });
await this.escrow.deposit(payee2, { from: owner, value: amount.muln(2) });
expect(await balance.current(this.escrow.address)).to.be.bignumber.equal(amount.muln(3));
@ -58,8 +58,8 @@ function shouldBehaveLikeEscrow (primary, [payee1, payee2]) {
it('can withdraw payments', async function () {
const balanceTracker = await balance.tracker(payee1);
await this.escrow.deposit(payee1, { from: primary, value: amount });
await this.escrow.withdraw(payee1, { from: primary });
await this.escrow.deposit(payee1, { from: owner, value: amount });
await this.escrow.withdraw(payee1, { from: owner });
expect(await balanceTracker.delta()).to.be.bignumber.equal(amount);
@ -68,18 +68,18 @@ function shouldBehaveLikeEscrow (primary, [payee1, payee2]) {
});
it('can do an empty withdrawal', async function () {
await this.escrow.withdraw(payee1, { from: primary });
await this.escrow.withdraw(payee1, { from: owner });
});
it('only the primary account can withdraw', async function () {
it('only the owner can withdraw', async function () {
await expectRevert(this.escrow.withdraw(payee1, { from: payee1 }),
'Secondary: caller is not the primary account'
'Ownable: caller is not the owner'
);
});
it('emits a withdrawn event', async function () {
await this.escrow.deposit(payee1, { from: primary, value: amount });
const { logs } = await this.escrow.withdraw(payee1, { from: primary });
await this.escrow.deposit(payee1, { from: owner, value: amount });
const { logs } = await this.escrow.withdraw(payee1, { from: owner });
expectEvent.inLogs(logs, 'Withdrawn', {
payee: payee1,
weiAmount: amount,

6
test/payment/escrow/Escrow.test.js
View File

@ -6,11 +6,11 @@ const { shouldBehaveLikeEscrow } = require('./Escrow.behavior');
const Escrow = contract.fromArtifact('Escrow');
describe('Escrow', function () {
const [ primary, ...otherAccounts ] = accounts;
const [ owner, ...otherAccounts ] = accounts;
beforeEach(async function () {
this.escrow = await Escrow.new({ from: primary });
this.escrow = await Escrow.new({ from: owner });
});
shouldBehaveLikeEscrow(primary, otherAccounts);
shouldBehaveLikeEscrow(owner, otherAccounts);
});

46
test/payment/escrow/RefundEscrow.test.js
View File

@ -8,20 +8,20 @@ const { expect } = require('chai');
const RefundEscrow = contract.fromArtifact('RefundEscrow');
describe('RefundEscrow', function () {
const [ primary, beneficiary, refundee1, refundee2 ] = accounts;
const [ owner, beneficiary, refundee1, refundee2 ] = accounts;
const amount = ether('54');
const refundees = [refundee1, refundee2];
it('requires a non-null beneficiary', async function () {
await expectRevert(
RefundEscrow.new(ZERO_ADDRESS, { from: primary }), 'RefundEscrow: beneficiary is the zero address'
RefundEscrow.new(ZERO_ADDRESS, { from: owner }), 'RefundEscrow: beneficiary is the zero address'
);
});
context('once deployed', function () {
beforeEach(async function () {
this.escrow = await RefundEscrow.new(beneficiary, { from: primary });
this.escrow = await RefundEscrow.new(beneficiary, { from: owner });
});
context('active state', function () {
@ -31,44 +31,44 @@ describe('RefundEscrow', function () {
});
it('accepts deposits', async function () {
await this.escrow.deposit(refundee1, { from: primary, value: amount });
await this.escrow.deposit(refundee1, { from: owner, value: amount });
expect(await this.escrow.depositsOf(refundee1)).to.be.bignumber.equal(amount);
});
it('does not refund refundees', async function () {
await this.escrow.deposit(refundee1, { from: primary, value: amount });
await this.escrow.deposit(refundee1, { from: owner, value: amount });
await expectRevert(this.escrow.withdraw(refundee1),
'ConditionalEscrow: payee is not allowed to withdraw'
);
});
it('does not allow beneficiary withdrawal', async function () {
await this.escrow.deposit(refundee1, { from: primary, value: amount });
await this.escrow.deposit(refundee1, { from: owner, value: amount });
await expectRevert(this.escrow.beneficiaryWithdraw(),
'RefundEscrow: beneficiary can only withdraw while closed'
);
});
});
it('only the primary account can enter closed state', async function () {
it('only the owner can enter closed state', async function () {
await expectRevert(this.escrow.close({ from: beneficiary }),
'Secondary: caller is not the primary account'
'Ownable: caller is not the owner'
);
const { logs } = await this.escrow.close({ from: primary });
const { logs } = await this.escrow.close({ from: owner });
expectEvent.inLogs(logs, 'RefundsClosed');
});
context('closed state', function () {
beforeEach(async function () {
await Promise.all(refundees.map(refundee => this.escrow.deposit(refundee, { from: primary, value: amount })));
await Promise.all(refundees.map(refundee => this.escrow.deposit(refundee, { from: owner, value: amount })));
await this.escrow.close({ from: primary });
await this.escrow.close({ from: owner });
});
it('rejects deposits', async function () {
await expectRevert(this.escrow.deposit(refundee1, { from: primary, value: amount }),
await expectRevert(this.escrow.deposit(refundee1, { from: owner, value: amount }),
'RefundEscrow: can only deposit while active'
);
});
@ -86,36 +86,36 @@ describe('RefundEscrow', function () {
});
it('prevents entering the refund state', async function () {
await expectRevert(this.escrow.enableRefunds({ from: primary }),
await expectRevert(this.escrow.enableRefunds({ from: owner }),
'RefundEscrow: can only enable refunds while active'
);
});
it('prevents re-entering the closed state', async function () {
await expectRevert(this.escrow.close({ from: primary }),
await expectRevert(this.escrow.close({ from: owner }),
'RefundEscrow: can only close while active'
);
});
});
it('only the primary account can enter refund state', async function () {
it('only the owner can enter refund state', async function () {
await expectRevert(this.escrow.enableRefunds({ from: beneficiary }),
'Secondary: caller is not the primary account'
'Ownable: caller is not the owner'
);
const { logs } = await this.escrow.enableRefunds({ from: primary });
const { logs } = await this.escrow.enableRefunds({ from: owner });
expectEvent.inLogs(logs, 'RefundsEnabled');
});
context('refund state', function () {
beforeEach(async function () {
await Promise.all(refundees.map(refundee => this.escrow.deposit(refundee, { from: primary, value: amount })));
await Promise.all(refundees.map(refundee => this.escrow.deposit(refundee, { from: owner, value: amount })));
await this.escrow.enableRefunds({ from: primary });
await this.escrow.enableRefunds({ from: owner });
});
it('rejects deposits', async function () {
await expectRevert(this.escrow.deposit(refundee1, { from: primary, value: amount }),
await expectRevert(this.escrow.deposit(refundee1, { from: owner, value: amount }),
'RefundEscrow: can only deposit while active'
);
});
@ -123,7 +123,7 @@ describe('RefundEscrow', function () {
it('refunds refundees', async function () {
for (const refundee of [refundee1, refundee2]) {
const balanceTracker = await balance.tracker(refundee);
await this.escrow.withdraw(refundee, { from: primary });
await this.escrow.withdraw(refundee, { from: owner });
expect(await balanceTracker.delta()).to.be.bignumber.equal(amount);
}
});
@ -135,13 +135,13 @@ describe('RefundEscrow', function () {
});
it('prevents entering the closed state', async function () {
await expectRevert(this.escrow.close({ from: primary }),
await expectRevert(this.escrow.close({ from: owner }),
'RefundEscrow: can only close while active'
);
});
it('prevents re-entering the refund state', async function () {
await expectRevert(this.escrow.enableRefunds({ from: primary }),
await expectRevert(this.escrow.enableRefunds({ from: owner }),
'RefundEscrow: can only enable refunds while active'
);
});