Add fixed subscription duration by viatrix · Pull Request #120 · ChainSafe/open-creator-rails

viatrix · 2026-04-29T13:13:38Z

Implements #31

rob1997

Good work! Clean PR I like it but needs some changes

rob1997 · 2026-04-29T21:10:15Z

+    }
+
+    function getSubscriptionDuration(uint256 value) external view returns (uint256) {
+        return (value / subscriptionPrice) * SUBSCRIPTION_DURATION;


This method should enforce value to be a multiple of subscriptionPrice in which case getSubscriptionPriceAndDuration already covers its use case, so it's not relevant.

rob1997 · 2026-04-29T21:15:47Z

@@ -146,7 +177,7 @@ contract Asset is Ownable, ReentrancyGuard, IAsset {
    }

    function _subscribe(bytes32 subscriber, address payer, uint256 value) internal returns (uint256) {


based on this new implementation, value should be refactored to count instead, so value = count * subscriptionPrice and duration = subscriptionDuration * count, subscribe should be changed from
subscribe(bytes32 subscriber, address payer, address spender, uint256 value, uint256 deadline, uint8 v, bytes32 r, bytes32 s)
to
subscribe(bytes32 subscriber, address payer, address spender, uint256 count, uint256 deadline, uint8 v, bytes32 r, bytes32 s)
signature is the same but value and duration derivation changes

rob1997 · 2026-04-29T21:23:25Z

        bool isRegistry,
        uint256 timestamp
-    ) internal view returns (uint256 claimable, uint256 claimedNonce) {
+    ) internal view returns (uint256 claimable, uint256 claimedNonce, uint256 newClaimedAtTimestamp) {


The claim function should not change, the Asset/Registry Owner should still be able to claim all the amount due. If this is a fix for a flaw/bug lmk but if this is just an optimization please create another PR for it

It was added to avoid losing dust during claiming. If we allow claiming for partial periods then some money will be lost in this computation and stuck in the contract (for example, the price for the 2-week period is 5, we claim for 1 week, half of that, round to 2, then claim for the other half, round to 2 again, and 1 is lost). One of the solutions is to claim only for whole passed periods, that's why the time is clamped to the end of the period and timestamps are stored. Let me know if we should choose another solution

Right yeah, Good catch!

One of the solutions is to claim only for whole passed periods

Yeah this is a good rule, lets go with this!

rob1997 · 2026-04-29T21:36:08Z

+                // Active subscription: mark cancelled immediately; truncate endTime to end of last
+                // started period so _claimable charges the full in-progress period as fee
+                subscriptions[id].cancelled = true;
+                subscriptions[id].endTime = subscription.endTime - (refundablePeriods * SUBSCRIPTION_DURATION);


while emitting SubscriptionCancelled perhaps we should also add the new endTime of the subscription as an input

rob1997 · 2026-04-29T21:57:39Z


-            uint256 returnable = 0;
+            // Effective start for refund: use startTime for future subscriptions, timestamp for active subscriptions
+            uint256 effectiveStart = subscription.startTime >= timestamp ? subscription.startTime : timestamp;


if (subscription.startTime >= timestamp) subscription hasn't started yet, considering that subscription duration must be a multiple of subscriptionDuration refundable amount would be equal to ((endTime - startTime) / subscriptionDuration) * subscriptionPrice, we can avoid some extra computation.

rob1997 · 2026-04-29T21:58:43Z

+            // and payer are the same (and it was not cancelled).
            if (
-                startTime == subscription.endTime && subscription.payer == payer
+                !subscription.cancelled && startTime == subscription.endTime && subscription.payer == payer


We should allow subscription extensions even after a subscription has been cancelled

Since endTime - startTime is still a multiple of subscriptionDuration even after cancellation it shouldn't be an issue

In the current implementation endTime can be in the future for the cancelled subscription. What should the behavior be? I suppose we should create a new subscription, not extend the cancelled one

We should still be able to extend it, there's no need to add a new index/nonce

rob1997 · 2026-04-29T22:10:04Z

        uint256 subscriptionPrice;
        uint256 registryFeeShare;
        address payer;
+        bool cancelled;


Is this additional field cancelled to avoid extra computation if a subscriber tries to cancel again? Which isn't consequential because it's unlikely and the computation is also negligible since we start the loop from the latest subscription and the condition check should be simple enough
if ((endTime - timestamp) / subscriptionDuration == 0) break;

The cancelled field was added to allow claiming fees for the future part of the current period. If we just set endTime for the subscription to the current timestamp then the claiming should be modified as well because it would lose the future part of the fee. In the current version, the endTime is clamped to the end of the current period to allow full fee collection, but it would mean that the subscription stays active without introducing the cancelled flag. We can discuss other options

I'm not suggesting we set endTime to current timestamp, it should be set to the end of the current period yes but claiming in its current implementation can only claim any amount till the current timestamp. Consider the following

duration = 3 startTime = 2 endTime = 14

This means subscribed for 4 periods, if subscriber cancels at timestamp = 6 so now endTime can now be 8. 8 - 14 (2 periods) is refundable. Asset/Registry owners can still claim for duration 2 - 8 until timestamp == 8

yes, that's how claiming will work now: only for fully passed periods. If we set endTime to the end of the current period then the subscription will be still considered active without the cancelled flag

Add fixed subscription duration

e33193c

viatrix requested a review from rob1997 April 29, 2026 13:13

viatrix added 2 commits April 29, 2026 23:37

Merge branch 'main' into feat/fixed-subscription-price

e374375

Update formatting

18b6b8d

rob1997 requested changes Apr 29, 2026

View reviewed changes

viatrix added 5 commits April 30, 2026 14:02

Updates after review

6b035f8

Merge branch 'main' into feat/fixed-subscription-price

84a288f

Remove cancelled flag

d50fca1

Add endTime to events

85ffc43

Update tests

31f3603

viatrix requested a review from rob1997 May 5, 2026 20:06

		@@ -146,7 +177,7 @@ contract Asset is Ownable, ReentrancyGuard, IAsset {
		}

		function _subscribe(bytes32 subscriber, address payer, uint256 value) internal returns (uint256) {

Conversation

viatrix commented Apr 29, 2026

Uh oh!

rob1997 left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

rob1997 Apr 30, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

rob1997 Apr 30, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

rob1997 Apr 30, 2026 •

edited

Loading

rob1997 Apr 30, 2026 •

edited

Loading