Tidy up nUnresolvedPayments edge cases; allow editing of interval

contracts/extensions/StreamingPayments.sol

@@ -34,7 +34,12 @@ contract StreamingPayments is ColonyExtensionMeta {
     address token,
     uint256 amount
   );
-  event PaymentTokenUpdated(address agent, uint256 indexed streamingPaymentId, uint256 amount);
+  event PaymentTokenUpdated(
+    address agent,
+    uint256 indexed streamingPaymentId,
+    uint256 amount,
+    uint256 interval
+  );
   event StartTimeSet(address agent, uint256 indexed streamingPaymentId, uint256 startTime);
   event EndTimeSet(address agent, uint256 indexed streamingPaymentId, uint256 endTime);
   event ClaimWaived(address agent, uint256 indexed streamingPaymentId);
@@ -258,6 +263,7 @@ contract StreamingPayments is ColonyExtensionMeta {
   /// @param _toChildSkillIndex The linking the domainId to the toPot domain
   /// @param _id The id of the streaming payment
   /// @param _amount The new amount to pay out
+  /// @param _interval The new interval over which _amount is paid out
   // slither-disable-next-line reentrancy-no-eth
   function setTokenAmount(
     uint256 _fundingPermissionDomainId,
@@ -267,7 +273,8 @@ contract StreamingPayments is ColonyExtensionMeta {
     uint256 _fromChildSkillIndex,
     uint256 _toChildSkillIndex,
     uint256 _id,
-    uint256 _amount
+    uint256 _amount,
+    uint256 _interval
   )
     public
     validateFundingPermission(
@@ -276,28 +283,36 @@ contract StreamingPayments is ColonyExtensionMeta {
       streamingPayments[_id].domainId
     )
   {
-    claim(_permissionDomainId, _childSkillIndex, _fromChildSkillIndex, _toChildSkillIndex, _id);
     StreamingPayment storage streamingPayment = streamingPayments[_id];
+    if (streamingPayment.startTime < block.timestamp) {
+      claim(_permissionDomainId, _childSkillIndex, _fromChildSkillIndex, _toChildSkillIndex, _id);
+      // This require checks that the above claim paid out the full amount the recipient is entitled to
+      // before any changes are made.
+      require(
+        streamingPayment.pseudoAmountClaimedFromStart >= getAmountEntitledFromStart(_id),
+        "streaming-payments-insufficient-funds"
+      );
+    }
+
+    bool wasResolved = streamingPayment.pseudoAmountClaimedFromStart >=
+      getAmountClaimableLifetime(_id);
 
-    // This require checks that the above claim paid out the full amount the recipient is entitled to
-    // before any changes are made.
-    require(
-      streamingPayment.pseudoAmountClaimedFromStart >= getAmountEntitledFromStart(_id),
-      "streaming-payments-insufficient-funds"
-    );
     streamingPayment.amount = _amount;
+    streamingPayment.interval = _interval;
 
     // Update 'claimed' as if we've had this rate since the beginning
     streamingPayment.pseudoAmountClaimedFromStart = getAmountEntitledFromStart(_id);
 
-    // Note that if we're at this point, the payment prior to our editing _must_ have been resolved
-    // So there's no way we're going from an unresolved payment to a still-unresolved payment and accidentally
-    // incrementing this when we shouldn't.
-    if (getAmountClaimableLifetime(_id) >= streamingPayment.pseudoAmountClaimedFromStart) {
+    bool isResolved = streamingPayment.pseudoAmountClaimedFromStart >=
+      getAmountClaimableLifetime(_id);
+
+    if (wasResolved && !isResolved) {
       nUnresolvedStreamingPayments += 1;
+    } else if (!wasResolved && isResolved) {
+      nUnresolvedStreamingPayments -= 1;
     }
 
-    emit PaymentTokenUpdated(msgSender(), _id, _amount);
+    emit PaymentTokenUpdated(msgSender(), _id, _amount, _interval);
   }
 
   /// @notice Update the startTime, only if the current startTime is in the future
@@ -371,7 +386,8 @@ contract StreamingPayments is ColonyExtensionMeta {
 
     uint256 newLifetimeClaimable = getAmountClaimableLifetime(_id);
 
-    // Unlike when we're setting start time, we need to check if the payment is unresolved here.
+    // Unlike when we're setting start time, we need to compare to pseudoAmountClaimedFromStart
+    // in order to determine if the payment is resolved or not.
     bool wasResolved = streamingPayment.pseudoAmountClaimedFromStart >= oldLifetimeClaimable;
     bool isResolved = streamingPayment.pseudoAmountClaimedFromStart >= newLifetimeClaimable;
 

docs/interfaces/extensions/streamingpayments.md

@@ -221,7 +221,7 @@ Update the startTime, only if the current startTime is in the future
 |_startTime|uint256|The new startTime to set
 
 
-### ▸ `setTokenAmount(uint256 _fundingPermissionDomainId, uint256 _fundingChildSkillIndex, uint256 _permissionDomainId, uint256 _childSkillIndex, uint256 _fromChildSkillIndex, uint256 _toChildSkillIndex, uint256 _id, uint256 _amount)`
+### ▸ `setTokenAmount(uint256 _fundingPermissionDomainId, uint256 _fundingChildSkillIndex, uint256 _permissionDomainId, uint256 _childSkillIndex, uint256 _fromChildSkillIndex, uint256 _toChildSkillIndex, uint256 _id, uint256 _amount, uint256 _interval)`
 
 Update the token amount to be paid out. Claims existing payout prior to the change
 
@@ -238,6 +238,7 @@ Update the token amount to be paid out. Claims existing payout prior to the chan
 |_toChildSkillIndex|uint256|The linking the domainId to the toPot domain
 |_id|uint256|The id of the streaming payment
 |_amount|uint256|The new amount to pay out
+|_interval|uint256|The new interval over which _amount is paid out
 
 
 ### ▸ `uninstall()`

test/extensions/streaming-payments.js

@@ -604,7 +604,7 @@ contract("Streaming Payments", (accounts) => {
 
       // Claim one wad
       balancePre = await token.balanceOf(USER1);
-      const updateArgs = [1, UINT256_MAX, 1, UINT256_MAX, UINT256_MAX, UINT256_MAX, streamingPaymentId, WAD.muln(2)];
+      const updateArgs = [1, UINT256_MAX, 1, UINT256_MAX, UINT256_MAX, UINT256_MAX, streamingPaymentId, WAD.muln(2), SECONDS_PER_DAY];
       await makeTxAtTimestamp(streamingPayments.setTokenAmount, updateArgs, blockTime + SECONDS_PER_DAY, this);
       balancePost = await token.balanceOf(USER1);
       expect(balancePost.sub(balancePre)).to.eq.BN(WAD.muln(1).subn(1)); // -1 for network fee
@@ -627,7 +627,7 @@ contract("Streaming Payments", (accounts) => {
       await forwardTime(SECONDS_PER_DAY, this);
 
       await checkErrorRevert(
-        streamingPayments.setTokenAmount(1, UINT256_MAX, 1, UINT256_MAX, UINT256_MAX, UINT256_MAX, streamingPaymentId, WAD.muln(2)),
+        streamingPayments.setTokenAmount(1, UINT256_MAX, 1, UINT256_MAX, UINT256_MAX, UINT256_MAX, streamingPaymentId, WAD.muln(2), SECONDS_PER_DAY),
         "streaming-payments-insufficient-funds",
       );
     });
@@ -802,5 +802,105 @@ contract("Streaming Payments", (accounts) => {
       n = await streamingPayments.getNUnresolvedStreamingPayments();
       expect(n).to.eq.BN(0);
     });
+
+    it("various edge cases for nUnresolvedPayments are treated correctly when setting token amounts", async () => {
+      await fundColonyWithTokens(colony, token, WAD.muln(10));
+
+      const blockTime = await getBlockTime();
+
+      await streamingPayments.create(
+        1,
+        UINT256_MAX,
+        1,
+        UINT256_MAX,
+        1,
+        blockTime + SECONDS_PER_DAY,
+        blockTime + SECONDS_PER_DAY + 2,
+        SECONDS_PER_DAY,
+        USER1,
+        token.address,
+        1,
+      );
+      const streamingPaymentId = await streamingPayments.getNumStreamingPayments();
+
+      let nUnresolvedPayments = await streamingPayments.getNUnresolvedStreamingPayments();
+      expect(nUnresolvedPayments).to.eq.BN(0);
+
+      await streamingPayments.setTokenAmount(1, UINT256_MAX, 1, UINT256_MAX, 1, UINT256_MAX, streamingPaymentId, SECONDS_PER_DAY, SECONDS_PER_DAY);
+      // Should now pay out 1 token
+
+      nUnresolvedPayments = await streamingPayments.getNUnresolvedStreamingPayments();
+      expect(nUnresolvedPayments).to.eq.BN(1);
+
+      // No change expected
+      await streamingPayments.setTokenAmount(1, UINT256_MAX, 1, UINT256_MAX, 1, UINT256_MAX, streamingPaymentId, SECONDS_PER_DAY, SECONDS_PER_DAY);
+      nUnresolvedPayments = await streamingPayments.getNUnresolvedStreamingPayments();
+      expect(nUnresolvedPayments).to.eq.BN(1);
+
+      // Reduce payout
+      await streamingPayments.setTokenAmount(1, UINT256_MAX, 1, UINT256_MAX, 1, UINT256_MAX, streamingPaymentId, 1, SECONDS_PER_DAY);
+      nUnresolvedPayments = await streamingPayments.getNUnresolvedStreamingPayments();
+      expect(nUnresolvedPayments).to.eq.BN(0);
+
+      // Set payout again
+      await streamingPayments.setTokenAmount(1, UINT256_MAX, 1, UINT256_MAX, 1, UINT256_MAX, streamingPaymentId, SECONDS_PER_DAY, SECONDS_PER_DAY);
+      nUnresolvedPayments = await streamingPayments.getNUnresolvedStreamingPayments();
+      expect(nUnresolvedPayments).to.eq.BN(1);
+
+      // Forward to way past the end of the payment
+      await forwardTime(SECONDS_PER_DAY * 2, this);
+
+      await streamingPayments.claim(1, UINT256_MAX, UINT256_MAX, UINT256_MAX, streamingPaymentId);
+      nUnresolvedPayments = await streamingPayments.getNUnresolvedStreamingPayments();
+      expect(nUnresolvedPayments).to.eq.BN(0);
+
+      await streamingPayments.claim(1, UINT256_MAX, UINT256_MAX, UINT256_MAX, streamingPaymentId);
+      nUnresolvedPayments = await streamingPayments.getNUnresolvedStreamingPayments();
+      expect(nUnresolvedPayments).to.eq.BN(0);
+
+      await streamingPayments.setTokenAmount(1, UINT256_MAX, 1, UINT256_MAX, 1, UINT256_MAX, streamingPaymentId, SECONDS_PER_DAY, SECONDS_PER_DAY);
+      // Still only pays out one token, but already claimed, so should be resolved
+      nUnresolvedPayments = await streamingPayments.getNUnresolvedStreamingPayments();
+      expect(nUnresolvedPayments).to.eq.BN(0);
+
+      await streamingPayments.setTokenAmount(1, UINT256_MAX, 1, UINT256_MAX, 1, UINT256_MAX, streamingPaymentId, WAD, SECONDS_PER_DAY);
+      // Would have paid out more, but updating claims only affects future payouts, and this payment was already finished
+      // and resolved, so no change expected
+      nUnresolvedPayments = await streamingPayments.getNUnresolvedStreamingPayments();
+      expect(nUnresolvedPayments).to.eq.BN(0);
+    });
+
+    it("various edge cases for nUnresolvedPayments are treated correctly when setting start and end times", async () => {
+      await fundColonyWithTokens(colony, token, WAD.muln(10));
+
+      const blockTime = await getBlockTime();
+
+      await streamingPayments.create(
+        1,
+        UINT256_MAX,
+        1,
+        UINT256_MAX,
+        1,
+        blockTime + SECONDS_PER_DAY,
+        blockTime + SECONDS_PER_DAY + 1,
+        SECONDS_PER_DAY,
+        USER1,
+        token.address,
+        1,
+      );
+
+      const streamingPaymentId = await streamingPayments.getNumStreamingPayments();
+
+      let nUnresolvedPayments = await streamingPayments.getNUnresolvedStreamingPayments();
+      expect(nUnresolvedPayments).to.eq.BN(0);
+
+      await streamingPayments.setStartTime(1, UINT256_MAX, streamingPaymentId, blockTime + SECONDS_PER_DAY);
+      nUnresolvedPayments = await streamingPayments.getNUnresolvedStreamingPayments();
+      expect(nUnresolvedPayments).to.eq.BN(0);
+
+      await streamingPayments.setEndTime(1, UINT256_MAX, streamingPaymentId, blockTime + SECONDS_PER_DAY + 1);
+      nUnresolvedPayments = await streamingPayments.getNUnresolvedStreamingPayments();
+      expect(nUnresolvedPayments).to.eq.BN(0);
+    });
   });
 });