From 58d4d6ead9ec4be8f7346361650dd8ed8cec9b7d Mon Sep 17 00:00:00 2001 From: Alex Rea Date: Mon, 29 Apr 2024 12:23:04 +0100 Subject: [PATCH] Tidy up nUnresolvedPayments edge cases; allow editing of interval --- contracts/extensions/StreamingPayments.sol | 46 +++++--- .../extensions/streamingpayments.md | 3 +- test/extensions/streaming-payments.js | 104 +++++++++++++++++- 3 files changed, 135 insertions(+), 18 deletions(-) diff --git a/contracts/extensions/StreamingPayments.sol b/contracts/extensions/StreamingPayments.sol index 92e1437a72..7a0415f676 100644 --- a/contracts/extensions/StreamingPayments.sol +++ b/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; diff --git a/docs/interfaces/extensions/streamingpayments.md b/docs/interfaces/extensions/streamingpayments.md index 34a5c2282f..99019e3827 100644 --- a/docs/interfaces/extensions/streamingpayments.md +++ b/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()` diff --git a/test/extensions/streaming-payments.js b/test/extensions/streaming-payments.js index 861d8810c3..f400099b61 100644 --- a/test/extensions/streaming-payments.js +++ b/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); + }); }); });