Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Prevent StreamingPayment uninstallation if pending streams remain #1251

Draft
wants to merge 5 commits into
base: develop
Choose a base branch
from

Conversation

area
Copy link
Member

@area area commented May 8, 2024

The main feature here is to prevent uninstallation of the extension in the event that there are some streaming payments unclaimed. It also:

  • Reworks some calculations to prevent over/underflows. This is somewhat opinionated in that the intention is to 'cap' the calculations
  • Allows editing of the interval, which is a requirement. While in an ideal world, editing the interval shouldn't be necessary as you could edit the amount, rounding errors meant that this should be editable as well. This is possible as we have moved to single-stream payments.

@area area force-pushed the maint/single-stream-payments branch from 1a54bad to a17d2b7 Compare May 8, 2024 15:51
@area area force-pushed the feat/no-uninstall-pending-streams branch from f75fde4 to d63d67b Compare May 8, 2024 15:51
@area area force-pushed the maint/single-stream-payments branch from a17d2b7 to 9fddd41 Compare May 29, 2024 10:43
@area area force-pushed the feat/no-uninstall-pending-streams branch 2 times, most recently from 53b7bbf to 58d4d6e Compare May 29, 2024 13:43
@area area force-pushed the maint/single-stream-payments branch from fc1cdaa to a78f8c2 Compare May 30, 2024 07:52
@area area force-pushed the feat/no-uninstall-pending-streams branch from 1929430 to 78ab0ee Compare May 30, 2024 07:53
@area area force-pushed the maint/single-stream-payments branch 2 times, most recently from c0ee585 to 5a5ae87 Compare June 5, 2024 08:35
@area area force-pushed the feat/no-uninstall-pending-streams branch from 78ab0ee to 273ee11 Compare June 5, 2024 08:36
@area area force-pushed the maint/single-stream-payments branch from 5a5ae87 to 89fe76c Compare June 6, 2024 16:22
@area area force-pushed the feat/no-uninstall-pending-streams branch from 273ee11 to 943a097 Compare June 6, 2024 16:23
Copy link
Member

@kronosapiens kronosapiens left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is a lot of new code just to implement an uninstall check, but so it goes I guess

@@ -184,6 +191,10 @@ contract StreamingPayments is ColonyExtensionMeta {
0
);

if (getAmountClaimableLifetime(numStreamingPayments) > 0) {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is this check necessary? Why would we allow a streaming payment to be created which was worth nothing?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We already allowed it, so I had to accommodate it. The alternative would be adding a check instead of this, so it feels like much of a wash to me. In addition, there's nothing stopping you editing a stream you create down to being worth nothing, so preventing creation feels quite arbitrary?

contracts/extensions/StreamingPayments.sol Outdated Show resolved Hide resolved
@area area force-pushed the feat/no-uninstall-pending-streams branch from 943a097 to d168916 Compare June 10, 2024 09:20
Base automatically changed from maint/single-stream-payments to develop June 10, 2024 13:26
@area area force-pushed the feat/no-uninstall-pending-streams branch from d168916 to 295f2e2 Compare June 10, 2024 16:39
@area area force-pushed the feat/no-uninstall-pending-streams branch from 295f2e2 to de82deb Compare June 10, 2024 17:10
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants