Merge pull request #113 from OpenQDev/fix-review-token-whitelist

Fix review - removes TOKEN_ADDRESS_LIMIT logic in favor of a simple token whitelist
OpenQDev · Feb 28, 2023 · 26c11e0 · 26c11e0
2 parents c14e922 + b32f0b2
commit 26c11e0
Show file tree

Hide file tree

Showing 8 changed files with 11 additions and 84 deletions.
diff --git a/contracts/DepositManager/Implementations/DepositManagerV1.sol b/contracts/DepositManager/Implementations/DepositManagerV1.sol
@@ -43,12 +43,7 @@ contract DepositManagerV1 is DepositManagerStorageV1 {
     ) external payable onlyProxy {
         IBounty bounty = IBounty(payable(_bountyAddress));
 
-        if (!isWhitelisted(_tokenAddress)) {
-            require(
-                !tokenAddressLimitReached(_bountyAddress),
-                Errors.TOO_MANY_TOKEN_ADDRESSES
-            );
-        }
+        require(isWhitelisted(_tokenAddress), Errors.TOKEN_NOT_ACCEPTED);
 
         require(bountyIsOpen(_bountyAddress), Errors.CONTRACT_ALREADY_CLOSED);
 
@@ -202,21 +197,6 @@ contract DepositManagerV1 is DepositManagerStorageV1 {
         return openQTokenWhitelist.isWhitelisted(_tokenAddress);
     }
 
-    /// @notice Returns true if the total number of unique tokens deposited on then bounty is greater than the OpenQWhitelist TOKEN_ADDRESS_LIMIT
-    /// @param _bountyAddress Address of bounty
-    /// @return True if the token address limit has been reached
-    function tokenAddressLimitReached(address _bountyAddress)
-        public
-        view
-        returns (bool)
-    {
-        IBounty bounty = IBounty(payable(_bountyAddress));
-
-        return
-            bounty.getTokenAddressesCount() >=
-            openQTokenWhitelist.TOKEN_ADDRESS_LIMIT();
-    }
-
     /// @notice Checks if bounty associated with _bountyId is open
     /// @param _bountyAddress Address of bounty
     /// @return bool True if _bountyId is associated with an open bounty

diff --git a/contracts/TokenWhitelist/OpenQTokenWhitelist.sol b/contracts/TokenWhitelist/OpenQTokenWhitelist.sol
@@ -9,8 +9,5 @@ import './TokenWhitelist.sol';
 /// @dev Whitelisting and token address limit is implemented primarily as a means of preventing out-of-gas exceptions when looping over funded addresses for payouts
 contract OpenQTokenWhitelist is TokenWhitelist {
     /// @notice Initializes OpenQTokenWhitelist with maximum token address limit to prevent out-of-gas errors
-    /// @param _tokenAddressLimit Maximum number of token addresses allowed
-    constructor(uint256 _tokenAddressLimit) TokenWhitelist() {
-        TOKEN_ADDRESS_LIMIT = _tokenAddressLimit;
-    }
+    constructor() TokenWhitelist() {}
 }
diff --git a/contracts/TokenWhitelist/TokenWhitelist.sol b/contracts/TokenWhitelist/TokenWhitelist.sol
@@ -9,7 +9,6 @@ import '../Library/Errors.sol';
 /// @notice Base contract for token whitelists
 /// @dev Whitelisting and token address limit is implemented primarily as a means of preventing out-of-gas exceptions when looping over funded addresses for payouts
 abstract contract TokenWhitelist is Ownable {
-    uint256 public TOKEN_ADDRESS_LIMIT;
     uint256 public tokenCount;
     mapping(address => bool) public whitelist;
 
@@ -41,13 +40,4 @@ abstract contract TokenWhitelist is Ownable {
         whitelist[_tokenAddress] = false;
         tokenCount--;
     }
-
-    /// @notice Updates the tokenAddressLimit
-    /// @param _newTokenAddressLimit The new value for TOKEN_ADDRESS_LIMIT
-    function setTokenAddressLimit(uint256 _newTokenAddressLimit)
-        external
-        onlyOwner
-    {
-        TOKEN_ADDRESS_LIMIT = _newTokenAddressLimit;
-    }
 }
diff --git a/deploy/deploy_contracts.js b/deploy/deploy_contracts.js
@@ -117,7 +117,7 @@ async function deployContracts() {
 
 	console.log('Deploying OpenQTokenWhitelist...');
 	const OpenQTokenWhitelist = await ethers.getContractFactory('OpenQTokenWhitelist');
-	const openQTokenWhitelist = await OpenQTokenWhitelist.deploy(5);
+	const openQTokenWhitelist = await OpenQTokenWhitelist.deploy();
 	await openQTokenWhitelist.deployed();
 	await optionalSleep(10000);
 	console.log(`OpenQTokenWhitelist Deployed to ${openQTokenWhitelist.address}\n`);

diff --git a/test/ClaimManager.test.js b/test/ClaimManager.test.js
@@ -119,7 +119,7 @@ describe('ClaimManager.sol', () => {
 		mockNft = await MockNft.deploy();
 		await mockNft.deployed();
 
-		openQTokenWhitelist = await OpenQTokenWhitelist.deploy(5);
+		openQTokenWhitelist = await OpenQTokenWhitelist.deploy();
 		await openQTokenWhitelist.deployed();
 
 		await openQTokenWhitelist.addToken(mockLink.address);

diff --git a/test/DepositManager.test.js b/test/DepositManager.test.js
@@ -15,7 +15,7 @@ const {
 	tieredFixedBountyInitOperationBuilder_permissionless
 } = require('./constants');
 
-describe('DepositManager.sol', () => {
+describe.only('DepositManager.sol', () => {
 	// MOCK ASSETS
 	let openQProxy;
 	let openQImplementation;
@@ -98,7 +98,7 @@ describe('DepositManager.sol', () => {
 		mockNft = await MockNft.deploy();
 		await mockNft.deployed();
 
-		openQTokenWhitelist = await OpenQTokenWhitelist.deploy(5);
+		openQTokenWhitelist = await OpenQTokenWhitelist.deploy();
 		await openQTokenWhitelist.deployed();
 
 		await openQTokenWhitelist.addToken(mockLink.address);
@@ -192,7 +192,7 @@ describe('DepositManager.sol', () => {
 
 			// ARRANGE
 			const OpenQTokenWhitelist = await ethers.getContractFactory('OpenQTokenWhitelist');
-			const openQTokenWhitelist = await OpenQTokenWhitelist.deploy(20);
+			const openQTokenWhitelist = await OpenQTokenWhitelist.deploy();
 			await openQTokenWhitelist.deployed();
 
 			// ACT
@@ -217,7 +217,7 @@ describe('DepositManager.sol', () => {
 			await expect(depositManager.fundBountyToken(bountyAddress, mockLink.address, 10000000, 1, Constants.funderUuid)).to.be.revertedWith('CONTRACT_ALREADY_CLOSED');
 		});
 
-		it('should revert if funded with a non-whitelisted token and bounty is at funded token address capacity', async () => {
+		it('should revert if funded with a non-whitelisted token', async () => {
 			// ARRANGE
 			await openQProxy.mintBounty(Constants.bountyId, Constants.organization, atomicBountyInitOperation);
 
@@ -226,31 +226,10 @@ describe('DepositManager.sol', () => {
 			await blacklistedMockDai.approve(bountyAddress, 10000000);
 			await mockLink.approve(bountyAddress, 10000000);
 
-			// set lower capacity for token
-			await openQTokenWhitelist.setTokenAddressLimit(1);
-
-			await depositManager.fundBountyToken(bountyAddress, mockLink.address, 10000000, 1, Constants.funderUuid);
-
-			// ACT + ASSERT
-			await expect(depositManager.fundBountyToken(bountyAddress, blacklistedMockDai.address, 10000000, 1, Constants.funderUuid)).to.be.revertedWith('TOO_MANY_TOKEN_ADDRESSES');
-		});
-
-		it('should ALLOW funding with whitelisted token EVEN IF bounty is at funded token address capacity', async () => {
-			// ARRANGE
-			await openQProxy.mintBounty(Constants.bountyId, Constants.organization, atomicBountyInitOperation);
-
-			const bountyAddress = await openQProxy.bountyIdToAddress(Constants.bountyId);
-
-			await mockDai.approve(bountyAddress, 10000000);
-			await mockLink.approve(bountyAddress, 10000000);
-
-			// set lower capacity for token
-			await openQTokenWhitelist.setTokenAddressLimit(1);
-
 			await depositManager.fundBountyToken(bountyAddress, mockLink.address, 10000000, 1, Constants.funderUuid);
 
 			// ACT + ASSERT
-			await expect(depositManager.fundBountyToken(bountyAddress, mockDai.address, 10000000, 1, Constants.funderUuid)).to.not.be.revertedWith('TOO_MANY_TOKEN_ADDRESSES');
+			await expect(depositManager.fundBountyToken(bountyAddress, blacklistedMockDai.address, 10000000, 1, Constants.funderUuid)).to.be.revertedWith('TOKEN_NOT_ACCEPTED');
 		});
 
 		it('should set funder to msg.sender', async () => {

diff --git a/test/OpenQ.test.js b/test/OpenQ.test.js
@@ -127,7 +127,7 @@ describe('OpenQ.sol', () => {
     mockNft = await MockNft.deploy()
     await mockNft.deployed()
 
-    openQTokenWhitelist = await OpenQTokenWhitelist.deploy(5)
+    openQTokenWhitelist = await OpenQTokenWhitelist.deploy()
     await openQTokenWhitelist.deployed()
 
     await openQTokenWhitelist.addToken(mockLink.address)

diff --git a/test/OpenQTokenWhitelist.test.js b/test/OpenQTokenWhitelist.test.js
@@ -20,7 +20,7 @@ describe('OpenQTokenWhitelist.sol', () => {
 
 		[owner, notOwner] = await ethers.getSigners();
 
-		openQTokenWhitelist = await OpenQTokenWhitelist.deploy(2);
+		openQTokenWhitelist = await OpenQTokenWhitelist.deploy();
 		await openQTokenWhitelist.deployed();
 
 		mockLink = await MockLink.deploy();
@@ -31,11 +31,6 @@ describe('OpenQTokenWhitelist.sol', () => {
 	});
 
 	describe('OpenQTokenWhitelist methods', () => {
-		it('initializes', async () => {
-			const totalTokenAddresses = await openQTokenWhitelist.TOKEN_ADDRESS_LIMIT();
-			expect(totalTokenAddresses).to.equal(2);
-		});
-
 		it('can add token', async () => {
 			// ASSUME
 			let mockLinkWhitelist = await openQTokenWhitelist.isWhitelisted(mockLink.address);
@@ -89,19 +84,6 @@ describe('OpenQTokenWhitelist.sol', () => {
 			expect(tokenCount).to.equal(0);
 		});
 
-		it('increases token address limit', async () => {
-			// ASSUME
-			let tokenAddressLimit = await openQTokenWhitelist.TOKEN_ADDRESS_LIMIT();
-			expect(tokenAddressLimit).to.equal(2);
-
-			// ACT
-			await openQTokenWhitelist.setTokenAddressLimit(5);
-
-			// ASSERT
-			tokenAddressLimit = await openQTokenWhitelist.TOKEN_ADDRESS_LIMIT();
-			expect(tokenAddressLimit).to.equal(5);
-		});
-
 		it('add and remove token is idempotent', async () => {
 			// ARRANGE
 			await openQTokenWhitelist.addToken(mockLink.address);
@@ -116,7 +98,6 @@ describe('OpenQTokenWhitelist.sol', () => {
 			// ACT/ASSERT
 			await expect(openQTokenWhitelist.connect(notOwner).addToken(mockLink.address)).to.revertedWith('Ownable: caller is not the owner');
 			await expect(openQTokenWhitelist.connect(notOwner).removeToken(mockLink.address)).to.revertedWith('Ownable: caller is not the owner');
-			await expect(openQTokenWhitelist.connect(notOwner).setTokenAddressLimit(5)).to.revertedWith('Ownable: caller is not the owner');
 		});
 	});
 });