From 5b20c2c987b0805e8e82b90239818228ca96c7c3 Mon Sep 17 00:00:00 2001 From: Juraj Piar Date: Tue, 7 Feb 2023 14:16:33 +0000 Subject: [PATCH 01/16] feat(collector): add multitoken collector --- contracts/MultitokenCollector.sol | 136 ++++++++++ hardhat.config.ts | 6 +- package.json | 2 +- test/MultitokenCollector.test.ts | 409 ++++++++++++++++++++++++++++++ tsconfig.json | 7 + 5 files changed, 556 insertions(+), 4 deletions(-) create mode 100644 contracts/MultitokenCollector.sol create mode 100644 test/MultitokenCollector.test.ts diff --git a/contracts/MultitokenCollector.sol b/contracts/MultitokenCollector.sol new file mode 100644 index 00000000..a7f52195 --- /dev/null +++ b/contracts/MultitokenCollector.sol @@ -0,0 +1,136 @@ +// SPDX-License-Identifier:MIT +pragma solidity ^0.6.12; +pragma experimental ABIEncoderV2; + +import "./interfaces/ICollector.sol"; +import "@openzeppelin/contracts/token/ERC20/IERC20.sol"; +import "@openzeppelin/contracts/math/SafeMath.sol"; + +contract MultitokenCollector is ICollector { + address private _remainderAddress; + RevenuePartner[] private _partners; + IERC20[] private _tokens; + address public owner; + + modifier onlyOwner() { + require(msg.sender == owner, "Only owner can call this"); + _; + } + + modifier noBalanceToShare() { + for (uint256 i = 0; i < _tokens.length; i++) { + require( + _tokens[i].balanceOf(address(this)) < _partners.length, + "There is balance to share" + ); + } + _; + } + + modifier updateValidShares(RevenuePartner[] memory partners) { + _; + uint256 totalShares; + for (uint256 i = 0; i < partners.length; i++) { + require(partners[i].share > 0, "0 is not a valid share"); + totalShares = totalShares + partners[i].share; + _partners.push(partners[i]); + } + require(totalShares == 100, "Shares must add up to 100%"); + } + + constructor( + address _owner, + IERC20[] memory tokens, + RevenuePartner[] memory partners, + address remainderAddress + ) public updateValidShares(partners) { + owner = _owner; + _remainderAddress = remainderAddress; + + for (uint i = 0; i < tokens.length; i++) { + _tokens.push(tokens[i]); + } + } + + function getPartners() external view returns (RevenuePartner[] memory) { + return _partners; + } + + function updateShares( + RevenuePartner[] memory partners + ) external onlyOwner noBalanceToShare updateValidShares(partners) { + delete _partners; + } + + //@notice Withdraw the actual remainder and then update the remainder's address + //for a new one. This function is the only way to withdraw the remainder. + function updateRemainderAddress( + address remainderAddress + ) external onlyOwner noBalanceToShare { + for (uint256 i = 0; i < _tokens.length; i++) { + IERC20 token = _tokens[i]; + uint256 balance = token.balanceOf(address(this)); + + if (balance != 0) { + // solhint-disable-next-line avoid-low-level-calls + (bool success, bytes memory ret) = address(token).call{ + gas: 200000 + }( + abi.encodeWithSelector( + hex"a9059cbb", + _remainderAddress, + balance + ) + ); + + require( + success && (ret.length == 0 || abi.decode(ret, (bool))), + "Unable to transfer remainder" + ); + } + } + _remainderAddress = remainderAddress; + } + + function getTokens() external view returns (IERC20[] memory) { + return _tokens; + } + + function getRemainderAddress() external view returns (address) { + return _remainderAddress; + } + + function withdrawToken(IERC20 token) public onlyOwner { + uint256 balance = token.balanceOf(address(this)); + require(balance >= _partners.length, "Not enough balance to split"); + + address tokenAddr = address(token); + + for (uint256 i = 0; i < _partners.length; i++) { + // solhint-disable-next-line avoid-low-level-calls + (bool success, bytes memory ret) = tokenAddr.call( + abi.encodeWithSelector( + hex"a9059cbb", + _partners[i].beneficiary, + SafeMath.div(SafeMath.mul(balance, _partners[i].share), 100) + ) + ); + + require( + success && (ret.length == 0 || abi.decode(ret, (bool))), + "Unable to withdraw" + ); + } + } + + function withdraw() external override onlyOwner { + for (uint256 i = 0; i < _tokens.length; i++) { + withdrawToken(_tokens[i]); + } + } + + function transferOwnership(address _owner) external override onlyOwner { + require(_owner != address(0), "Owner cannot be zero address"); + owner = _owner; + } +} diff --git a/hardhat.config.ts b/hardhat.config.ts index e534c368..21c45b96 100644 --- a/hardhat.config.ts +++ b/hardhat.config.ts @@ -79,8 +79,8 @@ const config: HardhatUserConfig = { }, tdd: { tasks: [ - // 'clean', - // { command: 'compile', params: { quiet: true } }, + 'clean', + { command: 'compile', params: { quiet: true } }, { command: 'test', params: { @@ -89,7 +89,7 @@ const config: HardhatUserConfig = { }, }, ], - files: ['./test/**/*.ts'], + files: ['./test/**/*.ts', './contracts/*.sol'], verbose: true, clearOnStart: true, }, diff --git a/package.json b/package.json index 4b5024f9..70285556 100644 --- a/package.json +++ b/package.json @@ -1,6 +1,6 @@ { "name": "@rsksmart/rif-relay-contracts", - "version": "2.0.0-beta.0", + "version": "2.0.0-beta.1", "private": false, "description": "This project contains all the contracts needed for the rif relay system.", "license": "MIT", diff --git a/test/MultitokenCollector.test.ts b/test/MultitokenCollector.test.ts new file mode 100644 index 00000000..32dfc00b --- /dev/null +++ b/test/MultitokenCollector.test.ts @@ -0,0 +1,409 @@ +import { + FakeContract, + MockContract, + MockContractFactory, + smock, +} from '@defi-wonderland/smock'; +import { SignerWithAddress } from '@nomiclabs/hardhat-ethers/signers'; +import chai from 'chai'; +import chaiAsPromised from 'chai-as-promised'; +import { ethers } from 'hardhat'; +import { Wallet, BigNumber } from 'ethers'; +import { + ERC20, + MultitokenCollector, + MultitokenCollector__factory, +} from '../typechain-types'; + +chai.use(chaiAsPromised); + +const expect = chai.expect; + +const PARTNER_SHARES = [20, 30, 40, 10]; + +type Partner = { + beneficiary: string; + share: number; +}; + +const buildPartners = async ( + partnerWallets: SignerWithAddress[], + shares: number[] +) => { + return Promise.all( + partnerWallets.map(async (wallet, index) => { + const partner: Partner = { + beneficiary: await wallet.getAddress(), + share: shares[index], + }; + + return partner; + }) + ); +}; + +type CollectorDeployParams = { + ownerAddr?: string; + tokens?: string[]; + partners?: Partner[]; + remainderDestinationAddr?: string; +}; + +type CollectorDeployFunction = ( + collectorParams: CollectorDeployParams +) => ReturnType['deploy']>; + +const prepareDefaultDeployer = + ( + collectorFactory: MockContractFactory, + defaultOwnerAddr: string, + defaultTokens: string[], + defaultPartners: Partner[], + defaultRemainderDestinationAddr: string + ): CollectorDeployFunction => + ({ + ownerAddr = defaultOwnerAddr, + tokens = defaultTokens, + partners = defaultPartners, + remainderDestinationAddr = defaultRemainderDestinationAddr, + }: CollectorDeployParams) => + collectorFactory.deploy( + ownerAddr, + tokens, + partners, + remainderDestinationAddr + ); + +describe('MultitokenCollector', function () { + let owner: SignerWithAddress; + let collectorFactory: MockContractFactory; + let fakeERC20Tokens: FakeContract[]; + let partners: Partner[]; + let remainderDestination: SignerWithAddress; + let partner1: SignerWithAddress; + let partner2: SignerWithAddress; + let partner3: SignerWithAddress; + let partner4: SignerWithAddress; + + beforeEach(async function () { + [owner, partner1, partner2, partner3, partner4, remainderDestination] = + await ethers.getSigners(); + partners = await buildPartners( + [partner1, partner2, partner3, partner4], + PARTNER_SHARES + ); + + fakeERC20Tokens = await Promise.all( + Array(2) + .fill(await smock.fake('ERC20')) + .map((fakeToken: FakeContract) => { + fakeToken.transfer.returns(true); + + return fakeToken; + }) + ); + + collectorFactory = await smock.mock( + 'MultitokenCollector' + ); + }); + + describe('constructor()', function () { + it('Should deploy a MultitokenCollector with single token', async function () { + partners = await buildPartners( + [partner1, partner2, partner3, partner4], + PARTNER_SHARES + ); + const expectedTokens = fakeERC20Tokens.map(({ address }) => address)[0]; + const expectedOwnerAddress = owner.address; + + const collector: MockContract = + await collectorFactory.deploy( + expectedOwnerAddress, + [expectedTokens], + partners, + remainderDestination.address + ); + + const actualOwnerAddress = await collector.owner(); + const actualTokens = await collector.getTokens(); + + expect(actualOwnerAddress, 'Failed to set the owner').to.equal( + expectedOwnerAddress + ); + expect(actualTokens.toString(), 'Failed to set tokens').to.equal( + expectedTokens.toString() + ); + }); + + it('Should deploy a MultitokenCollector with multiple tokens', async function () { + partners = await buildPartners( + [partner1, partner2, partner3, partner4], + PARTNER_SHARES + ); + const expectedTokens = fakeERC20Tokens.map(({ address }) => address); + const expectedOwnerAddress = owner.address; + + const collector: MockContract = + await collectorFactory.deploy( + owner.address, + expectedTokens, + partners, + remainderDestination.address + ); + const actualOwnerAddress = await collector.owner(); + const actualTokens = await collector.getTokens(); + + expect(actualOwnerAddress, 'Failed to set the owner').to.equal( + expectedOwnerAddress + ); + expect(actualTokens.toString(), 'Failed to set tokens').to.equal( + expectedTokens.toString() + ); + }); + + it('Should not deploy with invalid shares', async function () { + const partners = await buildPartners( + [partner1, partner2, partner3, partner4], + [25, 25, 25, 26] + ); + + await expect( + collectorFactory.deploy( + owner.address, + fakeERC20Tokens.map(({ address }) => address), + partners, + remainderDestination.address + ) + ).to.be.rejectedWith('Shares must add up to 100%'); + }); + + it('Should not deploy if the array of partners is empty', async function () { + const partners: Partner[] = []; + + await expect( + collectorFactory.deploy( + owner.address, + fakeERC20Tokens.map(({ address }) => address), + partners, + remainderDestination.address + ) + ).to.be.rejectedWith('Shares must add up to 100%'); + }); + + it('Should not deploy if a share is 0', async function () { + const partners = await buildPartners( + [partner1, partner2, partner3, partner4], + [30, 30, 0, 40] + ); + + await expect( + collectorFactory.deploy( + owner.address, + fakeERC20Tokens.map(({ address }) => address), + partners, + remainderDestination.address + ) + ).to.be.rejectedWith('0 is not a valid share'); + }); + }); + + describe('updateShares', function () { + let newPartners: Partner[]; + let deployColector: CollectorDeployFunction; + + beforeEach(async function () { + newPartners = await buildPartners( + (await ethers.getSigners()).slice(6, 10), + PARTNER_SHARES + ); + + deployColector = prepareDefaultDeployer( + collectorFactory, + owner.address, + fakeERC20Tokens.map(({ address }) => address), + partners, + remainderDestination.address + ); + }); + + it('Should update shares and partners when any token has zero balance', async function () { + const collector = await deployColector({}); + + const expectedPartnersShares = newPartners.map( + ({ beneficiary, share }) => [beneficiary, share] + ); + + expect(await collector.getPartners()).to.not.have.deep.members( + expectedPartnersShares + ); // ensure the state is initially different + + await collector.updateShares(newPartners); + const actualPartnersShares = (await collector.getPartners()).map( + (partnerShare) => [partnerShare.beneficiary, partnerShare.share] + ); + + expect(actualPartnersShares).to.have.deep.members(expectedPartnersShares); + }); + + it('Should update shares and partners when all token balances are less than the number of partners', async function () { + const tokens = fakeERC20Tokens.map((token) => { + token.balanceOf.returns(3); + + return token.address; + }); + const collector = await deployColector({ + tokens, + }); + const expectedPartnersShares = newPartners.map( + ({ beneficiary, share }) => [beneficiary, share] + ); + + expect(await collector.getPartners()).to.not.have.deep.members( + expectedPartnersShares + ); // make sure the state is initially different + + await collector.updateShares(newPartners); + const actualPartnersShares = (await collector.getPartners()).map( + (partnerShare) => [partnerShare.beneficiary, partnerShare.share] + ); + + expect(actualPartnersShares).to.have.deep.members(expectedPartnersShares); + }); + + it('Should fail when any token balance > number of partners', async function () { + fakeERC20Tokens[1].balanceOf.returns(5); + + const collector = await deployColector({ + tokens: fakeERC20Tokens.map(({ address }) => address), + }); + + await expect(collector.updateShares(newPartners)).to.be.rejectedWith( + 'There is balance to share' + ); + }); + + it('Should fail when it is called by an account different than the owner', async function () { + const collector = await deployColector({}); + const notTheOwner = (await ethers.getSigners()).at( + 11 + ) as SignerWithAddress; + + await expect( + collector.connect(notTheOwner).updateShares(newPartners) + ).to.eventually.be.rejectedWith('Only owner can call this'); + }); + + it('Should fail if the shares does not sum up to 100', async function () { + const collector = await deployColector({}); + newPartners[0].share = 25; + newPartners[1].share = 25; + newPartners[2].share = 25; + newPartners[3].share = 26; + + await expect(collector.updateShares(newPartners)).to.be.rejectedWith( + 'Shares must add up to 100%' + ); + }); + + it('Should fail if any one of the shares is 0', async function () { + const collector = await deployColector({}); + newPartners[3].share = 0; + + await expect(collector.updateShares(newPartners)).to.be.rejectedWith( + '0 is not a valid share' + ); + }); + }); + + describe('updateRemainderAddress', function () { + let deployColector: CollectorDeployFunction; + + beforeEach(function () { + deployColector = prepareDefaultDeployer( + collectorFactory, + owner.address, + fakeERC20Tokens.map(({ address }) => address), + partners, + remainderDestination.address + ); + }); + + it('Should update remainder address when any token balance is zero', async function () { + const collector = await deployColector({}); + const expectedAddress = Wallet.createRandom().address; + await collector.updateRemainderAddress(expectedAddress); + const actualAddress = await collector.getRemainderAddress(); + + expect(actualAddress).eql(expectedAddress); + }); + + it(`Should withdraw all tokens' non-zero balances to the previous remainder address`, async function () { + const tokens: string[] = fakeERC20Tokens.map((token) => { + token.balanceOf.returns(3); + + return token.address; + }); + const collector = await deployColector({ + tokens, + }); + const previousRemainderAddress = remainderDestination.address; + const newRemainderAddress = Wallet.createRandom().address; + await collector.updateRemainderAddress(newRemainderAddress); + + await Promise.allSettled( + fakeERC20Tokens.map(async (token, i) => { + const tokenBalance: BigNumber = await token.balanceOf( + previousRemainderAddress + ); + + expect( + token.transfer, + `Token #${i} @${ + token.address + } didn't transfer ${tokenBalance.toString()}` + ).to.have.been.calledWith(remainderDestination.address, tokenBalance); + }) + ); + }); + + it('Should update remainder address when any token balance is non-zero', async function () { + fakeERC20Tokens[0].balanceOf.returns(1); + const collector = await deployColector({ + tokens: fakeERC20Tokens.map(({ address }) => address), + }); + const expectedAddress = Wallet.createRandom().address; + await collector.updateRemainderAddress(expectedAddress); + const actualAddress = await collector.getRemainderAddress(); + + expect(actualAddress).eql(expectedAddress); + }); + + it('Should fail when any token balance >= remainder', async function () { + const tokens = fakeERC20Tokens.map((token) => { + token.balanceOf.returns(5); + + return token.address; + }); + const collector = await deployColector({ tokens }); + + await expect( + collector.updateRemainderAddress(Wallet.createRandom().address) + ).to.be.rejectedWith('There is balance to share'); + }); + + it('Should reject non-owner calls', async function () { + const collector = await deployColector({}); + + const [newRemainderDestination, notTheOwner] = ( + await ethers.getSigners() + ).slice(11, 13); + + await expect( + collector + .connect(notTheOwner) + .updateRemainderAddress(newRemainderDestination.address) + ).to.be.rejectedWith('Only owner can call this'); + }); + }); +}); diff --git a/tsconfig.json b/tsconfig.json index 729ea8e8..88fa65a6 100644 --- a/tsconfig.json +++ b/tsconfig.json @@ -8,6 +8,13 @@ * due to import of types from typechain-types. */ "importsNotUsedAsValues": "remove", + /* + * Adding `undefined` to array item type makes less sense + * in testing, where items of arrays in runtime are + * generally guaranteed to exist, thus we overwrite + * `noUncheckedIndexedAccess` to false. + */ + "noUncheckedIndexedAccess": false, "resolveJsonModule": true, "outDir": "dist", From 42ded5d2b59af9e43dbd43fa7cc9a217bed9a879 Mon Sep 17 00:00:00 2001 From: jurajpiar Date: Wed, 8 Feb 2023 09:34:15 +0000 Subject: [PATCH 02/16] fix(collector): removes unnecessary loops --- contracts/Collector.sol | 32 +++++++++++++------------------- 1 file changed, 13 insertions(+), 19 deletions(-) diff --git a/contracts/Collector.sol b/contracts/Collector.sol index 12fd8b3b..aa2c523b 100644 --- a/contracts/Collector.sol +++ b/contracts/Collector.sol @@ -12,18 +12,6 @@ contract Collector is ICollector { IERC20 public token; address public owner; - modifier validShares(RevenuePartner[] memory partners) { - uint256 totalShares; - - for (uint256 i = 0; i < partners.length; i++) { - require(partners[i].share > 0, "0 is not a valid share"); - totalShares = totalShares + partners[i].share; - } - - require(totalShares == 100, "Shares must add up to 100%"); - _; - } - modifier onlyOwner() { require(msg.sender == owner, "Only owner can call this"); _; @@ -37,17 +25,26 @@ contract Collector is ICollector { _; } + modifier updateValidShares(RevenuePartner[] memory partners) { + _; + uint256 totalShares; + for (uint256 i = 0; i < partners.length; i++) { + require(partners[i].share > 0, "0 is not a valid share"); + totalShares = totalShares + partners[i].share; + _partners.push(partners[i]); + } + require(totalShares == 100, "Shares must add up to 100%"); + } + constructor( address _owner, IERC20 _token, RevenuePartner[] memory partners, address remainderAddress - ) public validShares(partners) { + ) public updateValidShares(partners) { owner = _owner; token = _token; _remainderAddress = remainderAddress; - for (uint256 i = 0; i < partners.length; i++) - _partners.push(partners[i]); } function getPartners() external view returns (RevenuePartner[] memory) { @@ -56,11 +53,8 @@ contract Collector is ICollector { function updateShares( RevenuePartner[] memory partners - ) external validShares(partners) onlyOwner noBalanceToShare { + ) external onlyOwner noBalanceToShare updateValidShares(partners) { delete _partners; - - for (uint256 i = 0; i < partners.length; i++) - _partners.push(partners[i]); } //@notice Withdraw the actual remainder and then update the remainder's address From 7087dbe8bd70c101839b0b3d9473ccae12bcea9a Mon Sep 17 00:00:00 2001 From: jurajpiar Date: Wed, 8 Feb 2023 09:46:40 +0000 Subject: [PATCH 03/16] chore: adds generated json to gitignore --- .gitignore | 2 ++ 1 file changed, 2 insertions(+) diff --git a/.gitignore b/.gitignore index c48502e8..60b33fb0 100644 --- a/.gitignore +++ b/.gitignore @@ -16,3 +16,5 @@ artifacts /contracts-exposed .vscode +revenue-sharing-addresses.json +deploy-collector.input.json From b3a71a903eee79722f83118614f2af6cda6ff946 Mon Sep 17 00:00:00 2001 From: jurajpiar Date: Sun, 12 Feb 2023 09:36:44 +0000 Subject: [PATCH 04/16] feat(collector): unify collectors --- contracts/Collector.sol | 75 +-- contracts/MultitokenCollector.sol | 136 ------ hardhat.config.ts | 2 +- package-lock.json | 4 +- tasks/deployCollector.ts | 14 +- ...kenCollector.test.ts => Collector.test.ts} | 173 +++++-- test/collector.test.ts | 434 ------------------ test/tasks/deployCollector.test.ts | 23 +- 8 files changed, 213 insertions(+), 648 deletions(-) delete mode 100644 contracts/MultitokenCollector.sol rename test/{MultitokenCollector.test.ts => Collector.test.ts} (70%) delete mode 100644 test/collector.test.ts diff --git a/contracts/Collector.sol b/contracts/Collector.sol index aa2c523b..e8452d0d 100644 --- a/contracts/Collector.sol +++ b/contracts/Collector.sol @@ -9,7 +9,7 @@ import "@openzeppelin/contracts/math/SafeMath.sol"; contract Collector is ICollector { address private _remainderAddress; RevenuePartner[] private _partners; - IERC20 public token; + IERC20[] private _tokens; address public owner; modifier onlyOwner() { @@ -18,10 +18,12 @@ contract Collector is ICollector { } modifier noBalanceToShare() { - require( - token.balanceOf(address(this)) < _partners.length, - "There is balance to share" - ); + for (uint256 i = 0; i < _tokens.length; i++) { + require( + _tokens[i].balanceOf(address(this)) < _partners.length, + "There is balance to share" + ); + } _; } @@ -38,13 +40,16 @@ contract Collector is ICollector { constructor( address _owner, - IERC20 _token, + IERC20[] memory tokens, RevenuePartner[] memory partners, address remainderAddress ) public updateValidShares(partners) { owner = _owner; - token = _token; _remainderAddress = remainderAddress; + + for (uint i = 0; i < tokens.length; i++) { + _tokens.push(tokens[i]); + } } function getPartners() external view returns (RevenuePartner[] memory) { @@ -62,34 +67,38 @@ contract Collector is ICollector { function updateRemainderAddress( address remainderAddress ) external onlyOwner noBalanceToShare { - uint256 balance = token.balanceOf(address(this)); - address tokenAddr = address(token); - - if (balance != 0) { - // solhint-disable-next-line avoid-low-level-calls - (bool success, bytes memory ret) = tokenAddr.call{gas: 200000}( - abi.encodeWithSelector( - hex"a9059cbb", - _remainderAddress, - balance - ) - ); - - require( - success && (ret.length == 0 || abi.decode(ret, (bool))), - "Unable to transfer remainder" - ); + for (uint256 i = 0; i < _tokens.length; i++) { + IERC20 token = _tokens[i]; + uint256 balance = token.balanceOf(address(this)); + + if (balance != 0) { + // solhint-disable-next-line avoid-low-level-calls + (bool success, bytes memory ret) = address(token).call( + abi.encodeWithSelector( + hex"a9059cbb", + _remainderAddress, + balance + ) + ); + + require( + success && (ret.length == 0 || abi.decode(ret, (bool))), + "Unable to transfer remainder" + ); + } } - - // solhint-disable-next-line _remainderAddress = remainderAddress; } - function getBalance() external view returns (uint256) { - return token.balanceOf(address(this)); + function getTokens() external view returns (IERC20[] memory) { + return _tokens; } - function withdraw() external override onlyOwner { + function getRemainderAddress() external view returns (address) { + return _remainderAddress; + } + + function withdrawToken(IERC20 token) public onlyOwner { uint256 balance = token.balanceOf(address(this)); require(balance >= _partners.length, "Not enough balance to split"); @@ -112,8 +121,14 @@ contract Collector is ICollector { } } + function withdraw() external override onlyOwner { + for (uint256 i = 0; i < _tokens.length; i++) { + withdrawToken(_tokens[i]); + } + } + function transferOwnership(address _owner) external override onlyOwner { - require(_owner != address(0), "New owner is the zero address"); + require(_owner != address(0), "Owner cannot be zero address"); owner = _owner; } } diff --git a/contracts/MultitokenCollector.sol b/contracts/MultitokenCollector.sol deleted file mode 100644 index a7f52195..00000000 --- a/contracts/MultitokenCollector.sol +++ /dev/null @@ -1,136 +0,0 @@ -// SPDX-License-Identifier:MIT -pragma solidity ^0.6.12; -pragma experimental ABIEncoderV2; - -import "./interfaces/ICollector.sol"; -import "@openzeppelin/contracts/token/ERC20/IERC20.sol"; -import "@openzeppelin/contracts/math/SafeMath.sol"; - -contract MultitokenCollector is ICollector { - address private _remainderAddress; - RevenuePartner[] private _partners; - IERC20[] private _tokens; - address public owner; - - modifier onlyOwner() { - require(msg.sender == owner, "Only owner can call this"); - _; - } - - modifier noBalanceToShare() { - for (uint256 i = 0; i < _tokens.length; i++) { - require( - _tokens[i].balanceOf(address(this)) < _partners.length, - "There is balance to share" - ); - } - _; - } - - modifier updateValidShares(RevenuePartner[] memory partners) { - _; - uint256 totalShares; - for (uint256 i = 0; i < partners.length; i++) { - require(partners[i].share > 0, "0 is not a valid share"); - totalShares = totalShares + partners[i].share; - _partners.push(partners[i]); - } - require(totalShares == 100, "Shares must add up to 100%"); - } - - constructor( - address _owner, - IERC20[] memory tokens, - RevenuePartner[] memory partners, - address remainderAddress - ) public updateValidShares(partners) { - owner = _owner; - _remainderAddress = remainderAddress; - - for (uint i = 0; i < tokens.length; i++) { - _tokens.push(tokens[i]); - } - } - - function getPartners() external view returns (RevenuePartner[] memory) { - return _partners; - } - - function updateShares( - RevenuePartner[] memory partners - ) external onlyOwner noBalanceToShare updateValidShares(partners) { - delete _partners; - } - - //@notice Withdraw the actual remainder and then update the remainder's address - //for a new one. This function is the only way to withdraw the remainder. - function updateRemainderAddress( - address remainderAddress - ) external onlyOwner noBalanceToShare { - for (uint256 i = 0; i < _tokens.length; i++) { - IERC20 token = _tokens[i]; - uint256 balance = token.balanceOf(address(this)); - - if (balance != 0) { - // solhint-disable-next-line avoid-low-level-calls - (bool success, bytes memory ret) = address(token).call{ - gas: 200000 - }( - abi.encodeWithSelector( - hex"a9059cbb", - _remainderAddress, - balance - ) - ); - - require( - success && (ret.length == 0 || abi.decode(ret, (bool))), - "Unable to transfer remainder" - ); - } - } - _remainderAddress = remainderAddress; - } - - function getTokens() external view returns (IERC20[] memory) { - return _tokens; - } - - function getRemainderAddress() external view returns (address) { - return _remainderAddress; - } - - function withdrawToken(IERC20 token) public onlyOwner { - uint256 balance = token.balanceOf(address(this)); - require(balance >= _partners.length, "Not enough balance to split"); - - address tokenAddr = address(token); - - for (uint256 i = 0; i < _partners.length; i++) { - // solhint-disable-next-line avoid-low-level-calls - (bool success, bytes memory ret) = tokenAddr.call( - abi.encodeWithSelector( - hex"a9059cbb", - _partners[i].beneficiary, - SafeMath.div(SafeMath.mul(balance, _partners[i].share), 100) - ) - ); - - require( - success && (ret.length == 0 || abi.decode(ret, (bool))), - "Unable to withdraw" - ); - } - } - - function withdraw() external override onlyOwner { - for (uint256 i = 0; i < _tokens.length; i++) { - withdrawToken(_tokens[i]); - } - } - - function transferOwnership(address _owner) external override onlyOwner { - require(_owner != address(0), "Owner cannot be zero address"); - owner = _owner; - } -} diff --git a/hardhat.config.ts b/hardhat.config.ts index 21c45b96..4b276631 100644 --- a/hardhat.config.ts +++ b/hardhat.config.ts @@ -89,7 +89,7 @@ const config: HardhatUserConfig = { }, }, ], - files: ['./test/**/*.ts', './contracts/*.sol'], + files: ['./contracts/*.sol', './test/**/*.ts', './tasks/**/*.ts'], verbose: true, clearOnStart: true, }, diff --git a/package-lock.json b/package-lock.json index f996d950..5dd42979 100644 --- a/package-lock.json +++ b/package-lock.json @@ -1,12 +1,12 @@ { "name": "@rsksmart/rif-relay-contracts", - "version": "2.0.0-beta.0", + "version": "2.0.0-beta.1", "lockfileVersion": 2, "requires": true, "packages": { "": { "name": "@rsksmart/rif-relay-contracts", - "version": "2.0.0-beta.0", + "version": "2.0.0-beta.1", "license": "MIT", "dependencies": { "@metamask/eth-sig-util": "^4.0.1", diff --git a/tasks/deployCollector.ts b/tasks/deployCollector.ts index 09816bb3..9145a8df 100644 --- a/tasks/deployCollector.ts +++ b/tasks/deployCollector.ts @@ -11,12 +11,12 @@ export interface Partner { share: number; } -interface CollectorConfig { +export type CollectorConfig = { collectorOwner: string; partners: Partner[]; - tokenAddress: string; + tokenAddresses: string[]; remainderAddress: string; -} +}; export type DeployCollectorArg = { configFileName?: string; @@ -54,13 +54,13 @@ export const deployCollector = async ( fs.readFileSync(configFileName, { encoding: 'utf-8' }) ) as CollectorConfig; - const { collectorOwner, partners, tokenAddress, remainderAddress } = + const { collectorOwner, partners, tokenAddresses, remainderAddress } = inputConfig; const collectorFactory = await ethers.getContractFactory('Collector'); const collector = await collectorFactory.deploy( collectorOwner, - tokenAddress, + tokenAddresses, partners, remainderAddress ); @@ -82,7 +82,7 @@ export const deployCollector = async ( const objToPrint = { 'Collector Contract': collector.address, 'Collector Owner': await collector.owner(), - 'Collector Token': await collector.token(), + 'Collector Tokens': await collector.getTokens(), 'Collector Remainder': remainderAddress, ...partnerPrintings, }; @@ -106,7 +106,7 @@ export const deployCollector = async ( jsonConfig[networkId] = { collectorContract: collector.address, collectorOwner: await collector.owner(), - tokenAddress: await collector.token(), + tokenAddresses: await collector.getTokens(), remainderAddress: remainderAddress, partners, }; diff --git a/test/MultitokenCollector.test.ts b/test/Collector.test.ts similarity index 70% rename from test/MultitokenCollector.test.ts rename to test/Collector.test.ts index 32dfc00b..32b0798a 100644 --- a/test/MultitokenCollector.test.ts +++ b/test/Collector.test.ts @@ -9,13 +9,10 @@ import chai from 'chai'; import chaiAsPromised from 'chai-as-promised'; import { ethers } from 'hardhat'; import { Wallet, BigNumber } from 'ethers'; -import { - ERC20, - MultitokenCollector, - MultitokenCollector__factory, -} from '../typechain-types'; +import { ERC20, Collector, Collector__factory } from '../typechain-types'; chai.use(chaiAsPromised); +chai.use(smock.matchers); const expect = chai.expect; @@ -51,11 +48,11 @@ type CollectorDeployParams = { type CollectorDeployFunction = ( collectorParams: CollectorDeployParams -) => ReturnType['deploy']>; +) => ReturnType['deploy']>; const prepareDefaultDeployer = ( - collectorFactory: MockContractFactory, + collectorFactory: MockContractFactory, defaultOwnerAddr: string, defaultTokens: string[], defaultPartners: Partner[], @@ -74,9 +71,9 @@ const prepareDefaultDeployer = remainderDestinationAddr ); -describe('MultitokenCollector', function () { +describe('Collector', function () { let owner: SignerWithAddress; - let collectorFactory: MockContractFactory; + let collectorFactory: MockContractFactory; let fakeERC20Tokens: FakeContract[]; let partners: Partner[]; let remainderDestination: SignerWithAddress; @@ -103,13 +100,11 @@ describe('MultitokenCollector', function () { }) ); - collectorFactory = await smock.mock( - 'MultitokenCollector' - ); + collectorFactory = await smock.mock('Collector'); }); describe('constructor()', function () { - it('Should deploy a MultitokenCollector with single token', async function () { + it('Should deploy a Collector with single token', async function () { partners = await buildPartners( [partner1, partner2, partner3, partner4], PARTNER_SHARES @@ -117,13 +112,12 @@ describe('MultitokenCollector', function () { const expectedTokens = fakeERC20Tokens.map(({ address }) => address)[0]; const expectedOwnerAddress = owner.address; - const collector: MockContract = - await collectorFactory.deploy( - expectedOwnerAddress, - [expectedTokens], - partners, - remainderDestination.address - ); + const collector: MockContract = await collectorFactory.deploy( + expectedOwnerAddress, + [expectedTokens], + partners, + remainderDestination.address + ); const actualOwnerAddress = await collector.owner(); const actualTokens = await collector.getTokens(); @@ -136,7 +130,7 @@ describe('MultitokenCollector', function () { ); }); - it('Should deploy a MultitokenCollector with multiple tokens', async function () { + it('Should deploy a Collector with multiple tokens', async function () { partners = await buildPartners( [partner1, partner2, partner3, partner4], PARTNER_SHARES @@ -144,13 +138,12 @@ describe('MultitokenCollector', function () { const expectedTokens = fakeERC20Tokens.map(({ address }) => address); const expectedOwnerAddress = owner.address; - const collector: MockContract = - await collectorFactory.deploy( - owner.address, - expectedTokens, - partners, - remainderDestination.address - ); + const collector: MockContract = await collectorFactory.deploy( + owner.address, + expectedTokens, + partners, + remainderDestination.address + ); const actualOwnerAddress = await collector.owner(); const actualTokens = await collector.getTokens(); @@ -406,4 +399,128 @@ describe('MultitokenCollector', function () { ).to.be.rejectedWith('Only owner can call this'); }); }); + + describe('withdraw', function () { + let deployColector: CollectorDeployFunction; + + beforeEach(function () { + deployColector = prepareDefaultDeployer( + collectorFactory, + owner.address, + fakeERC20Tokens.map(({ address }) => address), + partners, + remainderDestination.address + ); + }); + + it('Should withdraw for each parther', async function () { + const tokens = fakeERC20Tokens.map((token) => { + token.balanceOf.returns(100); + + return token.address; + }); + const collector = await deployColector({ tokens }); + await collector.withdraw(); + + for (let tokenIndex = 0; tokenIndex < tokens.length; tokenIndex++) { + for ( + let partnerIndex = 0; + partnerIndex < partners.length; + partnerIndex++ + ) { + expect( + fakeERC20Tokens[tokenIndex].transfer, + `Partner[${partnerIndex}] balance for token[${tokenIndex}]` + ).to.have.been.calledWith( + partners[partnerIndex].beneficiary, + PARTNER_SHARES[partnerIndex] + ); + } + } + }); + + it('Should not fail if the revenue to share is equal to the number of partners', async function () { + const expectedTransferCount = partners.length; + const token = fakeERC20Tokens[0]; + token.balanceOf.returns(expectedTransferCount); + const collector = await deployColector({ tokens: [token.address] }); + + await collector.withdraw(); + + expect(token.transfer).to.have.callCount(expectedTransferCount); + }); + + it('Should fail when not enough revenue to share in any of the tokens', async function () { + const tokens = fakeERC20Tokens.map((token, i) => { + const isLastToken = fakeERC20Tokens.length - 1; + token.balanceOf.returns(partners.length - (i === isLastToken ? 1 : 0)); + + return token.address; + }); + const collector = await deployColector({ tokens }); + + await expect(collector.withdraw()).to.be.revertedWith( + 'Not enough balance to split' + ); + }); + + it('Should fail if called by non-owner', async function () { + const collector = await deployColector({}); + + const notTheOwner = (await ethers.getSigners()).at( + 11 + ) as SignerWithAddress; + + await expect( + collector.connect(notTheOwner).withdraw() + ).to.be.revertedWith('Only owner can call this'); + }); + + it('Should fail if the transfer returns false', async function () { + const token = fakeERC20Tokens[0]; + token.balanceOf.returns(100); + token.transfer.returns(false); + const collector = await deployColector({ tokens: [token.address] }); + + await expect(collector.withdraw()).to.be.revertedWith( + 'Unable to withdraw' + ); + }); + }); + + describe('transferOwnership', function () { + let collector: MockContract; + + beforeEach(async function () { + collector = await prepareDefaultDeployer( + collectorFactory, + owner.address, + fakeERC20Tokens.map(({ address }) => address), + partners, + remainderDestination.address + )({}); + }); + + it('Should transfer ownership', async function () { + const { address: newOwnerAddress } = (await ethers.getSigners()).at( + 6 + ) as SignerWithAddress; + + await collector.transferOwnership(newOwnerAddress); + + expect(await collector.owner()).to.be.equal(newOwnerAddress); + }); + + it('Should fail when is called by an address that is not the owner', async function () { + // const [, , , , , , newOwner, notTheOwner] = await hardhat.getSigners(); + const [newOwner, notCurrentOwner] = (await ethers.getSigners()).slice( + 6, + 8 + ); + + await expect( + collector.connect(notCurrentOwner).transferOwnership(newOwner.address) + ).to.be.revertedWith('Only owner can call this'); + }); + }); }); diff --git a/test/collector.test.ts b/test/collector.test.ts deleted file mode 100644 index 0e7144c9..00000000 --- a/test/collector.test.ts +++ /dev/null @@ -1,434 +0,0 @@ -import { - smock, - FakeContract, - MockContractFactory, - MockContract, -} from '@defi-wonderland/smock'; -import chaiAsPromised from 'chai-as-promised'; -import chai from 'chai'; -import { ethers as hardhat } from 'hardhat'; -import { SignerWithAddress } from '@nomiclabs/hardhat-ethers/signers'; -import { Collector, Collector__factory } from '../typechain-types'; - -chai.use(smock.matchers); -chai.use(chaiAsPromised); - -const expect = chai.expect; - -const PARTNER_SHARES = [20, 30, 40, 10]; -const NUMBER_OF_PARTNERS = PARTNER_SHARES.length; - -type Partner = { - beneficiary: string; - share: number; -}; - -async function buildPartners( - partnerWallets: SignerWithAddress[], - shares: number[] -) { - return await Promise.all( - partnerWallets.map(async (wallet, index) => { - const partner: Partner = { - beneficiary: await wallet.getAddress(), - share: shares[index], - }; - - return partner; - }) - ); -} - -describe('Collector', function () { - let owner: SignerWithAddress; - let collectorFactory: MockContractFactory; - let fakeToken: FakeContract; - let partners: Partner[]; - let remainderDestination: SignerWithAddress; - let partner1: SignerWithAddress; - let partner2: SignerWithAddress; - let partner3: SignerWithAddress; - let partner4: SignerWithAddress; - let collector: MockContract; - - async function deployCollector() { - partners = await buildPartners( - [partner1, partner2, partner3, partner4], - PARTNER_SHARES - ); - - collector = await collectorFactory.deploy( - owner.address, - fakeToken.address, - partners, - remainderDestination.address - ); - } - - beforeEach(async function () { - [owner, partner1, partner2, partner3, partner4, remainderDestination] = - await hardhat.getSigners(); - - fakeToken = await smock.fake('ERC20'); - fakeToken['transfer'].returns(true); - - collectorFactory = await smock.mock('Collector'); - }); - - describe('constructor()', function () { - it('Should deploy a Collector', async function () { - partners = await buildPartners( - [partner1, partner2, partner3, partner4], - PARTNER_SHARES - ); - - const collector = await collectorFactory.deploy( - owner.address, - fakeToken.address, - partners, - remainderDestination.address - ); - - expect(await collector.owner(), 'Failed to set the owner').to.be.a - .properAddress; - expect(await collector.token(), 'Failed to set the token').not.to.be.null; - }); - - it('Should not let deploy with invalid shares', async function () { - const partners = await buildPartners( - [partner1, partner2, partner3, partner4], - [25, 25, 25, 26] - ); - - await expect( - collectorFactory.deploy( - owner.address, - fakeToken.address, - partners, - remainderDestination.address - ) - ).to.be.revertedWith('Shares must add up to 100%'); - }); - - it('Should not let deploy if the array of partners is empty', async function () { - const partners: Partner[] = []; - - await expect( - collectorFactory.deploy( - owner.address, - fakeToken.address, - partners, - remainderDestination.address - ) - ).to.be.revertedWith('Shares must add up to 100%'); - }); - - it('Should not let deploy if a share is 0', async function () { - const partners = await buildPartners( - [partner1, partner2, partner3, partner4], - [30, 30, 0, 40] - ); - - await expect( - collectorFactory.deploy( - owner.address, - fakeToken.address, - partners, - remainderDestination.address - ) - ).to.be.revertedWith('0 is not a valid share'); - }); - }); - - describe('updateShares', function () { - beforeEach(async function () { - await deployCollector(); - }); - - it('Should update shares and partners when token balance is zero', async function () { - const [, , , , , , newPartner1, newPartner2, newPartner3, newPartner4] = - await hardhat.getSigners(); - - const newPartners = await buildPartners( - [newPartner1, newPartner2, newPartner3, newPartner4], - PARTNER_SHARES - ); - - await collector.updateShares(newPartners); - - expect(collector.updateShares).to.have.been.called; - }); - - it('Should update shares and partners when token balance is a remainder amount', async function () { - fakeToken.balanceOf.returns(3); - - const [, , , , , , newPartner1, newPartner2, newPartner3, newPartner4] = - await hardhat.getSigners(); - - const newPartners = await buildPartners( - [newPartner1, newPartner2, newPartner3, newPartner4], - PARTNER_SHARES - ); - - await collector.updateShares(newPartners); - - expect(collector.updateShares).to.have.been.called; - }); - - it('Should fail when token balance is grater than a remainder', async function () { - fakeToken.balanceOf.returns(5); - - const [, , , , , , newPartner1, newPartner2, newPartner3, newPartner4] = - await hardhat.getSigners(); - - const newPartners = await buildPartners( - [newPartner1, newPartner2, newPartner3, newPartner4], - PARTNER_SHARES - ); - - await expect(collector.updateShares(newPartners)).to.be.revertedWith( - 'There is balance to share' - ); - }); - - it('Should fail when is called by an account different than the owner', async function () { - const [ - , - , - , - , - , - , - newPartner1, - newPartner2, - newPartner3, - newPartner4, - notTheOwner, - ] = await hardhat.getSigners(); - - const newPartners = await buildPartners( - [newPartner1, newPartner2, newPartner3, newPartner4], - PARTNER_SHARES - ); - - await expect( - collector.connect(notTheOwner).updateShares(newPartners) - ).to.be.revertedWith('Only owner can call this'); - }); - - it('Should fail if the shares does not sum up to 100', async function () { - const [, , , , , , newPartner1, newPartner2, newPartner3, newPartner4] = - await hardhat.getSigners(); - - const newPartners = await buildPartners( - [newPartner1, newPartner2, newPartner3, newPartner4], - [25, 25, 25, 26] - ); - - await expect(collector.updateShares(newPartners)).to.be.revertedWith( - 'Shares must add up to 100%' - ); - }); - - it('Should fail if a share is 0', async function () { - const [, , , , , , newPartner1, newPartner2, newPartner3, newPartner4] = - await hardhat.getSigners(); - - const newPartners = await buildPartners( - [newPartner1, newPartner2, newPartner3, newPartner4], - [30, 30, 40, 0] - ); - - await expect(collector.updateShares(newPartners)).to.be.revertedWith( - '0 is not a valid share' - ); - }); - }); - - describe('updateRemainderAddress', function () { - beforeEach(async function () { - await deployCollector(); - }); - - it('Should update remainder address when token balance is zero', async function () { - const [, , , , , , newRemainderDestination] = await hardhat.getSigners(); - - await collector.updateRemainderAddress(newRemainderDestination.address); - - expect(collector.updateRemainderAddress).to.have.been.called; - }); - - it('Should update remainder when token balance is a remainder and should withdraw remainder', async function () { - fakeToken.balanceOf.returns(3); - - const [, , , , , , newRemainderDestination] = await hardhat.getSigners(); - - await collector.updateRemainderAddress(newRemainderDestination.address); - - expect(fakeToken.transfer).to.have.been.calledWith( - remainderDestination.address, - 3 - ); - }); - - it('Should withdraw when the remainders address sent is the same as the current one', async function () { - fakeToken.balanceOf.returns(3); - - await collector.updateRemainderAddress(remainderDestination.address); - - expect(fakeToken.transfer).to.have.been.calledWith( - remainderDestination.address, - 3 - ); - }); - - it('Should fail when token balance > = remainder', async function () { - fakeToken.balanceOf.returns(5); - - const [, , , , , , newRemainderDestination] = await hardhat.getSigners(); - - await expect( - collector.updateRemainderAddress(newRemainderDestination.address) - ).to.be.revertedWith('There is balance to share'); - }); - - it('Should fail when is called by an address that is not the owner', async function () { - fakeToken.balanceOf.returns(5); - - const [, , , , , , newRemainderDestination, notTheOwner] = - await hardhat.getSigners(); - - await expect( - collector - .connect(notTheOwner) - .updateRemainderAddress(newRemainderDestination.address) - ).to.be.revertedWith('Only owner can call this'); - }); - }); - - describe('getBalance()', function () { - beforeEach(async function () { - await deployCollector(); - }); - - it('Should return 0 if the contract has been just deployed', async function () { - fakeToken.balanceOf.returns(0); - - expect(await collector.getBalance()).to.equal(0); - }); - - describe('updateRemainderAddress', function () { - it('Should fail if the transfer returns false', async function () { - const [owner, , newRemainder] = await hardhat.getSigners(); - - fakeToken['transfer'].returns(false); - fakeToken['balanceOf'].returns(2); - - await expect( - collector.updateRemainderAddress(newRemainder.address, { - from: owner.address, - gasLimit: 100000, - }) - ).to.be.revertedWith('Unable to transfer remainder'); - }); - - it('Should return 100 after that value has been minted', async function () { - fakeToken.balanceOf.returns(100); - - expect(await collector.getBalance()).to.equal(100); - }); - }); - - describe('withdraw', function () { - beforeEach(async function () { - await deployCollector(); - }); - - it('Should withdraw', async function () { - fakeToken.balanceOf.returns(100); - await collector.withdraw(); - - expect( - fakeToken.transfer, - 'Partner[0] balance' - ).to.have.been.calledWith(partners[0].beneficiary, PARTNER_SHARES[0]); - - expect( - fakeToken.transfer, - 'Partner[1] balance' - ).to.have.been.calledWith(partners[1].beneficiary, PARTNER_SHARES[1]); - - expect( - fakeToken.transfer, - 'Partner[2] balance' - ).to.have.been.calledWith(partners[2].beneficiary, PARTNER_SHARES[2]); - - expect( - fakeToken.transfer, - 'Partner[3] balance' - ).to.have.been.calledWith(partners[3].beneficiary, PARTNER_SHARES[3]); - }); - - it('Should not fail if the revenue to share is equal to the number of partners', async function () { - // We assume the current balance of the collector to be - // equal to the number of partners - fakeToken.balanceOf.returns(NUMBER_OF_PARTNERS); - - await collector.withdraw(); - - expect(fakeToken.transfer).to.have.callCount(4); - }); - - it('Should fail when no revenue to share', async function () { - fakeToken.balanceOf.returns(NUMBER_OF_PARTNERS - 1); - - await expect(collector.withdraw()).to.be.revertedWith( - 'Not enough balance to split' - ); - }); - - it('Should fail when is called by an address that is not the owner', async function () { - fakeToken.balanceOf.returns(100); - - const [, , , , , , notTheOwner] = await hardhat.getSigners(); - - await expect( - collector.connect(notTheOwner).withdraw() - ).to.be.revertedWith('Only owner can call this'); - }); - - it('Should fail if the transfer returns false', async function () { - const [owner] = await hardhat.getSigners(); - - fakeToken['transfer'].returns(false); - fakeToken['balanceOf'].returns(100); - - await expect( - collector.withdraw({ from: owner.address, gasLimit: 1000000 }) - ).to.be.revertedWith('Unable to withdraw'); - }); - }); - - describe('transferOwnership', function () { - beforeEach(async function () { - await deployCollector(); - }); - - it('Should transfer ownership', async function () { - const [, , , , , , newOwner] = await hardhat.getSigners(); - - await collector.transferOwnership(newOwner.address); - - expect(await collector.owner()).to.be.equal(newOwner.address); - }); - - it('Should fail when is called by an address that is not the owner', async function () { - const [, , , , , , newOwner, notTheOwner] = await hardhat.getSigners(); - - await expect( - collector.connect(notTheOwner).transferOwnership(newOwner.address) - ).to.be.revertedWith('Only owner can call this'); - }); - }); - }); -}); diff --git a/test/tasks/deployCollector.test.ts b/test/tasks/deployCollector.test.ts index 4c288f65..2c4d31c9 100644 --- a/test/tasks/deployCollector.test.ts +++ b/test/tasks/deployCollector.test.ts @@ -8,6 +8,7 @@ import { ethers } from 'hardhat'; import sinon, { SinonStub, SinonStubbedInstance } from 'sinon'; import { Collector, Collector__factory } from 'typechain-types'; import { + CollectorConfig, DEFAULT_CONFIG_FILE_NAME, DEFAULT_OUTPUT_FILE_NAME, deployCollector, @@ -19,7 +20,7 @@ use(smock.matchers); use(chaiAsPromised); describe('Deploy Script', function () { - const collectorConfiguration = { + const collectorConfiguration: CollectorConfig = { collectorOwner: '0x4E28f372BCe2d0Bf1B129b6A278F582558BF08a7', partners: [ { @@ -31,7 +32,7 @@ describe('Deploy Script', function () { share: 80, }, ], - tokenAddress: '0x726ECC75d5D51356AA4d0a5B648790cC345985ED', + tokenAddresses: ['0x726ECC75d5D51356AA4d0a5B648790cC345985ED'], remainderAddress: '0xc354D97642FAa06781b76Ffb6786f72cd7746C97', }; let configurationFileContent: string; @@ -47,7 +48,7 @@ describe('Deploy Script', function () { ); collector = await collectorFactoryMock.deploy( collectorConfiguration.collectorOwner, - collectorConfiguration.tokenAddress, + collectorConfiguration.tokenAddresses, collectorConfiguration.partners, collectorConfiguration.remainderAddress ); @@ -111,6 +112,7 @@ describe('Deploy Script', function () { }); it('deploy a collector', async function () { + //FIXME: Mismatch of test case and test itself const configFileName = 'inputFile.json'; const taskArgs: DeployCollectorArg = { configFileName, @@ -120,16 +122,17 @@ describe('Deploy Script', function () { expect(await collector.owner()).to.be.eq( collectorConfiguration.collectorOwner ); - expect(await collector.token()).to.be.eq( - collectorConfiguration.tokenAddress + expect(await collector.getTokens()).to.have.members( + collectorConfiguration.tokenAddresses ); const partners = await collector.getPartners(); - partners.forEach(([beneficiary, share]) => { - expect(collectorConfiguration.partners).to.deep.include({ + + expect(partners).to.have.deep.members( + collectorConfiguration.partners.map(({ beneficiary, share }) => [ beneficiary, share, - }); - }); + ]) + ); }); describe('', function () { @@ -146,7 +149,7 @@ describe('Deploy Script', function () { [chainId.toString()]: { collectorContract: collector.address, collectorOwner: await collector.owner(), - tokenAddress: await collector.token(), + tokenAddresses: await collector.getTokens(), remainderAddress: collectorConfiguration.remainderAddress, partners: collectorConfiguration.partners, }, From 2d17851c97c7325f7edba12bb570ed1a217fe0b3 Mon Sep 17 00:00:00 2001 From: jurajpiar Date: Mon, 13 Feb 2023 22:31:14 +0000 Subject: [PATCH 05/16] feat(collector): allow adding/removing tokens --- contracts/Collector.sol | 14 ++++ test/Collector.test.ts | 150 +++++++++++++++++++++++++++++++--------- 2 files changed, 133 insertions(+), 31 deletions(-) diff --git a/contracts/Collector.sol b/contracts/Collector.sol index e8452d0d..0851b5fa 100644 --- a/contracts/Collector.sol +++ b/contracts/Collector.sol @@ -90,10 +90,24 @@ contract Collector is ICollector { _remainderAddress = remainderAddress; } + function addToken(IERC20 token) external onlyOwner { + _tokens.push(token); + } + function getTokens() external view returns (IERC20[] memory) { return _tokens; } + function removeToken(uint256 tokenIndex) external onlyOwner { + require( + _tokens[tokenIndex].balanceOf(address(this)) == 0, + "There is balance to share" + ); + + _tokens[tokenIndex] = _tokens[_tokens.length - 1]; + _tokens.pop(); + } + function getRemainderAddress() external view returns (address) { return _remainderAddress; } diff --git a/test/Collector.test.ts b/test/Collector.test.ts index 32b0798a..97449d61 100644 --- a/test/Collector.test.ts +++ b/test/Collector.test.ts @@ -91,13 +91,12 @@ describe('Collector', function () { ); fakeERC20Tokens = await Promise.all( - Array(2) - .fill(await smock.fake('ERC20')) - .map((fakeToken: FakeContract) => { - fakeToken.transfer.returns(true); + Array.from(Array(2)).map(async (fakeToken: FakeContract) => { + fakeToken = await smock.fake('ERC20'); + fakeToken.transfer.returns(true); - return fakeToken; - }) + return fakeToken; + }) ); collectorFactory = await smock.mock('Collector'); @@ -201,9 +200,99 @@ describe('Collector', function () { }); }); + describe('addToken', function () { + let deployCollector: CollectorDeployFunction; + + beforeEach(function () { + deployCollector = prepareDefaultDeployer( + collectorFactory, + owner.address, + fakeERC20Tokens.map(({ address }) => address), + partners, + remainderDestination.address + ); + }); + + it('should reject if called by non-owner', async function () { + const collector = await deployCollector({}); + const nonOwner = (await ethers.getSigners()).at(6) as SignerWithAddress; + + await expect( + collector.connect(nonOwner).addToken(Wallet.createRandom().address) + ).to.have.been.rejectedWith('Only owner can call this'); + }); + + it('should add token to the list', async function () { + const collector = await deployCollector({}); + const { address: expectedTokenAddress } = await smock.fake( + 'ERC20' + ); + await collector.addToken(expectedTokenAddress); + + const actualTokenAddresses = await collector.getTokens(); + + expect(actualTokenAddresses).to.include.members([expectedTokenAddress]); + }); + }); + + describe('removeToken', function () { + let deployCollector: CollectorDeployFunction; + + beforeEach(function () { + deployCollector = prepareDefaultDeployer( + collectorFactory, + owner.address, + fakeERC20Tokens.map(({ address }) => address), + partners, + remainderDestination.address + ); + }); + + it('should reject if called by non-owner', async function () { + const collector = await deployCollector({}); + const nonOwner = (await ethers.getSigners()).at(6) as SignerWithAddress; + + await expect( + collector.connect(nonOwner).removeToken(0) + ).to.have.been.rejectedWith('Only owner can call this'); + }); + + it('should check token balance of the collector', async function () { + const collector = await deployCollector({}); + const tokenToRemove = fakeERC20Tokens.length - 1; + await collector.removeToken(tokenToRemove); + + expect(fakeERC20Tokens[tokenToRemove].balanceOf).to.have.been.calledWith( + collector.address + ); + }); + + it(`should reject if collector balance is non-zero`, async function () { + const balance = 1; + const tokenToRemove = fakeERC20Tokens.length - 1; + fakeERC20Tokens[tokenToRemove].balanceOf.returns(balance); + const collector = await deployCollector({}); + + await expect(collector.removeToken(tokenToRemove)).to.have.rejectedWith( + 'There is balance to share' + ); + }); + + it('should remove token', async function () { + const tokenToRemove = fakeERC20Tokens.length - 1; + const collector = await deployCollector({}); + await collector.removeToken(tokenToRemove); + + expect( + await collector.getTokens(), + 'Before removal' + ).to.not.include.members([fakeERC20Tokens[tokenToRemove].address]); + }); + }); + describe('updateShares', function () { let newPartners: Partner[]; - let deployColector: CollectorDeployFunction; + let deployCollector: CollectorDeployFunction; beforeEach(async function () { newPartners = await buildPartners( @@ -211,7 +300,7 @@ describe('Collector', function () { PARTNER_SHARES ); - deployColector = prepareDefaultDeployer( + deployCollector = prepareDefaultDeployer( collectorFactory, owner.address, fakeERC20Tokens.map(({ address }) => address), @@ -221,7 +310,7 @@ describe('Collector', function () { }); it('Should update shares and partners when any token has zero balance', async function () { - const collector = await deployColector({}); + const collector = await deployCollector({}); const expectedPartnersShares = newPartners.map( ({ beneficiary, share }) => [beneficiary, share] @@ -245,7 +334,7 @@ describe('Collector', function () { return token.address; }); - const collector = await deployColector({ + const collector = await deployCollector({ tokens, }); const expectedPartnersShares = newPartners.map( @@ -267,7 +356,7 @@ describe('Collector', function () { it('Should fail when any token balance > number of partners', async function () { fakeERC20Tokens[1].balanceOf.returns(5); - const collector = await deployColector({ + const collector = await deployCollector({ tokens: fakeERC20Tokens.map(({ address }) => address), }); @@ -277,7 +366,7 @@ describe('Collector', function () { }); it('Should fail when it is called by an account different than the owner', async function () { - const collector = await deployColector({}); + const collector = await deployCollector({}); const notTheOwner = (await ethers.getSigners()).at( 11 ) as SignerWithAddress; @@ -288,7 +377,7 @@ describe('Collector', function () { }); it('Should fail if the shares does not sum up to 100', async function () { - const collector = await deployColector({}); + const collector = await deployCollector({}); newPartners[0].share = 25; newPartners[1].share = 25; newPartners[2].share = 25; @@ -300,7 +389,7 @@ describe('Collector', function () { }); it('Should fail if any one of the shares is 0', async function () { - const collector = await deployColector({}); + const collector = await deployCollector({}); newPartners[3].share = 0; await expect(collector.updateShares(newPartners)).to.be.rejectedWith( @@ -310,10 +399,10 @@ describe('Collector', function () { }); describe('updateRemainderAddress', function () { - let deployColector: CollectorDeployFunction; + let deployCollector: CollectorDeployFunction; beforeEach(function () { - deployColector = prepareDefaultDeployer( + deployCollector = prepareDefaultDeployer( collectorFactory, owner.address, fakeERC20Tokens.map(({ address }) => address), @@ -323,7 +412,7 @@ describe('Collector', function () { }); it('Should update remainder address when any token balance is zero', async function () { - const collector = await deployColector({}); + const collector = await deployCollector({}); const expectedAddress = Wallet.createRandom().address; await collector.updateRemainderAddress(expectedAddress); const actualAddress = await collector.getRemainderAddress(); @@ -337,7 +426,7 @@ describe('Collector', function () { return token.address; }); - const collector = await deployColector({ + const collector = await deployCollector({ tokens, }); const previousRemainderAddress = remainderDestination.address; @@ -362,7 +451,7 @@ describe('Collector', function () { it('Should update remainder address when any token balance is non-zero', async function () { fakeERC20Tokens[0].balanceOf.returns(1); - const collector = await deployColector({ + const collector = await deployCollector({ tokens: fakeERC20Tokens.map(({ address }) => address), }); const expectedAddress = Wallet.createRandom().address; @@ -378,7 +467,7 @@ describe('Collector', function () { return token.address; }); - const collector = await deployColector({ tokens }); + const collector = await deployCollector({ tokens }); await expect( collector.updateRemainderAddress(Wallet.createRandom().address) @@ -386,7 +475,7 @@ describe('Collector', function () { }); it('Should reject non-owner calls', async function () { - const collector = await deployColector({}); + const collector = await deployCollector({}); const [newRemainderDestination, notTheOwner] = ( await ethers.getSigners() @@ -401,10 +490,10 @@ describe('Collector', function () { }); describe('withdraw', function () { - let deployColector: CollectorDeployFunction; + let deployCollector: CollectorDeployFunction; beforeEach(function () { - deployColector = prepareDefaultDeployer( + deployCollector = prepareDefaultDeployer( collectorFactory, owner.address, fakeERC20Tokens.map(({ address }) => address), @@ -413,13 +502,13 @@ describe('Collector', function () { ); }); - it('Should withdraw for each parther', async function () { + it('Should withdraw for each partner', async function () { const tokens = fakeERC20Tokens.map((token) => { token.balanceOf.returns(100); return token.address; }); - const collector = await deployColector({ tokens }); + const collector = await deployCollector({ tokens }); await collector.withdraw(); for (let tokenIndex = 0; tokenIndex < tokens.length; tokenIndex++) { @@ -443,7 +532,7 @@ describe('Collector', function () { const expectedTransferCount = partners.length; const token = fakeERC20Tokens[0]; token.balanceOf.returns(expectedTransferCount); - const collector = await deployColector({ tokens: [token.address] }); + const collector = await deployCollector({ tokens: [token.address] }); await collector.withdraw(); @@ -457,7 +546,7 @@ describe('Collector', function () { return token.address; }); - const collector = await deployColector({ tokens }); + const collector = await deployCollector({ tokens }); await expect(collector.withdraw()).to.be.revertedWith( 'Not enough balance to split' @@ -465,7 +554,7 @@ describe('Collector', function () { }); it('Should fail if called by non-owner', async function () { - const collector = await deployColector({}); + const collector = await deployCollector({}); const notTheOwner = (await ethers.getSigners()).at( 11 @@ -480,7 +569,7 @@ describe('Collector', function () { const token = fakeERC20Tokens[0]; token.balanceOf.returns(100); token.transfer.returns(false); - const collector = await deployColector({ tokens: [token.address] }); + const collector = await deployCollector({ tokens: [token.address] }); await expect(collector.withdraw()).to.be.revertedWith( 'Unable to withdraw' @@ -511,8 +600,7 @@ describe('Collector', function () { expect(await collector.owner()).to.be.equal(newOwnerAddress); }); - it('Should fail when is called by an address that is not the owner', async function () { - // const [, , , , , , newOwner, notTheOwner] = await hardhat.getSigners(); + it('Should fail when called by an address that is not the owner', async function () { const [newOwner, notCurrentOwner] = (await ethers.getSigners()).slice( 6, 8 From 0b94c880db169cf16ff66d52582c870c8800aafa Mon Sep 17 00:00:00 2001 From: Christos Otarola Date: Thu, 16 Feb 2023 07:10:37 -0500 Subject: [PATCH 06/16] feat: include token address on removal --- contracts/Collector.sol | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/contracts/Collector.sol b/contracts/Collector.sol index 0851b5fa..0e3b15ea 100644 --- a/contracts/Collector.sol +++ b/contracts/Collector.sol @@ -98,7 +98,8 @@ contract Collector is ICollector { return _tokens; } - function removeToken(uint256 tokenIndex) external onlyOwner { + function removeToken(address token, uint256 tokenIndex) external onlyOwner { + require(_tokens[tokenIndex] == token, 'Incorrect token'); require( _tokens[tokenIndex].balanceOf(address(this)) == 0, "There is balance to share" From c77618fc1da8ec00a8b50323dcddf135a09397c4 Mon Sep 17 00:00:00 2001 From: Christos Otarola Date: Thu, 16 Feb 2023 07:15:49 -0500 Subject: [PATCH 07/16] fix: IERC20 instead of address --- contracts/Collector.sol | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/contracts/Collector.sol b/contracts/Collector.sol index 0e3b15ea..aa093b59 100644 --- a/contracts/Collector.sol +++ b/contracts/Collector.sol @@ -98,7 +98,7 @@ contract Collector is ICollector { return _tokens; } - function removeToken(address token, uint256 tokenIndex) external onlyOwner { + function removeToken(IERC20 token, uint256 tokenIndex) external onlyOwner { require(_tokens[tokenIndex] == token, 'Incorrect token'); require( _tokens[tokenIndex].balanceOf(address(this)) == 0, From 642fdd8344da3ac725d1ef0cb73339ae8880fb88 Mon Sep 17 00:00:00 2001 From: Christos Otarola Date: Thu, 16 Feb 2023 07:50:09 -0500 Subject: [PATCH 08/16] fix: collector removeToken tests --- test/Collector.test.ts | 21 +++++++++++---------- 1 file changed, 11 insertions(+), 10 deletions(-) diff --git a/test/Collector.test.ts b/test/Collector.test.ts index 97449d61..d3d383d2 100644 --- a/test/Collector.test.ts +++ b/test/Collector.test.ts @@ -251,42 +251,43 @@ describe('Collector', function () { it('should reject if called by non-owner', async function () { const collector = await deployCollector({}); const nonOwner = (await ethers.getSigners()).at(6) as SignerWithAddress; + const randomAddress = Wallet.createRandom().address; await expect( - collector.connect(nonOwner).removeToken(0) + collector.connect(nonOwner).removeToken(randomAddress, 0) ).to.have.been.rejectedWith('Only owner can call this'); }); it('should check token balance of the collector', async function () { const collector = await deployCollector({}); - const tokenToRemove = fakeERC20Tokens.length - 1; - await collector.removeToken(tokenToRemove); + const tokenIndexToRemove = fakeERC20Tokens.length - 1; + await collector.removeToken(fakeERC20Tokens[tokenIndexToRemove].address, tokenIndexToRemove); - expect(fakeERC20Tokens[tokenToRemove].balanceOf).to.have.been.calledWith( + expect(fakeERC20Tokens[tokenIndexToRemove].balanceOf).to.have.been.calledWith( collector.address ); }); it(`should reject if collector balance is non-zero`, async function () { const balance = 1; - const tokenToRemove = fakeERC20Tokens.length - 1; - fakeERC20Tokens[tokenToRemove].balanceOf.returns(balance); + const tokenIndexToRemove = fakeERC20Tokens.length - 1; + fakeERC20Tokens[tokenIndexToRemove].balanceOf.returns(balance); const collector = await deployCollector({}); - await expect(collector.removeToken(tokenToRemove)).to.have.rejectedWith( + await expect(collector.removeToken(fakeERC20Tokens[tokenIndexToRemove].address, tokenIndexToRemove)).to.have.rejectedWith( 'There is balance to share' ); }); it('should remove token', async function () { - const tokenToRemove = fakeERC20Tokens.length - 1; + const tokenIndexToRemove = fakeERC20Tokens.length - 1; const collector = await deployCollector({}); - await collector.removeToken(tokenToRemove); + await collector.removeToken(fakeERC20Tokens[tokenIndexToRemove].address, tokenIndexToRemove); expect( await collector.getTokens(), 'Before removal' - ).to.not.include.members([fakeERC20Tokens[tokenToRemove].address]); + ).to.not.include.members([fakeERC20Tokens[tokenIndexToRemove].address]); }); }); From 41b3e20661ae365b2ede1b4c5e11d9e6cbb7cd37 Mon Sep 17 00:00:00 2001 From: Christos Otarola Date: Thu, 16 Feb 2023 07:53:40 -0500 Subject: [PATCH 09/16] fix: format for collector.sol --- contracts/Collector.sol | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/contracts/Collector.sol b/contracts/Collector.sol index aa093b59..4b3cf803 100644 --- a/contracts/Collector.sol +++ b/contracts/Collector.sol @@ -99,7 +99,7 @@ contract Collector is ICollector { } function removeToken(IERC20 token, uint256 tokenIndex) external onlyOwner { - require(_tokens[tokenIndex] == token, 'Incorrect token'); + require(_tokens[tokenIndex] == token, "Incorrect token"); require( _tokens[tokenIndex].balanceOf(address(this)) == 0, "There is balance to share" From 51923e70289730f960e2a796bc0e1acb53fcafbd Mon Sep 17 00:00:00 2001 From: Christos Otarola Date: Thu, 16 Feb 2023 07:55:49 -0500 Subject: [PATCH 10/16] fix: format for collector.test --- test/Collector.test.ts | 25 +++++++++++++++++-------- 1 file changed, 17 insertions(+), 8 deletions(-) diff --git a/test/Collector.test.ts b/test/Collector.test.ts index d3d383d2..38d5bbee 100644 --- a/test/Collector.test.ts +++ b/test/Collector.test.ts @@ -261,11 +261,14 @@ describe('Collector', function () { it('should check token balance of the collector', async function () { const collector = await deployCollector({}); const tokenIndexToRemove = fakeERC20Tokens.length - 1; - await collector.removeToken(fakeERC20Tokens[tokenIndexToRemove].address, tokenIndexToRemove); - - expect(fakeERC20Tokens[tokenIndexToRemove].balanceOf).to.have.been.calledWith( - collector.address + await collector.removeToken( + fakeERC20Tokens[tokenIndexToRemove].address, + tokenIndexToRemove ); + + expect( + fakeERC20Tokens[tokenIndexToRemove].balanceOf + ).to.have.been.calledWith(collector.address); }); it(`should reject if collector balance is non-zero`, async function () { @@ -274,15 +277,21 @@ describe('Collector', function () { fakeERC20Tokens[tokenIndexToRemove].balanceOf.returns(balance); const collector = await deployCollector({}); - await expect(collector.removeToken(fakeERC20Tokens[tokenIndexToRemove].address, tokenIndexToRemove)).to.have.rejectedWith( - 'There is balance to share' - ); + await expect( + collector.removeToken( + fakeERC20Tokens[tokenIndexToRemove].address, + tokenIndexToRemove + ) + ).to.have.rejectedWith('There is balance to share'); }); it('should remove token', async function () { const tokenIndexToRemove = fakeERC20Tokens.length - 1; const collector = await deployCollector({}); - await collector.removeToken(fakeERC20Tokens[tokenIndexToRemove].address, tokenIndexToRemove); + await collector.removeToken( + fakeERC20Tokens[tokenIndexToRemove].address, + tokenIndexToRemove + ); expect( await collector.getTokens(), From 524a36eba37ba34883468cafb8d8a29f30fb545e Mon Sep 17 00:00:00 2001 From: Christos Otarola Date: Thu, 16 Feb 2023 08:45:28 -0500 Subject: [PATCH 11/16] fix: deploy script sample and summary table --- deploy-collector.input.sample.json | 4 +++- tasks/deployCollector.ts | 8 +++++++- 2 files changed, 10 insertions(+), 2 deletions(-) diff --git a/deploy-collector.input.sample.json b/deploy-collector.input.sample.json index b52797a5..a6108074 100644 --- a/deploy-collector.input.sample.json +++ b/deploy-collector.input.sample.json @@ -18,6 +18,8 @@ "share": 32 } ], - "tokenAddress": "0xb824784A5bF2Bc7139d1786639444e4Da259934B", + "tokenAddresses": [ + "0xb824784A5bF2Bc7139d1786639444e4Da259934B" + ], "remainderAddress": "0xc354D97642FAa06781b76Ffb6786f72cd7746C97" } \ No newline at end of file diff --git a/tasks/deployCollector.ts b/tasks/deployCollector.ts index 9145a8df..42a80cf0 100644 --- a/tasks/deployCollector.ts +++ b/tasks/deployCollector.ts @@ -79,11 +79,17 @@ export const deployCollector = async ( }, {}); + const collectorTokens = await collector.getTokens(); + const tokensToPrint = {}; + collectorTokens.forEach((tokenAddress, i) => { + Object.assign(tokensToPrint, { [`Collector Token ${i}`]: tokenAddress }); + }); + const objToPrint = { 'Collector Contract': collector.address, 'Collector Owner': await collector.owner(), - 'Collector Tokens': await collector.getTokens(), 'Collector Remainder': remainderAddress, + ...tokensToPrint, ...partnerPrintings, }; console.table(objToPrint); From 11b5b5634656681fa1284f839e3bdcdb6738af84 Mon Sep 17 00:00:00 2001 From: Christos Otarola Date: Thu, 16 Feb 2023 11:51:20 -0500 Subject: [PATCH 12/16] feat: add token mapping --- contracts/Collector.sol | 6 ++++++ test/Collector.test.ts | 20 ++++++++++++++++++++ 2 files changed, 26 insertions(+) diff --git a/contracts/Collector.sol b/contracts/Collector.sol index 4b3cf803..1e3f6915 100644 --- a/contracts/Collector.sol +++ b/contracts/Collector.sol @@ -11,6 +11,7 @@ contract Collector is ICollector { RevenuePartner[] private _partners; IERC20[] private _tokens; address public owner; + mapping(IERC20 => bool) public tokenMap; modifier onlyOwner() { require(msg.sender == owner, "Only owner can call this"); @@ -49,6 +50,7 @@ contract Collector is ICollector { for (uint i = 0; i < tokens.length; i++) { _tokens.push(tokens[i]); + tokenMap[tokens[i]] = true; } } @@ -91,6 +93,8 @@ contract Collector is ICollector { } function addToken(IERC20 token) external onlyOwner { + require(tokenMap[token] == false, "Token is already accepted"); + tokenMap[token] = true; _tokens.push(token); } @@ -99,12 +103,14 @@ contract Collector is ICollector { } function removeToken(IERC20 token, uint256 tokenIndex) external onlyOwner { + require(tokenMap[token] == true, "Token is not accepted"); require(_tokens[tokenIndex] == token, "Incorrect token"); require( _tokens[tokenIndex].balanceOf(address(this)) == 0, "There is balance to share" ); + delete tokenMap[token]; _tokens[tokenIndex] = _tokens[_tokens.length - 1]; _tokens.pop(); } diff --git a/test/Collector.test.ts b/test/Collector.test.ts index 38d5bbee..d0f799b1 100644 --- a/test/Collector.test.ts +++ b/test/Collector.test.ts @@ -222,6 +222,15 @@ describe('Collector', function () { ).to.have.been.rejectedWith('Only owner can call this'); }); + it('should reject if token is already added', async function () { + const collector = await deployCollector({}); + const { address: tokenAddress } = await smock.fake('ERC20'); + await collector.addToken(tokenAddress); + await expect(collector.addToken(tokenAddress)).to.be.rejectedWith( + 'Token is already accepted' + ); + }); + it('should add token to the list', async function () { const collector = await deployCollector({}); const { address: expectedTokenAddress } = await smock.fake( @@ -285,6 +294,17 @@ describe('Collector', function () { ).to.have.rejectedWith('There is balance to share'); }); + it('should be rejected if token is not accepted', async function () { + const tokenIndexToRemove = fakeERC20Tokens.length - 1; + const collector = await deployCollector({}); + + const randomAddress = Wallet.createRandom().address; + + await expect( + collector.removeToken(randomAddress, tokenIndexToRemove) + ).to.be.rejectedWith('Token is not accepted'); + }); + it('should remove token', async function () { const tokenIndexToRemove = fakeERC20Tokens.length - 1; const collector = await deployCollector({}); From b6a97a88eec08ad9e078a407bf602ea2852c8924 Mon Sep 17 00:00:00 2001 From: Antonio Morrone Date: Fri, 17 Feb 2023 13:56:10 +0100 Subject: [PATCH 13/16] fix: check if token is accepted in withdrawToken --- contracts/Collector.sol | 1 + test/Collector.test.ts | 116 +++++++++++++++++++++++++++++++++++----- 2 files changed, 104 insertions(+), 13 deletions(-) diff --git a/contracts/Collector.sol b/contracts/Collector.sol index 1e3f6915..4b31161f 100644 --- a/contracts/Collector.sol +++ b/contracts/Collector.sol @@ -120,6 +120,7 @@ contract Collector is ICollector { } function withdrawToken(IERC20 token) public onlyOwner { + require(tokenMap[token], "Token is not accepted"); uint256 balance = token.balanceOf(address(this)); require(balance >= _partners.length, "Not enough balance to split"); diff --git a/test/Collector.test.ts b/test/Collector.test.ts index d0f799b1..002d5b41 100644 --- a/test/Collector.test.ts +++ b/test/Collector.test.ts @@ -71,6 +71,21 @@ const prepareDefaultDeployer = remainderDestinationAddr ); +function checkPartnerBalanceForToken( + partners: Partner[], + tokenToWithdraw: FakeContract +) { + for (let partnerIndex = 0; partnerIndex < partners.length; partnerIndex++) { + expect( + tokenToWithdraw.transfer, + `Partner[${partnerIndex}] balance for token[${tokenToWithdraw.address}]` + ).to.have.been.calledWith( + partners[partnerIndex].beneficiary, + PARTNER_SHARES[partnerIndex] + ); + } +} + describe('Collector', function () { let owner: SignerWithAddress; let collectorFactory: MockContractFactory; @@ -542,19 +557,7 @@ describe('Collector', function () { await collector.withdraw(); for (let tokenIndex = 0; tokenIndex < tokens.length; tokenIndex++) { - for ( - let partnerIndex = 0; - partnerIndex < partners.length; - partnerIndex++ - ) { - expect( - fakeERC20Tokens[tokenIndex].transfer, - `Partner[${partnerIndex}] balance for token[${tokenIndex}]` - ).to.have.been.calledWith( - partners[partnerIndex].beneficiary, - PARTNER_SHARES[partnerIndex] - ); - } + checkPartnerBalanceForToken(partners, fakeERC20Tokens[tokenIndex]); } }); @@ -607,6 +610,93 @@ describe('Collector', function () { }); }); + describe('withdrawToken', function () { + let deployCollector: CollectorDeployFunction; + + beforeEach(function () { + deployCollector = prepareDefaultDeployer( + collectorFactory, + owner.address, + fakeERC20Tokens.map(({ address }) => address), + partners, + remainderDestination.address + ); + }); + + it('Should withdraw for each partner', async function () { + const tokens = fakeERC20Tokens.map((token) => { + token.balanceOf.returns(100); + + return token.address; + }); + + const tokenToWithdraw = fakeERC20Tokens[0]; + const collector = await deployCollector({ tokens }); + await collector.withdrawToken(tokenToWithdraw.address); + + checkPartnerBalanceForToken(partners, tokenToWithdraw); + }); + + it('Should not fail if the revenue to share is equal to the number of partners', async function () { + const expectedTransferCount = partners.length; + const token = fakeERC20Tokens[0]; + token.balanceOf.returns(expectedTransferCount); + const collector = await deployCollector({ tokens: [token.address] }); + + await collector.withdrawToken(token.address); + + expect(token.transfer).to.have.callCount(expectedTransferCount); + }); + + it('Should fail when not enough revenue to share', async function () { + const tokens = fakeERC20Tokens.map((token) => { + return token.address; + }); + const token = fakeERC20Tokens[0]; + token.balanceOf.returns(partners.length - 1); + const collector = await deployCollector({ tokens }); + + await expect(collector.withdrawToken(token.address)).to.be.revertedWith( + 'Not enough balance to split' + ); + }); + + it("should raise an error if the token isn't accepted", async function () { + const token = fakeERC20Tokens[0]; + const notAllowedToken = fakeERC20Tokens[1]; + [token, notAllowedToken].map((token) => token.balanceOf.returns(100)); + const collector = await deployCollector({ tokens: [token.address] }); + + await expect( + collector.withdrawToken(notAllowedToken.address) + ).to.be.revertedWith('Token is not accepted'); + }); + + it('Should fail if the transfer returns false', async function () { + const token = fakeERC20Tokens[0]; + token.balanceOf.returns(100); + token.transfer.returns(false); + const collector = await deployCollector({ tokens: [token.address] }); + + await expect(collector.withdrawToken(token.address)).to.be.revertedWith( + 'Unable to withdraw' + ); + }); + + it('Should fail if called by non-owner', async function () { + const collector = await deployCollector({}); + const token = fakeERC20Tokens[0]; + + const notTheOwner = (await ethers.getSigners()).at( + 11 + ) as SignerWithAddress; + + await expect( + collector.connect(notTheOwner).withdrawToken(token.address) + ).to.be.revertedWith('Only owner can call this'); + }); + }); + describe('transferOwnership', function () { let collector: MockContract; From 47109c6db1ef1e60dc3809f89fd50e51765d7b0c Mon Sep 17 00:00:00 2001 From: Antonio Morrone Date: Fri, 17 Feb 2023 15:37:42 +0100 Subject: [PATCH 14/16] test: add test for collector deployment without tokens --- test/Collector.test.ts | 33 ++++++++++++++++++++++++++++----- 1 file changed, 28 insertions(+), 5 deletions(-) diff --git a/test/Collector.test.ts b/test/Collector.test.ts index 002d5b41..e3b6a410 100644 --- a/test/Collector.test.ts +++ b/test/Collector.test.ts @@ -118,6 +118,29 @@ describe('Collector', function () { }); describe('constructor()', function () { + it('Should deploy a Collector without tokens', async function () { + partners = await buildPartners( + [partner1, partner2, partner3, partner4], + PARTNER_SHARES + ); + const expectedOwnerAddress = owner.address; + + const collector: MockContract = await collectorFactory.deploy( + expectedOwnerAddress, + [], + partners, + remainderDestination.address + ); + + const actualOwnerAddress = await collector.owner(); + const actualTokens = await collector.getTokens(); + + expect(actualOwnerAddress, 'Failed to set the owner').to.equal( + expectedOwnerAddress + ); + expect(actualTokens, 'Failed to set tokens').to.deep.equal([]); + }); + it('Should deploy a Collector with single token', async function () { partners = await buildPartners( [partner1, partner2, partner3, partner4], @@ -139,9 +162,9 @@ describe('Collector', function () { expect(actualOwnerAddress, 'Failed to set the owner').to.equal( expectedOwnerAddress ); - expect(actualTokens.toString(), 'Failed to set tokens').to.equal( - expectedTokens.toString() - ); + expect(actualTokens, 'Failed to set tokens').to.deep.equal([ + expectedTokens, + ]); }); it('Should deploy a Collector with multiple tokens', async function () { @@ -164,8 +187,8 @@ describe('Collector', function () { expect(actualOwnerAddress, 'Failed to set the owner').to.equal( expectedOwnerAddress ); - expect(actualTokens.toString(), 'Failed to set tokens').to.equal( - expectedTokens.toString() + expect(actualTokens, 'Failed to set tokens').to.deep.equal( + expectedTokens ); }); From aa3f0df63caf5896cdcbc71cef629b67d70cef7a Mon Sep 17 00:00:00 2001 From: Christos Otarola Date: Mon, 20 Feb 2023 11:45:49 -0500 Subject: [PATCH 15/16] fix: refactor tokensToPrint --- tasks/deployCollector.ts | 15 +++++++++------ 1 file changed, 9 insertions(+), 6 deletions(-) diff --git a/tasks/deployCollector.ts b/tasks/deployCollector.ts index 42a80cf0..bceed853 100644 --- a/tasks/deployCollector.ts +++ b/tasks/deployCollector.ts @@ -79,17 +79,20 @@ export const deployCollector = async ( }, {}); - const collectorTokens = await collector.getTokens(); - const tokensToPrint = {}; - collectorTokens.forEach((tokenAddress, i) => { - Object.assign(tokensToPrint, { [`Collector Token ${i}`]: tokenAddress }); - }); + const tokenPrintings = (await collector.getTokens()).reduce< + Record + >((accumulator, token, i) => { + return { + ...accumulator, + [`Collector Token ${i}`]: token, + }; + }, {}); const objToPrint = { 'Collector Contract': collector.address, 'Collector Owner': await collector.owner(), 'Collector Remainder': remainderAddress, - ...tokensToPrint, + ...tokenPrintings, ...partnerPrintings, }; console.table(objToPrint); From 8f5575883cbabc4b55cbb697ea1d257f77523e59 Mon Sep 17 00:00:00 2001 From: Christos Otarola Date: Mon, 20 Feb 2023 11:51:40 -0500 Subject: [PATCH 16/16] fix: small suggestions --- test/Collector.test.ts | 10 +++++----- 1 file changed, 5 insertions(+), 5 deletions(-) diff --git a/test/Collector.test.ts b/test/Collector.test.ts index e3b6a410..6335d4e2 100644 --- a/test/Collector.test.ts +++ b/test/Collector.test.ts @@ -596,9 +596,11 @@ describe('Collector', function () { }); it('Should fail when not enough revenue to share in any of the tokens', async function () { + const lastTokenIndex = fakeERC20Tokens.length - 1; const tokens = fakeERC20Tokens.map((token, i) => { - const isLastToken = fakeERC20Tokens.length - 1; - token.balanceOf.returns(partners.length - (i === isLastToken ? 1 : 0)); + token.balanceOf.returns( + partners.length - (i === lastTokenIndex ? 1 : 0) + ); return token.address; }); @@ -672,9 +674,7 @@ describe('Collector', function () { }); it('Should fail when not enough revenue to share', async function () { - const tokens = fakeERC20Tokens.map((token) => { - return token.address; - }); + const tokens = fakeERC20Tokens.map(({ address }) => address); const token = fakeERC20Tokens[0]; token.balanceOf.returns(partners.length - 1); const collector = await deployCollector({ tokens });