diff --git a/contracts/TokenManager.sol b/contracts/TokenManager.sol index 402cb1b..b9a4e83 100644 --- a/contracts/TokenManager.sol +++ b/contracts/TokenManager.sol @@ -22,9 +22,9 @@ contract TokenManager is Ownable { mapping (bytes32 => TokenEntry) private tokens; bytes32[] private names; - event TokenAdded(bytes32 _name); - event TokenDeleted(bytes32 _name); - event TokenUpgraded(bytes32 _name); + event TokenAdded(bytes32 name, ITokenX addr); + event TokenDeleted(bytes32 name, ITokenX addr); + event TokenUpgraded(bytes32 name, ITokenX from, ITokenX to); /** * @dev Require that the token _name exists @@ -70,7 +70,7 @@ contract TokenManager is Ownable { exists: true }); names.push(_name); - emit TokenAdded(_name); + emit TokenAdded(_name, _iTokenX); } /** @@ -82,10 +82,11 @@ contract TokenManager is Ownable { onlyOwner tokenExists(_name) { + ITokenX prev = tokens[_name].token; delete names[tokens[_name].index]; delete tokens[_name].token; delete tokens[_name]; - emit TokenDeleted(_name); + emit TokenDeleted(_name, prev); } /** @@ -99,8 +100,9 @@ contract TokenManager is Ownable { tokenExists(_name) notNullToken(_iTokenX) { + ITokenX prev = tokens[_name].token; tokens[_name].token = _iTokenX; - emit TokenUpgraded(_name); + emit TokenUpgraded(_name, prev, _iTokenX); } /** diff --git a/contracts/token/ERC20/ExternalERC20Storage.sol b/contracts/token/ERC20/ExternalERC20Storage.sol index bdcf157..3978e93 100644 --- a/contracts/token/ERC20/ExternalERC20Storage.sol +++ b/contracts/token/ERC20/ExternalERC20Storage.sol @@ -93,7 +93,7 @@ contract ExternalERC20Storage is Ownable { onlyImplementorOrOwner { require(newImplementor != _implementor, - "Cannot transfer to same implementor as existsing"); + "Cannot transfer to same implementor as existing"); address curImplementor = _implementor; _implementor = newImplementor; emit StorageImplementorTransferred(curImplementor, newImplementor); diff --git a/test/Accesslist.events.js b/test/Accesslist.events.js new file mode 100644 index 0000000..c8b211f --- /dev/null +++ b/test/Accesslist.events.js @@ -0,0 +1,21 @@ +const utils = require('./utils.js'); + +module.exports = utils.makeEventMap({ + // TODO: Implement the constructor using an inheritance system + constructor: (addr) => [{ eventName: 'OwnershipTransferred', + paramMap: { previousOwner: utils.ZERO_ADDRESS, + newOwner: addr } }, + { eventName: 'WhitelistAdminAdded', + paramMap: { account: addr } }, + { eventName: 'BlacklistAdminAdded', + paramMap: { account: addr } } + ], + addWhitelisted: (addr) => [{ eventName: 'WhitelistAdded', + paramMap: { account: addr } }], + removeWhitelisted: (addr) => [{ eventName: 'WhitelistRemoved', + paramMap: { account: addr } }], + addBlacklisted: (addr) => [{ eventName: 'BlacklistAdded', + paramMap: { account: addr } }], + removeBlacklisted: (addr) => [{ eventName: 'BlacklistRemoved', + paramMap: { account: addr } }] +}); diff --git a/test/Accesslist.test.js b/test/Accesslist.test.js index 68299bf..c5dc43b 100644 --- a/test/Accesslist.test.js +++ b/test/Accesslist.test.js @@ -4,205 +4,199 @@ 'use strict'; const util = require('./utils.js'); -const { inLogs } = require('openzeppelin-solidity/test/helpers/expectEvent.js'); -const truffleAssert = require('truffle-assertions'); const Accesslist = artifacts.require('Accesslist'); +const AccesslistE = require('./Accesslist.events.js'); const AccesslistGuardedMock = artifacts.require('AccesslistGuardedMock'); contract('Accesslist', async function ( [owner, user, user1, ...accounts] ) { let accesslist; - let accesslistGuardedMock; + let accesslistE; beforeEach(async function () { accesslist = await Accesslist.new(); - accesslistGuardedMock = - await AccesslistGuardedMock.new(accesslist.address, true); + accesslistE = AccesslistE.wrap(accesslist); }); describe('constructor', function () { - beforeEach(async function () { - const { logs } = - await truffleAssert.createTransactionResult( - accesslist, accesslist.transactionHash); - this.logs = logs; + it('emits events', async function () { + await accesslistE.constructor(accesslist, owner); }); - it('emits OwnershipTransferred event', function () { - inLogs(this.logs, 'OwnershipTransferred', - { previousOwner: util.ZERO_ADDRESS, - newOwner: owner }); + it('should reject null address accesslist', async function () { + await util.assertRevertsReason( + AccesslistGuardedMock.new(util.ZERO_ADDRESS, true), + 'Supplied accesslist is null'); }); + }); - it('emits BlacklistAdminAdded event', function () { - inLogs(this.logs, 'BlacklistAdminAdded', { account: owner }); - }); + describe('when whitelist is enabled', function () { + let accesslistGuardedMock; - it('emits WhitelistAdminAdded event', function () { - inLogs(this.logs, 'WhitelistAdminAdded', { account: owner }); + beforeEach(async function () { + accesslistGuardedMock = + await AccesslistGuardedMock.new(accesslist.address, true); }); - }); - it('should reject null address accesslist', async function () { - await util.assertReverts(AccesslistGuardedMock.new(util.ZERO_ADDRESS, true)); - }); + describe('blacklisted', function () { + beforeEach(async function () { + accesslist.addBlacklisted(user, { from: owner }); + }); - it('allows privileged privilege propagation: whitelist', async function () { - assert(!(await accesslist.isWhitelistAdmin(user, { from: owner }))); - await util.assertReverts(accesslist.addWhitelisted(user, { from: user1 })); - await accesslist.addWhitelistAdmin(user, { from: owner }); - assert(await accesslist.isWhitelistAdmin(user, { from: owner })); - await accesslist.addWhitelisted(user, { from: user }); - assert(await accesslist.isWhitelisted(user, { from: user1 })); - }); + it('emits event when adding to blacklist', async function () { + accesslistE.addBlacklisted(user1, { from: owner }); + }); - it('rejects unprivileged privilege propagation: whitelist', async function () { - assert(!(await accesslist.isWhitelistAdmin(user, { from: user1 }))); - await util.assertReverts(accesslist.addWhitelisted(user, { from: user1 })); - await util.assertReverts(accesslist.addWhitelistAdmin(user, { from: user1 })); - assert(!(await accesslist.isWhitelistAdmin(user, { from: user1 }))); - await util.assertReverts(accesslist.addWhitelisted(user, { from: user1 })); - }); + it('says that blacklisted user is blacklsited', async function () { + (await accesslist.isBlacklisted(user, { from: owner })).should.be.equal(true); + }); - it('allows privileged privilege propagation: blacklist', async function () { - assert(!(await accesslist.isBlacklistAdmin(user, { from: owner }))); - await util.assertReverts(accesslist.addBlacklisted(user, { from: user1 })); - await accesslist.addBlacklistAdmin(user1, { from: owner }); - assert(await accesslist.isBlacklistAdmin(user1, { from: owner })); - await accesslist.addBlacklisted(user, { from: user1 }); - assert(await accesslist.isBlacklisted(user, { from: user1 })); - }); + it('says that non-blacklisted user is not blacklsited', async function () { + (await accesslist.isBlacklisted(user1, { from: owner })).should.be.equal(false); + }); - it('rejects unprivileged privilege propagation: blacklist', async function () { - assert(!(await accesslist.isBlacklistAdmin(user1, { from: user }))); - await util.assertReverts(accesslist.addBlacklisted(user1, { from: user })); - await util.assertReverts(accesslist.addBlacklistAdmin(user1, { from: user })); - assert(!(await accesslist.isBlacklistAdmin(user1, { from: user }))); - await util.assertReverts(accesslist.addBlacklisted(user1, { from: user })); - }); + it('allows blacklist admin to add to blacklist', async function () { + await accesslist.addBlacklistAdmin(user, { from: owner }); + await accesslist.addBlacklisted(user1, { from: user }); + }); - describe('Blacklisting', async function () { - it('is initially not in blacklist from unprivileged', async function () { - assert(!(await accesslist.isBlacklisted(user, { from: user1 }))); - }); + it('guarded function reverts if user is blacklisted', async function () { + await util.assertRevertsReason( + accesslistGuardedMock.requireNotBlacklistedMock(user), 'no access'); + }); - it('is initially not in blacklist from privileged', async function () { - assert(!(await accesslist.isBlacklisted(user, { from: owner }))); - }); + it('guarded function reverts if caller is blacklisted', async function () { + await util.assertRevertsReason( + accesslistGuardedMock.onlyNotBlacklistedMock({ from: user }), 'no access'); + }); - async function addBlacklisted (al, user, from) { - const { logs } = await al.addBlacklisted(user, { from: owner }); - inLogs(logs, 'BlacklistAdded'); - } + it('reverts when adding same user to blacklist multiple times', async function () { + await util.assertRevertsNotReason(accesslist.addBlacklisted(user, { from: owner }), + 'no access'); + }); - it('allows privileged to add to blacklist', async function () { - assert(!(await accesslist.isBlacklisted(user, { from: owner }))); - addBlacklisted(accesslist, user, owner); - assert(await accesslist.isBlacklisted(user, { from: owner })); - }); + it('allows privileged to remove from blacklist', async function () { + await accesslist.removeBlacklisted(user, { from: owner }); + (await accesslist.isBlacklisted(user, { from: owner })).should.be.equal(false); + }); - it('rejects if user is blacklisted', async function () { - assert(await accesslistGuardedMock.requireNotBlacklistedMock(user)); - addBlacklisted(accesslist, user, owner); - await util.assertReverts(accesslistGuardedMock.requireNotBlacklistedMock(user)); - }); + it('emits event when removing from blacklist', async function () { + await accesslistE.removeBlacklisted(user, { from: owner }); + }); - it('rejects if caller is blacklisted', async function () { - assert(await accesslistGuardedMock.onlyNotBlacklistedMock({ from: user })); - addBlacklisted(accesslist, user, owner); - await util.assertReverts(accesslistGuardedMock.onlyNotBlacklistedMock({ from: user })); - }); + it('reverts when non-admin adds to blacklist ', async function () { + await util.assertReverts(accesslist.addBlacklisted(user, { from: user1 }), + 'not blacklistAdmin'); + }); - it('rejects privileged attempt to add same user to blacklist multiple times', async function () { - assert(!(await accesslist.isBlacklisted(user1, { from: owner }))); - addBlacklisted(accesslist, user1, owner); - await util.assertReverts(accesslist.addBlacklisted(user1, { from: owner })); - assert((await accesslist.isBlacklisted(user1, { from: owner }))); + it('reverts when non-admin removes from blacklist', async function () { + util.assertRevertsReason(accesslist.removeBlacklisted(user, { from: user1 }), + 'not blacklistAdmin'); + }); }); - it('allows privileged to remove from blacklist', async function () { - assert(!(await accesslist.isBlacklisted(user1, { from: owner }))); - await accesslist.addBlacklisted(user1, { from: owner }); - assert(await accesslist.isBlacklisted(user1, { from: owner })); - await accesslist.removeBlacklisted(user1, { from: owner }); - assert(!(await accesslist.isBlacklisted(user1, { from: owner }))); - }); + describe('whitelisted', function () { + beforeEach(async function () { + accesslist.addWhitelisted(user, { from: owner }); + }); - it('rejects unprivileged from removing from blacklist', async function () { - await accesslist.addBlacklisted(user, { from: owner }); - util.assertReverts(accesslist.removeBlacklisted(user, { from: user1 })); - assert(await accesslist.isBlacklisted(user, { from: user1 })); - }); - }); + it('says that whitelisted user is blacklsited', async function () { + (await accesslist.isWhitelisted(user, { from: owner })).should.be.equal(true); + }); - describe('Whitelisting', async function () { - it('is initially not in whitelist from unprivileged', async function () { - assert(!(await accesslist.isWhitelisted(user, { from: user1 }))); - }); + it('says that non-whitelisted user is not blacklsited', async function () { + (await accesslist.isWhitelisted(user1, { from: owner })).should.be.equal(false); + }); - it('is initially not in whitelist from privileged', async function () { - assert(!(await accesslist.isWhitelisted(user, { from: owner }))); - }); + it('allows blacklist admin to add to blacklist', async function () { + await accesslist.addBlacklistAdmin(user, { from: owner }); + await accesslist.addBlacklisted(user1, { from: user }); + }); - async function addWhitelisted (al, user, from) { - const { logs } = await al.addWhitelisted(user, { from: owner }); - inLogs(logs, 'WhitelistAdded'); - } + it('requireWhitelisted guard reverts if user is not whitelisted', async function () { + await util.assertRevertsReason( + accesslistGuardedMock.requireWhitelistedMock(user1), 'no access'); + }); - it('allows privileged to add to whitelist', async function () { - assert(!(await accesslist.isWhitelisted(user, { from: owner }))); - addWhitelisted(accesslist, user, owner); - assert(await accesslist.isWhitelisted(user, { from: owner })); - }); + it('onlyWhitelisted guard reverts if caller is not whitelisted', async function () { + await util.assertRevertsReason( + accesslistGuardedMock.onlyWhitelistedMock({ from: user1 }), 'no access'); + }); - it('rejects if user is not whitelisted', async function () { - await util.assertReverts(accesslistGuardedMock.requireWhitelistedMock(user)); - addWhitelisted(accesslist, user, owner); - assert(await accesslistGuardedMock.requireWhitelistedMock(user)); - }); + it('requireWhitelisted guard succeeds if user is whitelisted', async function () { + (await accesslistGuardedMock.requireWhitelistedMock(user, { from: owner })) + .should.be.equal(true); + }); - it('rejects if caller is whitelisted', async function () { - await util.assertReverts(accesslistGuardedMock.onlyWhitelistedMock({ from: user })); - addWhitelisted(accesslist, user, owner); - assert(await accesslistGuardedMock.onlyWhitelistedMock({ from: user })); - }); + it('onlyWitelisted guard succeeds if user is whitelisted', async function () { + (await accesslistGuardedMock.onlyWhitelistedMock({ from: user })) + .should.be.equal(true); + }); - it('rejects privileged attempt to add same user to whitelist multiple times', async function () { - assert(!(await accesslist.isWhitelisted(user, { from: owner }))); - addWhitelisted(accesslist, user, owner); - await util.assertReverts(accesslist.addWhitelisted(user, { from: owner })); - assert(await accesslist.isWhitelisted(user, { from: owner })); - }); + it('requireNotBlacklisted guard succeeds if user not blacklisted', async function () { + (await accesslistGuardedMock.requireNotBlacklistedMock(user, { from: owner })) + .should.be.equal(true); + }); - it('allows privileged to remove from whitelist', async function () { - assert(!(await accesslist.isWhitelisted(user, { from: owner }))); - await accesslist.addWhitelisted(user, { from: owner }); - assert((await accesslist.isWhitelisted(user, { from: owner }))); - await accesslist.removeWhitelisted(user, { from: owner }); - assert(!(await accesslist.isWhitelisted(user, { from: owner }))); - }); + it('onlyNotBlacklisted guard succeeds if user not blacklisted', async function () { + (await accesslistGuardedMock.onlyNotBlacklistedMock({ from: user })) + .should.be.equal(true); + }); + + it('reverts when adding same user to whitelist multiple times', async function () { + await util.assertRevertsNotReason( + accesslist.addWhitelisted(user, { from: owner }), 'no access'); + }); + + it('allows removing from whitelsit when admin', async function () { + await accesslist.removeWhitelisted(user, { from: owner }); + (await accesslist.isWhitelisted(user, { from: owner })) + .should.be.equal(false); + }); + + it('emits event when removing from whitelist', async function () { + await accesslistE.removeWhitelisted(user, { from: owner }); + }); + + it('reverts when non-admin adds to whitelist ', async function () { + await util.assertReverts(accesslist.addWhitelisted(user, { from: user1 }), + 'not whitelistAdmin'); + }); - it('rejects unprivileged from adding to whitelist', async function () { - assert(!(await accesslist.isWhitelisted(user, { from: user1 }))); - await util.assertReverts(accesslist.addWhitelisted(user, { from: user1 })); - assert(!(await accesslist.isWhitelisted(user, { from: user1 }))); + it('reverts when non-admin removes from whitelist', async function () { + util.assertRevertsReason(accesslist.removeWhitelisted(user, { from: user1 }), + 'not whitelistAdmin'); + }); }); - it('rejects unprivileged from removing from whitelist', async function () { - await accesslist.addWhitelisted(user, { from: owner }); - util.assertReverts(accesslist.removeWhitelisted(user, { from: user1 })); - assert(await accesslist.isWhitelisted(user, { from: user1 })); + describe('blacklisted and whitelisted', function () { + beforeEach(async function () { + accesslist.addWhitelisted(user, { from: owner }); + accesslist.addBlacklisted(user, { from: owner }); + }); + + it('requireHasAccess reverts', async function () { + await util.assertRevertsReason( + accesslistGuardedMock.requireHasAccessMock(user), + 'no access'); + }); + + it('onlyHasAccess reverts', async function () { + await util.assertRevertsReason( + accesslistGuardedMock.onlyHasAccessMock({ from: user }), + 'no access'); + }); }); }); - describe('when whitelist is not enabled', function () { + describe('when whitelist is disabled', function () { + let accesslistGuardedMock; + beforeEach(async function () { - accesslistGuardedMock = await AccesslistGuardedMock.new( - accesslist.address, - false - ); + accesslistGuardedMock = + await AccesslistGuardedMock.new(accesslist.address, false); }); describe('if whitelisted', function () { @@ -239,15 +233,15 @@ contract('Accesslist', async function ( }); it('should revert requireHasAccess', async function () { - await util.assertReverts( - accesslistGuardedMock.requireHasAccessMock(user) - ); + await util.assertRevertsReason( + accesslistGuardedMock.requireHasAccessMock(user), + 'no access'); }); it('should revert onlyHasAccess', async function () { - await util.assertReverts( - accesslistGuardedMock.onlyHasAccessMock({ from: user }) - ); + await util.assertRevertsReason( + accesslistGuardedMock.onlyHasAccessMock({ from: user }), + 'no access'); }); }); @@ -258,15 +252,15 @@ contract('Accesslist', async function ( }); it('should revert requireHasAccess', async function () { - await util.assertReverts( - accesslistGuardedMock.requireHasAccessMock(user) - ); + await util.assertRevertsReason( + accesslistGuardedMock.requireHasAccessMock(user), + 'no access'); }); it('should revert onlyHasAccess', async function () { - await util.assertReverts( - accesslistGuardedMock.onlyHasAccessMock({ from: user }) - ); + await util.assertRevertsReason( + accesslistGuardedMock.onlyHasAccessMock({ from: user }), + 'no access'); }); }); }); diff --git a/test/TokenManager.events.js b/test/TokenManager.events.js new file mode 100644 index 0000000..4e6d192 --- /dev/null +++ b/test/TokenManager.events.js @@ -0,0 +1,17 @@ +const utils = require('./utils.js'); + +module.exports = utils.makeEventMap({ + constructor: () => [], + addToken: (name, addr) => [{ eventName: 'TokenAdded', + paramMap: { name: utils.stringToBytes32(name), + addr: addr } }], + deleteToken: (name, addr) => [{ eventName: 'TokenDeleted', + paramMap: { + name: utils.stringToBytes32(name), + addr: addr } }], + upgradeToken: (name, newAddr, addr) => + [{ eventName: 'TokenUpgraded', + paramMap: { name: utils.stringToBytes32(name), + from: addr, + to: newAddr } }] +}); diff --git a/test/TokenManager.test.js b/test/TokenManager.test.js index 98b8635..1961112 100644 --- a/test/TokenManager.test.js +++ b/test/TokenManager.test.js @@ -1,4 +1,4 @@ -/* global artifacts, contract, assert */ +/* global artifacts, contract, assert, web3 */ /* eslint-env mocha */ 'use strict'; @@ -8,219 +8,170 @@ const { shouldBehaveLikeOwnable } = require('openzeppelin-solidity/test/ownership/Ownable.behavior.js'); const TokenManager = artifacts.require('TokenManager'); -const Accesslist = artifacts.require('Accesslist'); -const TokenX = artifacts.require('TokenX'); -const TokenXMock = artifacts.require('TokenXMock'); -const ExternalERC20Storage = artifacts.require('ExternalERC20Storage'); -const tokName = 'eUSD'; +const TokenManagerE = require('./TokenManager.events.js'); + +const BigNumber = web3.BigNumber; + +require('chai') + .use(require('chai-bignumber')(BigNumber)) + .should(); + +contract('TokenManager', async ([owner, otherAccount, ...accounts]) => { + let tokMgr; + let tokMgrE; + + const tokens = [{ address: new BigNumber(0xf00d), + name: 'tok1' }, + { address: new BigNumber(0xf00e), + name: 'tok2' }]; + const otherToken = { address: new BigNumber(0xf00f), + name: 'tok3' }; -contract('TokenManager', async ([owner, user, ...accounts]) => { beforeEach(async function () { - this.tokMgr = await TokenManager.new(); - this.accesslist = await Accesslist.new(); - const stor1 = await ExternalERC20Storage.new(); - const stor2 = await ExternalERC20Storage.new(); - - this.tokenX = await TokenXMock.new( - tokName, 'e', 4, this.accesslist.address, true, stor1.address, - 0, true, owner, 0, { from: owner }); - - this.tokenX2 = await TokenXMock.new( - tokName, 'se', 8, this.accesslist.address, true, stor2.address, - 0, true, owner, 0, { from: owner }); + tokMgr = await TokenManager.new(); + tokMgrE = TokenManagerE.wrap(tokMgr); }); - describe('ownable behavior', function () { + describe('behaves', function () { beforeEach(async function () { - this.ownable = await TokenManager.new(); + this.ownable = tokMgr; }); - shouldBehaveLikeOwnable(owner, [user]); + shouldBehaveLikeOwnable(owner, [otherAccount]); }); - it('Should throw on retrieving non-existing entires', async function () { - await util.assertReverts( - this.tokMgr.getToken(tokName, { from: accounts[0] }) - ); - }); - - describe('When added token', function () { - beforeEach(async function () { - await this.tokMgr.addToken( - tokName, this.tokenX.address, - { from: owner } - ); + describe('when empty', function () { + it('reverts when adding null-token', async function () { + const nulltoken = util.ZERO_ADDRESS; + await util.assertRevertsReason( + tokMgr.addToken('nulltoken', nulltoken, { from: owner }), + 'Supplied token is null'); }); - it('should add tokens', async function () { - const address = await this.tokMgr.getToken(tokName, { from: owner }); - const tok = TokenX.at(address); - const contractTokName = await tok.name({ from: owner }); + it('reverts when retrieving non-existing entries', async function () { + await util.assertRevertsReason( + tokMgr.getToken('someToken', { from: otherAccount }), + 'Token does not exist'); + }); - assert( - contractTokName === tokName, - 'Name of created contract did not match the expected' - ); + it('returns an empty list of tokens', async function () { + const expected = []; + const r = await tokMgr.getTokens({ from: owner }); + assert.deepEqual(r, expected); }); - it('should upgrade token', async function () { - const address = await this.tokMgr.getToken(tokName, { from: owner }); + it('should add token', async function () { + await tokMgr.addToken(otherToken.name, otherToken.address, + { from: owner }); + (await tokMgr.getToken(otherToken.name)) + .should.be.bignumber.equal(otherToken.address); + }); - assert( - this.tokenX.address === address, - 'Created contract did not match the expected' - ); + it('addToken emits event', async function () { + await tokMgrE.addToken(otherToken.name, otherToken.address, + { from: owner }); + }); + }); - await this.tokMgr.upgradeToken( - tokName, this.tokenX2.address, - { from: owner } - ); + describe('When non-empty', function () { + beforeEach(function () { + tokens.forEach(async (x) => { + await tokMgr.addToken(x.name, x.address, { from: owner }); + }); + }); - const address2 = await this.tokMgr.getToken(tokName, { from: owner }); + tokens.forEach(function (x) { + it(`should retrieve token ${x.name}`, async function () { + (await tokMgr.getToken(x.name)).should.be.bignumber.equal(x.address); + }); + }); - assert( - this.tokenX2.address === address2, - 'Created contract did not match the expected' + it('Elements are set to 0 when deleted', async function () { + // Delete existing tokens + await Promise.all( + tokens.map(x => tokMgr.deleteToken(x.name, { from: owner })) ); - }); - it('fails on duplicated names', async function () { - await util.assertReverts( - this.tokMgr.addToken(tokName, this.tokenX2.address, { from: owner }) + const newExpected = [0, 0]; + const actual = (await tokMgr.getTokens({ from: owner })) + .map((x) => parseInt(x)); + + assert.deepEqual( + actual, newExpected, + 'Token list returned does not match expected' ); }); - }); - it('should properly remove tokens', async function () { - // Token shouldn't exist before creation - await util.assertReverts(this.tokMgr.getToken(tokName, { from: owner })); - - // Create token and add token - await this.tokMgr.addToken(tokName, this.tokenX.address, { from: owner }); - - // Retrieve token. This should be successful - await this.tokMgr.getToken(tokName, { from: owner }); - // Delete token - await this.tokMgr.deleteToken(tokName, { from: owner }); - // Token should now no longer exist - await util.assertReverts( - this.tokMgr.getToken(tokName, { from: owner }) - ); - }); + it('should upgrade token', async function () { + await tokMgr.upgradeToken(tokens[0].name, otherToken.address, + { from: owner }); - it('should reject null ITokenX', async function () { - const nulltoken = util.ZERO_ADDRESS; - await util.assertReverts( - this.tokMgr.addToken('nulltoken', nulltoken, { from: owner }) - ); - }); + (await tokMgr.getToken(tokens[0].name, { from: owner })) + .should.be.bignumber.equal(otherToken.address); + }); - describe('Get Tokens', () => { - it('returns an empty list initially', async function () { - const expected = []; + it('emits upgrade event', async function () { + await tokMgrE.upgradeToken(tokens[0].name, otherToken.address, + tokens[0].address, { from: owner }); + }); - const r = await this.tokMgr.getTokens({ from: owner }); + it('returns a list of created tokens', async function () { + const expected = tokens.map((x) => x.name); + const r = (await tokMgr.getTokens({ from: owner })) + .map(util.bytes32ToString); + // Sort arrays since implementation + // does not require stable order of tokens assert.deepEqual( - r, expected, + r.sort(), expected.sort(), 'Token list returned does not match expected' ); }); - describe('when tokens are added', function () { - beforeEach(async function () { - this.expected = ['tok1', 'tok2']; - - await this.tokMgr.addToken( - this.expected[0], this.tokenX.address, - { from: owner } - ); - - await this.tokMgr.addToken( - this.expected[1], this.tokenX2.address, - { from: owner } - ); - }); - - it('returns a list of created tokens', async function () { - const r = (await this.tokMgr.getTokens({ from: owner })) - .map(util.bytes32ToString); + it('reverts when upgrading non-existing token', async function () { + await util.assertRevertsReason( + tokMgr.upgradeToken('noToken', 0xf00, { from: owner }), + 'Token does not exist'); + }); - // Sort arrays since implementation - // does not require stable order of tokens - assert.deepEqual( - r.sort(), this.expected.sort(), - 'Token list returned does not match expected' - ); - }); + it('fails on duplicated names', async function () { + await util.assertReverts( + tokMgr.addToken(tokens[1].name, tokens[1].address, { from: owner }) + ); + }); - it('Elements are set to 0 when deleted', async function () { - // Delete existing tokens - await Promise.all( - this.expected.map(x => this.tokMgr.deleteToken(x, { from: owner })) - ); + it('should properly remove tokens', async function () { + await tokMgr.deleteToken(tokens[1].name, { from: owner }); - const expected = [0, 0]; - const actual = (await this.tokMgr.getTokens({ from: owner })) - .map((x) => parseInt(x)); + await util.assertReverts(tokMgr.getToken(tokens[1].name, + { from: owner })); + }); - assert.deepEqual( - actual, expected, - 'Token list returned does not match expected' - ); - }); + it('should emit deleteToken event', async function () { + await tokMgrE.deleteToken(tokens[1].name, tokens[1].address, { from: owner }); }); }); - describe('Permissions', () => { - it('Rejects unauthorized newToken', async function () { - await util.assertReverts(this.tokMgr.addToken( - tokName, this.tokenX.address, - { from: user } + describe('reverts when not owner', () => { + it('addToken', async function () { + await util.assertReverts(tokMgr.addToken( + tokens[0].name, tokens[0].address, + { from: otherAccount } )); }); it('Rejects unauthorized deleteToken', async function () { - this.tokMgr.addToken( - tokName, this.tokenX.address, - { from: owner } - ); - - await util.assertReverts( - this.tokMgr.deleteToken(tokName, { from: user }) - ); - - const tokenAddress = await this.tokMgr.getToken(tokName, { from: owner }); - assert(tokenAddress === this.tokenX.address); + await util.assertRevertsNotReason( + tokMgr.deleteToken(tokens[0].name, { from: otherAccount }), + 'Token does not exist'); }); it('Rejects unauthorized upgradeToken', async function () { - await this.tokMgr.addToken( - tokName, this.tokenX.address, - { from: owner } - ); - - const address = await this.tokMgr.getToken(tokName); - - assert( - this.tokenX.address === address, - 'Created contract did not match the expected' - ); - - await util.assertReverts( - this.tokMgr.upgradeToken( - tokName, this.tokenX2.address, - { from: user } - ) - ); - - const address2 = await this.tokMgr.getToken(tokName); - - assert( - this.tokenX.address === address2, - 'Created contract did not match the expected' - ); + await util.assertRevertsNotReason( + tokMgr.upgradeToken(tokens[0].name, tokens[0].address, + { from: otherAccount }), + 'Token does not exist'); }); }); }); diff --git a/test/token/ERC20/ExternalERC20Storage.events.js b/test/token/ERC20/ExternalERC20Storage.events.js new file mode 100644 index 0000000..9965619 --- /dev/null +++ b/test/token/ERC20/ExternalERC20Storage.events.js @@ -0,0 +1,11 @@ +const utils = require('./../../utils.js'); + +module.exports = utils.makeEventMap({ + latchInitialImplementor: (addr) => [{ + eventName: 'StorageInitialImplementorSet', + paramMap: { to: addr } }], + transferImplementor: (toAddr, fromAddr) => [{ + eventName: 'StorageImplementorTransferred', + paramMap: { from: fromAddr, + to: toAddr } }] +}); diff --git a/test/token/ERC20/ExternalERC20Storage.test.js b/test/token/ERC20/ExternalERC20Storage.test.js index 03455a3..8b1e61f 100644 --- a/test/token/ERC20/ExternalERC20Storage.test.js +++ b/test/token/ERC20/ExternalERC20Storage.test.js @@ -1,14 +1,15 @@ /* global artifacts, contract, web3 */ /* eslint-env mocha */ -const shouldFail = require('openzeppelin-solidity/test/helpers/shouldFail'); -const expectEvent = require('openzeppelin-solidity/test/helpers/expectEvent'); const { ZERO_ADDRESS } = require('openzeppelin-solidity/test/helpers/constants'); const ExternalERC20Storage = artifacts.require('ExternalERC20Storage'); +const ExternalERC20StorageE = require('./ExternalERC20Storage.events.js'); const BigNumber = web3.BigNumber; +const utils = require('./../../utils.js'); + require('chai') .use(require('chai-bignumber')(BigNumber)) .should(); @@ -16,13 +17,14 @@ require('chai') contract('ExternalERC20Storage', function ([_, owner, implementor, anotherAccount, thirdAccount]) { beforeEach(async function () { this.token = await ExternalERC20Storage.new({ from: owner }); - const { logs } = await this.token.latchInitialImplementor({ from: implementor }); - expectEvent.inLogs(logs, 'StorageInitialImplementorSet', { to: implementor }); + this.tokenE = ExternalERC20StorageE.wrap(this.token); + await this.token.latchInitialImplementor({ from: implementor }); }); describe('setting initial implementor', function () { beforeEach(async function () { this.otherStorage = await ExternalERC20Storage.new({ from: owner }); + this.otherStorageE = ExternalERC20StorageE.wrap(this.otherStorage); }); it('hasImplementor should be false initially', async function () { @@ -47,13 +49,12 @@ contract('ExternalERC20Storage', function ([_, owner, implementor, anotherAccoun it('reverts when iplementor already exists', async function () { // Note: Uses this.token initialized in parent beforeEach - await shouldFail.reverting(this.token.latchInitialImplementor({ from: owner })); + await utils.assertRevertsReason(this.token.latchInitialImplementor({ from: owner }), + 'Storage implementor is already set'); }); it('Should emit initial implementor event', async function () { - const { logs } = await this.otherStorage.latchInitialImplementor({ from: implementor }); - expectEvent.inLogs(logs, 'StorageInitialImplementorSet', { - to: implementor }); + await this.otherStorageE.latchInitialImplementor(implementor, { from: implementor }); }); }); @@ -66,21 +67,20 @@ contract('ExternalERC20Storage', function ([_, owner, implementor, anotherAccoun }); it('should require non-zero address when transferring implementor', async function () { - await shouldFail.reverting(this.token.transferImplementor( - ZERO_ADDRESS, { from: ownerOrImplementorAddress })); + await utils.assertRevertsReason( + this.token.transferImplementor(ZERO_ADDRESS, { from: ownerOrImplementorAddress }), + 'Expected a non-zero address'); }); it('should emit implementor transferred event', async function () { - const { logs } = await this.token.transferImplementor( - anotherAccount, { from: ownerOrImplementorAddress }); - expectEvent.inLogs(logs, 'StorageImplementorTransferred', { - from: implementor, - to: anotherAccount }); + await this.tokenE.transferImplementor( + anotherAccount, implementor, { from: ownerOrImplementorAddress }); }); it('should revert when transferring to existing implementor', async function () { - await shouldFail.reverting( - this.token.transferImplementor(implementor, { from: ownerOrImplementorAddress })); + await utils.assertRevertsReason( + this.token.transferImplementor(implementor, { from: ownerOrImplementorAddress }), + 'Cannot transfer to same implementor as existing'); }); }; @@ -115,7 +115,8 @@ contract('ExternalERC20Storage', function ([_, owner, implementor, anotherAccoun describe('when is not owner or implementor', function () { it('should not allow transfer implementor', async function () { - await shouldFail.reverting(this.token.transferImplementor(anotherAccount, { from: thirdAccount })); + await utils.assertRevertsReason(this.token.transferImplementor(anotherAccount, { from: thirdAccount }), + 'Is not implementor or owner'); (await this.token.isImplementor({ from: anotherAccount })).should.equal(false); }); }); @@ -146,7 +147,9 @@ contract('ExternalERC20Storage', function ([_, owner, implementor, anotherAccoun const oldBalance = await this.token.balances(anotherAccount); oldBalance.should.not.be.bignumber.equal(newBalance); - await shouldFail.reverting(this.token.setBalance(anotherAccount, newBalance, { from: thirdAccount })); + await utils.assertRevertsReason( + this.token.setBalance(anotherAccount, newBalance, { from: thirdAccount }), + 'Is not implementor or owner'); (await this.token.balances(anotherAccount)).should.be.bignumber.equal(oldBalance); }); }); @@ -177,9 +180,9 @@ contract('ExternalERC20Storage', function ([_, owner, implementor, anotherAccoun const oldAllowance = (await this.token.allowed(anotherAccount, thirdAccount)); oldAllowance.should.not.be.bignumber.equal(newAllowance); - await shouldFail.reverting( - this.token.setAllowed(anotherAccount, thirdAccount, newAllowance, { from: anotherAccount }) - ); + await utils.assertRevertsReason( + this.token.setAllowed(anotherAccount, thirdAccount, newAllowance, { from: anotherAccount }), + 'Is not implementor or owner'); (await this.token.allowed(anotherAccount, thirdAccount)).should.be.bignumber.equal(oldAllowance); }); }); @@ -210,7 +213,9 @@ contract('ExternalERC20Storage', function ([_, owner, implementor, anotherAccoun const oldTotalSupply = await this.token.totalSupply(); oldTotalSupply.should.not.be.bignumber.equal(newTotalSupply); - await shouldFail.reverting(this.token.setTotalSupply(newTotalSupply, { from: thirdAccount })); + await utils.assertRevertsReason( + this.token.setTotalSupply(newTotalSupply, { from: thirdAccount }), + 'Is not implementor or owner'); (await this.token.totalSupply()).should.be.bignumber.equal(oldTotalSupply); }); }); diff --git a/test/token/TokenX.events.js b/test/token/TokenX.events.js new file mode 100644 index 0000000..435a96d --- /dev/null +++ b/test/token/TokenX.events.js @@ -0,0 +1,9 @@ +const utils = require('./../utils.js'); + +module.exports = utils.makeEventMap({ + upgrade: (contract, oldContract) => + [{ eventName: 'UpgradeFinalized', + paramMap: { c: oldContract, + sender: oldContract } }, + { eventName: 'Upgraded', + paramMap: { to: contract } }] }); diff --git a/test/token/TokenX.test.js b/test/token/TokenX.test.js index 362e0af..7f9a81f 100644 --- a/test/token/TokenX.test.js +++ b/test/token/TokenX.test.js @@ -16,6 +16,7 @@ const util = require('./../utils.js'); const Accesslist = artifacts.require('Accesslist'); const TokenXMock = artifacts.require('TokenXMock'); +const TokenXE = require('./TokenX.events.js'); const ExternalERC20Storage = artifacts.require('ExternalERC20Storage'); const BigNumber = web3.BigNumber; @@ -222,6 +223,7 @@ contract('TokenX', async function ( describe('functionality', function () { // Tokens used during tests let token; + let tokenE; let storage; let accesslist; let upgradeToken; @@ -233,6 +235,7 @@ contract('TokenX', async function ( token = await TokenXMock.new(tokNameOrig, symbolOrig, 10, accesslist.address, true, storage.address, 0, true, owner, 100, { from: owner }); + tokenE = TokenXE.wrap(token); upgradeToken = await TokenXMock.new( tokNameUpgraded, symbolUpgraded, 20, accesslist.address, true, storage.address, token.address, false, 0, 0, @@ -242,20 +245,22 @@ contract('TokenX', async function ( await f.addPauser(pauser, { from: owner }); await f.addBurner(burner, { from: owner }); }); + await accesslist.addWhitelistAdmin(whitelistAdmin, { from: owner }); + await accesslist.addWhitelisted(owner, { from: owner }); // Required by the burnFrom test await accesslist.addWhitelisted(burner, { from: owner }); - // // Required by the pauser test + // Required by the pauser test await accesslist.addWhitelisted(otherPauser, { from: owner }); await accesslist.addWhitelisted(pauser, { from: owner }); await accesslist.addWhitelisted(whitelisted, { from: owner }); await accesslist.addWhitelisted(whitelisted1, { from: owner }); - await accesslist.addBlacklisted(blacklisted, { from: owner }); - await accesslist.addBlacklisted(blacklisted1, { from: owner }); - await accesslist.addWhitelisted(blackwhite, { from: owner }); await accesslist.addWhitelisted(blackwhite1, { from: owner }); + + await accesslist.addBlacklisted(blacklisted, { from: owner }); + await accesslist.addBlacklisted(blacklisted1, { from: owner }); await accesslist.addBlacklisted(blackwhite, { from: owner }); await accesslist.addBlacklisted(blackwhite1, { from: owner }); }); @@ -291,9 +296,9 @@ contract('TokenX', async function ( it('reverts when doesn\'t own storage', async function () { await storage.transferImplementor(0xf00f); - await util.assertRevertsReason(token.upgrade( - upgradeToken.address, { from: owner }), - 'I don\'t own my storage. This will end badly.'); + await util.assertRevertsReason( + token.upgrade(upgradeToken.address, { from: owner }), + 'I don\'t own my storage. This will end badly.'); }); it('rejects upgrade from non-owner', async function () { @@ -301,6 +306,10 @@ contract('TokenX', async function ( upgradeToken.address, { from: whitelisted })); }); + it('upgrade emits event', async function () { + await tokenE.upgrade(upgradeToken.address, token.address, { from: owner }); + }); + it('Returns orig token name', async function () { (await this.token.name()).should.be.equal(tokNameOrig); }); @@ -342,8 +351,6 @@ contract('TokenX', async function ( it('Returns new token decimals', async function () { (await this.token.decimals()).should.be.bignumber.equal(20); }); - - // TODO: Test other query funtions? } describe('Upgraded token', function () { diff --git a/test/token/TokenXExplicitSender.events.js b/test/token/TokenXExplicitSender.events.js new file mode 100644 index 0000000..322b444 --- /dev/null +++ b/test/token/TokenXExplicitSender.events.js @@ -0,0 +1,7 @@ +const utils = require('./../utils.js'); + +module.exports = utils.makeEventMap({ + finalizeUpgrade: (contract, senderAddr) => [{ + eventName: 'UpgradeFinalized', + paramMap: { c: contract, + sender: senderAddr } }] }); diff --git a/test/token/TokenXExplicitSender.test.js b/test/token/TokenXExplicitSender.test.js index 02b9113..43e68b2 100644 --- a/test/token/TokenXExplicitSender.test.js +++ b/test/token/TokenXExplicitSender.test.js @@ -8,6 +8,7 @@ const util = require('./../utils.js'); const Accesslist = artifacts.require('Accesslist'); const ExternalERC20Storage = artifacts.require('ExternalERC20Storage'); const TokenXExplicitSender = artifacts.require('TokenXExplicitSenderMock'); +const TokenXExplicitSenderE = require('./TokenXExplicitSender.events.js'); const BigNumber = web3.BigNumber; @@ -40,6 +41,16 @@ contract('TokenXExplicitSender', function ([owner, someAddress, ...rest]) { await util.assertRevertsReason(token.finalizeUpgrade({ from: someAddress }), 'Sender is not old contract'); }); + + it('emits UpgradeFinalized event', async function () { + const token = await TokenXExplicitSender.new( + 'tok', 't', 10, accesslist.address, true, storage.address, + owner, false, { from: owner }); + + const tokenE = TokenXExplicitSenderE.wrap(token); + + await tokenE.finalizeUpgrade(owner, owner, { from: owner }); + }); }); // Remainder of contract tested through TokenX. }); diff --git a/test/utils.js b/test/utils.js index 7540d0e..5402894 100644 --- a/test/utils.js +++ b/test/utils.js @@ -1,9 +1,17 @@ -/* global assert */ +/* global assert, web3 */ + +const BigNumber = web3.BigNumber; +require('chai') + .use(require('chai-bignumber')(BigNumber)) + .should(); +const truffleAssert = require('truffle-assertions'); + +const nodeAssert = require('assert'); const _assertReverts = async (f, reason = '', invertMatch = false) => { let res = false; - assert(typeof (reason) === 'string'); - assert(typeof (invertMatch) === 'boolean'); + nodeAssert(typeof (reason) === 'string'); + nodeAssert(typeof (invertMatch) === 'boolean'); const specificReason = reason !== ''; if (reason !== '') { reason = ' ' + reason; @@ -55,7 +63,7 @@ exports.assertRevertsNotReason = async (f, reason) => { @returns A string of ASCII characters */ -exports.bytes32ToString = (str) => { +function bytes32ToString (str) { if (str.substring(0, 2) !== '0x') { throw new Error('Expected a string representation of a hex number starting with 0x'); } @@ -73,6 +81,123 @@ exports.bytes32ToString = (str) => { parts.push(part); } return parts.map((x) => String.fromCharCode(x)).join(''); -}; +} +exports.bytes32ToString = bytes32ToString; + +function stringToBytes32 (str) { + nodeAssert(typeof str === 'string'); + nodeAssert(str.length <= 32); + const cs = str.split(''); + // Ensure that all chars in string are printable ASCII + nodeAssert(cs.every(x => x.charCodeAt(0) >= 32 && x.charCodeAt(0) <= 126)); + return '0x'.concat(cs.map(x => x.charCodeAt(0).toString(16)).join('')).padEnd(66, '0'); +} +exports.stringToBytes32 = stringToBytes32; exports.ZERO_ADDRESS = '0x0000000000000000000000000000000000000000'; + +/** + Given a transaction log and a list of expected events, ensure that the list + contains a complete and in-order list of the events that were emitted during + the transaction. + + The list of events should have the format [{eventName: string, paramMap: + Object}], where eventName is the name of the event and paramMap lists event + parameters and their expected events. +*/ +let emittedEvents = (log, expectedEvents) => { + nodeAssert(log !== undefined); + nodeAssert(expectedEvents !== undefined); + log.sort((a, b) => a.logIndex - a.logIndex); + (log.length).should.be.equal(expectedEvents.length); + for (let i = 0; i < log.length; i++) { + const actual = log[i]; + const expected = expectedEvents[i]; + actual.event.should.be.equal(expected.eventName); + (Object.entries(actual.args).length).should.be.equal( + Object.entries(expected.paramMap).length); + for (const [k, v] of Object.entries(expected.paramMap)) { + contains(actual.args, k, v); + } + } +}; +exports.emittedEvents = emittedEvents; + +// Modified from Openzeppelin test library +function contains (args, key, value) { + if (isBigNumber(args[key]) || isBigNumber(value)) { + const v = typeof value === 'string' ? stringToBytes32(value) : value; + args[key].should.be.bignumber.equal(v); + } else { + args[key].should.be.equal(value); + } +} + +// From openzeppelin test library +function isBigNumber (object) { + return object.isBigNumber || + object instanceof BigNumber || + (object.constructor && object.constructor.name === 'BigNumber'); +} + +/** + Like emittedEvents, but takes a contract creation instead of a log +*/ +exports.emittedEventsContract = async (contract, expectedEvents) => { + const { logs } = + await truffleAssert.createTransactionResult(contract, + contract.transactionHash); + emittedEvents(logs, expectedEvents); +}; + +function fromEntries (it) { + let o = {}; + for (let [k, v] of it) { + o[k] = v; + } + return o; +} + +async function checkConstructorEvents (f, c, ...rest) { + const { logs } = await truffleAssert.createTransactionResult(c, c.transactionHash); + const e = f(...rest); + emittedEvents(logs, e); +} + +exports.makeEventMap = function (eventMap) { + return { wrap: function (c) { + return fromEntries(Object.entries(eventMap).map(([k, v]) => { + if (!(k in c)) { + throw Error(`Function ${k} not found in contract`); + } + if (k === 'constructor') { + return [k, async function (...params) { + await checkConstructorEvents(eventMap[k], ...params); + }]; + } else { + const fa = c.contract.abi.find(x => x.name === k); + nodeAssert(fa !== undefined); + const f = c[k]; + const fl = fa.inputs.length; + const e = eventMap[k]; + const el = e.length; + // TODO: Find a clever way to deal with type conversions between the + // param map and the emitted events + return [k, + async function (...params) { + if ((params.length < el || params.length > (el + 1))) { + throw Error( + `Event wrapper for ${k} called with the wrong number of parameters`); + } + const lastPar = params.length > el ? 1 : 0; + const expectedEvents = e(...(params.slice(0, el))); + const pars = params.slice(0, fl) + .concat(lastPar ? [params[params.length - 1]] : []); + const res = await f(...pars); + const { logs } = res; + emittedEvents(logs, expectedEvents); + }]; + } + })); + } }; +};