Improvements following review

Author: area

contracts/colony/ColonyFunding.sol

@@ -111,8 +111,6 @@ contract ColonyFunding is
     require(domainExists(_domainId), "colony-funding-domain-does-not-exist");
     address domainTokenReceiverAddress = IColonyNetwork(colonyNetworkAddress)
       .idempotentDeployDomainTokenReceiver(_domainId);
-    uint256 fundingPotId = domains[_domainId].fundingPotId;
-    // It's deployed, so check current balance of pot
 
     uint256 claimAmount;
 
@@ -131,6 +129,7 @@ contract ColonyFunding is
 
     fundingPots[0].balance[_token] += feeToPay;
 
+    uint256 fundingPotId = domains[_domainId].fundingPotId;
     uint256 approvedAmount = domainReputationApproval[_domainId];
 
     if (tokenEarnsReputationOnPayout(_token)) {
@@ -139,19 +138,22 @@ contract ColonyFunding is
 
       fundingPots[fundingPotId].balance[_token] += transferrableAmount;
       domainReputationApproval[_domainId] -= transferrableAmount;
+
       emit DomainFundsClaimed(msgSender(), _token, _domainId, feeToPay, transferrableAmount);
       if (untransferrableAmount > 0) {
         fundingPots[domains[1].fundingPotId].balance[_token] += untransferrableAmount;
+
         emit ColonyFundsClaimed(msgSender(), _token, 0, untransferrableAmount);
       }
     } else {
       fundingPots[fundingPotId].balance[_token] += remainder;
+
       emit DomainFundsClaimed(msgSender(), _token, _domainId, feeToPay, remainder);
     }
 
     // Claim funds
     if (_token == address(0x0)) {
-      DomainTokenReceiver(domainTokenReceiverAddress).transferNativeToColony();
+      DomainTokenReceiver(domainTokenReceiverAddress).transferChainNativeToColony();
     } else {
       DomainTokenReceiver(domainTokenReceiverAddress).approveTokenToColony(_token);
       // slither-disable-next-line arbitrary-send-erc20
@@ -163,6 +165,7 @@ contract ColonyFunding is
   }
 
   function tokenEarnsReputationOnPayout(address _token) internal view returns (bool) {
+    // This function is in anticipation of multiple tokens being able to earn reputation on payout.
     return _token == token;
   }
 
@@ -174,6 +177,8 @@ contract ColonyFunding is
     require(domainExists(_domainId), "colony-funding-domain-does-not-exist");
     require(_domainId > 1, "colony-funding-root-domain");
     if (_add) {
+      // Rather than setting the approval to the amount, we add the amount to the approval, so that if there
+      // are multiple calls to this function via motions etc, the approval will be the sum of all the amounts.
       domainReputationApproval[_domainId] += _amount;
     } else {
       domainReputationApproval[_domainId] -= _amount;

contracts/common/DomainTokenReceiver.sol

@@ -39,7 +39,7 @@ contract DomainTokenReceiver is DSAuth {
     colony = _colony;
   }
 
-  function transferNativeToColony() public onlyColony {
+  function transferChainNativeToColony() public onlyColony {
     payable(colony).transfer(address(this).balance);
   }
 

test/contracts-network/colony-funding.js

@@ -656,7 +656,7 @@ contract("Colony Funding", (accounts) => {
 
       const receiver = await DomainTokenReceiver.at(receiverAddress);
       await checkErrorRevert(receiver.approveTokenToColony(otherToken.address), "domain-token-receiver-unauthorized");
-      await checkErrorRevert(receiver.transferNativeToColony(), "domain-token-receiver-unauthorized");
+      await checkErrorRevert(receiver.transferChainNativeToColony(), "domain-token-receiver-unauthorized");
     });
 
     it("should not allow even the colonyNetwork to call setColonyAddress once it's set on domainTokenReceiver", async () => {
@@ -700,6 +700,47 @@ contract("Colony Funding", (accounts) => {
       expect(rootDomainPotBalanceAfter.sub(rootDomainPotBalanceBefore)).to.eq.BN(99);
     });
 
+    it(`when receiving native (reputation-earning) token, if full approval present for domain,
+      tokens are received by domain`, async () => {
+      // Get address for domain 2
+      await colony.addDomain(1, UINT256_MAX, 1);
+      const receiverAddress = await colonyNetwork.getDomainTokenReceiverAddress(colony.address, 2);
+      await colony.mintTokens(WAD.muln(100));
+      await colony.claimColonyFunds(token.address);
+      const domain1 = await colony.getDomain(1);
+
+      // Send an arbitrary transaction to mint tokens for receiverAddress
+      const txData = token.contract.methods["mint(address,uint256)"](receiverAddress, 100).encodeABI();
+      await colony.makeArbitraryTransaction(token.address, txData);
+
+      // Approve 250 for the domain
+      await colony.editAllowedDomainReputationReceipt(2, 250, true);
+      let allowedReceipt = await colony.getAllowedDomainReputationReceipt(2);
+      expect(allowedReceipt).to.eq.BN(250);
+
+      // Now test what happens when we claim them
+
+      const domain = await colony.getDomain(2);
+      const domainPotBalanceBefore = await colony.getFundingPotBalance(domain.fundingPotId, token.address);
+      const nonRewardPotsTotalBefore = await colony.getNonRewardPotsTotal(token.address);
+      const rootDomainPotBalanceBefore = await colony.getFundingPotBalance(domain1.fundingPotId, token.address);
+
+      // Claim the funds
+      await colony.claimDomainFunds(token.address, 2);
+
+      const domainPotBalanceAfter = await colony.getFundingPotBalance(domain.fundingPotId, token.address);
+      const nonRewardPotsTotalAfter = await colony.getNonRewardPotsTotal(token.address);
+      const rootDomainPotBalanceAfter = await colony.getFundingPotBalance(domain1.fundingPotId, token.address);
+
+      // Check the balance of the domain
+      expect(domainPotBalanceAfter.sub(domainPotBalanceBefore)).to.eq.BN(99);
+      expect(nonRewardPotsTotalAfter.sub(nonRewardPotsTotalBefore)).to.eq.BN(99);
+      expect(rootDomainPotBalanceAfter.sub(rootDomainPotBalanceBefore)).to.eq.BN(0);
+
+      allowedReceipt = await colony.getAllowedDomainReputationReceipt(2);
+      expect(allowedReceipt).to.eq.BN(151);
+    });
+
     it(`when receiving native (reputation-earning) token, if partial approval present for domain,
       tokens are split between intended domain and root`, async () => {
       // Get address for domain 2
@@ -769,47 +810,6 @@ contract("Colony Funding", (accounts) => {
       await checkErrorRevert(colony.editAllowedDomainReputationReceipt(1, 70, true), "colony-funding-root-domain");
     });
 
-    it(`when receiving native (reputation-earning) token, if full approval present for domain,
-      tokens are received by domain`, async () => {
-      // Get address for domain 2
-      await colony.addDomain(1, UINT256_MAX, 1);
-      const receiverAddress = await colonyNetwork.getDomainTokenReceiverAddress(colony.address, 2);
-      await colony.mintTokens(WAD.muln(100));
-      await colony.claimColonyFunds(token.address);
-      const domain1 = await colony.getDomain(1);
-
-      // Send an arbitrary transaction to mint tokens for receiverAddress
-      const txData = token.contract.methods["mint(address,uint256)"](receiverAddress, 100).encodeABI();
-      await colony.makeArbitraryTransaction(token.address, txData);
-
-      // Approve 250 for the domain
-      await colony.editAllowedDomainReputationReceipt(2, 250, true);
-      let allowedReceipt = await colony.getAllowedDomainReputationReceipt(2);
-      expect(allowedReceipt).to.eq.BN(250);
-
-      // Now test what happens when we claim them
-
-      const domain = await colony.getDomain(2);
-      const domainPotBalanceBefore = await colony.getFundingPotBalance(domain.fundingPotId, token.address);
-      const nonRewardPotsTotalBefore = await colony.getNonRewardPotsTotal(token.address);
-      const rootDomainPotBalanceBefore = await colony.getFundingPotBalance(domain1.fundingPotId, token.address);
-
-      // Claim the funds
-      await colony.claimDomainFunds(token.address, 2);
-
-      const domainPotBalanceAfter = await colony.getFundingPotBalance(domain.fundingPotId, token.address);
-      const nonRewardPotsTotalAfter = await colony.getNonRewardPotsTotal(token.address);
-      const rootDomainPotBalanceAfter = await colony.getFundingPotBalance(domain1.fundingPotId, token.address);
-
-      // Check the balance of the domain
-      expect(domainPotBalanceAfter.sub(domainPotBalanceBefore)).to.eq.BN(99);
-      expect(nonRewardPotsTotalAfter.sub(nonRewardPotsTotalBefore)).to.eq.BN(99);
-      expect(rootDomainPotBalanceAfter.sub(rootDomainPotBalanceBefore)).to.eq.BN(0);
-
-      allowedReceipt = await colony.getAllowedDomainReputationReceipt(2);
-      expect(allowedReceipt).to.eq.BN(151);
-    });
-
     it("should not be able to claim funds for a domain that does not exist", async () => {
       await checkErrorRevert(colony.claimDomainFunds(ethers.constants.AddressZero, 2), "colony-funding-domain-does-not-exist");
     });