Improve event signatures and subscriber cancellation flow (#123)

* checkpoint

* new cancel flow

* checkpoint

* fixed some warnings

* Better events

* tests and docs updated

* forge fmt

* modified subscription cancelled and revoked events and added new subscription removed event

* Tests fixed, new tests added and docs updated

* minor rename

Author: rob1997

README.md

@@ -98,7 +98,7 @@ New assets are appended to the `assets` array of the corresponding registry in `
 
 ### Subscribe
 
-Subscribe to an asset using ERC-2612 permit (gasless approval). The payer signs the permit and pays with tokens; the subscription is associated with a **subscriber** identity (a `bytes32` hash). For cancellation, the subscriber identity should be derived as `keccak256(abi.encode(subscriberId, subscriberAddress))`, so only that `subscriberAddress` can cancel using a signed cancellation flow. The payer and the subscriber can be the same or different (e.g. "pay for someone else"). The **payer** is the address entitled to refunds if the subscription is later cancelled or revoked (unearned time is refunded to the payer).
+Subscribe to an asset using ERC-2612 permit (gasless approval). The payer signs the permit and pays with tokens; the subscription is associated with a **subscriber** identity (a `bytes32` hash). For cancellation, the subscriber identity should be derived as `keccak256(abi.encode(subscriberId, subscriberAddress))`, so only that `subscriberAddress` can cancel by signing an off-chain message and calling `cancelSubscription` in one step (no on-chain commit or timestamp binding). The payer and the subscriber can be the same or different (e.g. "pay for someone else"). The **payer** is the address entitled to refunds if the subscription is later cancelled or revoked (unearned time is refunded to the payer).
 
 ```bash
 ./scripts/subscribe.sh <registry_index> <asset_id> <subscriber_id> <value> <payer_private_key>
@@ -269,7 +269,7 @@ All external functions for the registry and asset contracts, for use with JSON-R
 
 ---
 
-**subscribe** : Subscribes a subscriber to the asset using ERC-2612 permit; forwards to the asset contract. The permit is signed by the payer; the subscription is attributed to `_subscriber` (payer and subscriber can differ). The payer is the refund beneficiary on cancel/revoke. For cancellation-compatible identity, `_subscriber` should be `keccak256(abi.encode(subscriberId, subscriberAddress))`, where `subscriberAddress` is the address that will commit/sign cancellation.
+**subscribe** : Subscribes a subscriber to the asset using ERC-2612 permit; forwards to the asset contract. The permit is signed by the payer; the subscription is attributed to `_subscriber` (payer and subscriber can differ). The payer is the refund beneficiary on cancel/revoke. For cancellation-compatible identity, `_subscriber` should be `keccak256(abi.encode(subscriberId, subscriberAddress))`, where `subscriberAddress` is the address that will call `cancelSubscription` and sign the cancellation payload.
 - Type: write
 - Permission: none
 - Parameters:
@@ -474,7 +474,7 @@ All external functions for the registry and asset contracts, for use with JSON-R
 
 ---
 
-**subscribe** : Subscribes a subscriber using ERC-2612 permit: payer signs permit, then payment is pulled and subscription is attributed to the given subscriber. Payer and subscriber can differ (e.g. pay for someone else). The payer is the refund beneficiary on cancel/revoke. For cancellation-compatible identity, `subscriber` should be `keccak256(abi.encode(subscriberId, subscriberAddress))`, where `subscriberAddress` is the address that will commit/sign cancellation.
+**subscribe** : Subscribes a subscriber using ERC-2612 permit: payer signs permit, then payment is pulled and subscription is attributed to the given subscriber. Payer and subscriber can differ (e.g. pay for someone else). The payer is the refund beneficiary on cancel/revoke. For cancellation-compatible identity, `subscriber` should be `keccak256(abi.encode(subscriberId, subscriberAddress))`, where `subscriberAddress` is the address that will call `cancelSubscription` and sign the cancellation payload.
 - Type: write
 - Permission: none
 - Parameters:
@@ -546,23 +546,12 @@ All external functions for the registry and asset contracts, for use with JSON-R
 
 ---
 
-**commitCancellation** : Commits a cancellation intent for the caller's `(subscriberId, msg.sender)` pair.
+**cancelSubscription** : Cancels the caller's subscription after validating an EIP-191 signature from `msg.sender`. Unearned subscription value is refunded to each original payer. There is no separate on-chain commit step: the subscriber signs an off-chain message, then submits one transaction with that signature.
 - Type: write
-- Permission: caller is the subscriber address committing cancellation
+- Permission: `msg.sender` must be the subscriber address represented in `keccak256(abi.encode(subscriberId, msg.sender))` (the recovered signer must equal `msg.sender`).
 - Parameters:
   - `string subscriberId` : Human-readable subscriber id used in the subscriber hash.
-- Returns:
-  - `uint256` : Commitment timestamp used in the subsequent cancellation signature.
-
----
-
-**cancelSubscription** : Cancels the caller's subscription after validating a prior commitment and signature. Unearned subscription value is refunded to each original payer.
-- Type: write
-- Permission: caller must be the subscriber address represented in `keccak256(abi.encode(subscriberId, msg.sender))`
-- Parameters:
-  - `string subscriberId` : Human-readable subscriber id used in the subscriber hash.
-  - `uint256 timestamp` : Commitment timestamp returned by `commitCancellation`.
-  - `bytes signature` : ECDSA signature by `msg.sender` over `keccak256(abi.encodePacked(chainid, assetAddress, timestamp, subscriberHash))`.
+  - `bytes signature` : ECDSA signature by `msg.sender` over the Ethereum signed message hash of `keccak256(abi.encodePacked(chainid, assetAddress, subscriber))`, where `subscriber` is `keccak256(abi.encode(subscriberId, msg.sender))` and `assetAddress` is this asset contract.
 - Returns: void
 
 ---
@@ -618,13 +607,26 @@ All events emitted by the registry and asset contracts. Use for indexing, loggin
 
 ---
 
-**SubscriptionAdded** : Emitted when a new subscription record is created for a subscriber (new nonce). This happens on the first subscription and whenever the payer, subscription price, or registry fee share differs from the active subscription. For renewals that extend an existing active subscription under the same terms, see `SubscriptionExtended`.
+**SubscriptionAdded** : Emitted when the first subscription record is created for a subscriber.
 - Contract: `Asset`
 - Parameters:
   - `bytes32 indexed subscriber` : Subscriber identity (hash).
   - `uint256 indexed startTime` : Subscription start time (Unix timestamp).
   - `uint256 indexed endTime` : Subscription expiry time (Unix timestamp).
-  - `uint256 nonce` : Subscription nonce (increments each time a new record is created for the subscriber).
+  - `address payer` : Payer for this subscription (refund beneficiary on cancel/revoke).
+  - `uint256 subscriptionPrice` : Per-second subscription price snapshot used for this subscription record.
+  - `uint256 registryFeeShare` : Registry fee share snapshot (0-100) used for this subscription record.
+
+
+---
+
+**SubscriptionRenewed** : Emitted when a subscriber already has prior subscription history and a new subscription record is created (new nonce). This occurs when in-place extension is not possible (e.g. prior subscription expired, or payer/price/registry fee share differs from the current active record).
+- Contract: `Asset`
+- Parameters:
+  - `bytes32 indexed subscriber` : Subscriber identity (hash).
+  - `uint256 indexed startTime` : New subscription record start time (Unix timestamp).
+  - `uint256 indexed endTime` : New subscription record expiry time (Unix timestamp).
+  - `uint256 nonce` : New subscription nonce for this subscriber.
   - `address payer` : Payer for this subscription (refund beneficiary on cancel/revoke).
   - `uint256 subscriptionPrice` : Per-second subscription price snapshot used for this subscription record.
   - `uint256 registryFeeShare` : Registry fee share snapshot (0-100) used for this subscription record.
@@ -671,11 +673,23 @@ All events emitted by the registry and asset contracts. Use for indexing, loggin
 - Contract: `Asset`
 - Parameters:
   - `bytes32 indexed subscriber` : Subscriber whose subscription was revoked.
+  - `uint256 indexed nonce` : Active nonce after revocation/removal processing.
+  - `uint256 indexed endTime` : Effective end time after revocation. Will be `0` when the subscriber is fully removed.
 
 
 ---
 
 **SubscriptionCancelled** : Emitted when a subscriber's subscription is cancelled through the subscriber-signed cancellation flow.
 - Contract: `Asset`
 - Parameters:
-  - `bytes32 indexed subscriber` : Subscriber hash `keccak256(abi.encode(subscriberId, subscriberAddress))` whose subscription was cancelled.
+  - `bytes32 indexed subscriber` : Subscriber hash `keccak256(abi.encode(subscriberId, subscriberAddress))` whose subscription was cancelled.
+  - `uint256 indexed nonce` : Active nonce after cancellation/removal processing.
+  - `uint256 indexed endTime` : Effective end time after cancellation. Will be `0` when the subscriber is fully removed.
+
+
+---
+
+**SubscriptionRemoved** : Emitted when all remaining subscription records for a subscriber are deleted and the subscriber is removed from tracking state.
+- Contract: `Asset`
+- Parameters:
+  - `bytes32 indexed subscriber` : Subscriber identity (hash) that was fully removed.

src/Asset.sol

@@ -31,7 +31,6 @@ contract Asset is Ownable, ReentrancyGuard, IAsset {
 
     mapping(bytes32 => Subscription) internal subscriptions;
     mapping(bytes32 => uint256) internal nonces;
-    mapping(bytes32 => uint256) internal cancellations;
 
     mapping(bytes32 => uint256) internal creatorClaimedAtTimestamps;
     mapping(bytes32 => uint256) internal creatorClaimedAtNonces;
@@ -58,11 +57,19 @@ contract Asset is Ownable, ReentrancyGuard, IAsset {
     error SubscriptionNotFound();
     error SubscriptionRevocationFailed();
     error SubscriptionCancellationFailed();
-    error InvalidCancellationCommitment();
     error InvalidSignature();
     error OnlyRegistryUnauthorizedAccount();
 
     event SubscriptionAdded(
+        bytes32 indexed subscriber,
+        uint256 indexed startTime,
+        uint256 indexed endTime,
+        address payer,
+        uint256 subscriptionPrice,
+        uint256 registryFeeShare
+    );
+
+    event SubscriptionRenewed(
         bytes32 indexed subscriber,
         uint256 indexed startTime,
         uint256 indexed endTime,
@@ -71,12 +78,14 @@ contract Asset is Ownable, ReentrancyGuard, IAsset {
         uint256 subscriptionPrice,
         uint256 registryFeeShare
     );
+
     event SubscriptionExtended(bytes32 indexed subscriber, uint256 indexed endTime);
     event CreatorFeeClaimed(bytes32 indexed subscriber, uint256 amount);
     event CreatorFeeClaimedBatch(bytes32[] indexed subscribers, uint256 totalAmount);
     event SubscriptionPriceUpdated(uint256 newSubscriptionPrice);
-    event SubscriptionRevoked(bytes32 indexed subscriber);
-    event SubscriptionCancelled(bytes32 indexed subscriber);
+    event SubscriptionRevoked(bytes32 indexed subscriber, uint256 indexed nonce, uint256 indexed endTime);
+    event SubscriptionCancelled(bytes32 indexed subscriber, uint256 indexed nonce, uint256 indexed endTime);
+    event SubscriptionRemoved(bytes32 indexed subscriber);
 
     /// @notice Initializes the asset with id, price, payment token, and owner.
     ///         Callable only by the registry (msg.sender).
@@ -152,7 +161,7 @@ contract Asset is Ownable, ReentrancyGuard, IAsset {
         return _subscribe(subscriber, payer, value);
     }
 
-    function _subscribe(bytes32 subscriber, address payer, uint256 value) internal returns (uint256) {
+    function _subscribe(bytes32 subscriber, address payer, uint256 value) internal returns (uint256 endTime) {
         uint256 duration = value / subscriptionPrice;
 
         uint256 startTime = block.timestamp;
@@ -177,7 +186,7 @@ contract Asset is Ownable, ReentrancyGuard, IAsset {
                     && subscription.subscriptionPrice == subscriptionPrice
                     && subscription.registryFeeShare == registryFeeShare
             ) {
-                uint256 endTime = subscription.endTime + duration;
+                endTime = subscription.endTime + duration;
 
                 subscriptions[id].endTime = endTime;
 
@@ -189,14 +198,23 @@ contract Asset is Ownable, ReentrancyGuard, IAsset {
             nonce = ++nonces[subscriber];
 
             id = _hash(subscriber, nonce);
+
+            endTime = _addSubscription(id, subscriber, startTime, duration, registryFeeShare, payer);
+
+            emit SubscriptionRenewed(subscriber, startTime, endTime, nonce, payer, subscriptionPrice, registryFeeShare);
+
+            return endTime;
         }
 
-        return _addSubscription(id, nonce, subscriber, startTime, duration, registryFeeShare, payer);
+        endTime = _addSubscription(id, subscriber, startTime, duration, registryFeeShare, payer);
+
+        emit SubscriptionAdded(subscriber, startTime, endTime, payer, subscriptionPrice, registryFeeShare);
+
+        return endTime;
     }
 
     function _addSubscription(
         bytes32 id,
-        uint256 nonce,
         bytes32 subscriber,
         uint256 startTime,
         uint256 duration,
@@ -215,8 +233,6 @@ contract Asset is Ownable, ReentrancyGuard, IAsset {
 
         subscribers.add(subscriber);
 
-        emit SubscriptionAdded(subscriber, startTime, endTime, nonce, payer, subscriptionPrice, registryFeeShare);
-
         return endTime;
     }
 
@@ -485,6 +501,8 @@ contract Asset is Ownable, ReentrancyGuard, IAsset {
             delete registryClaimedAtNonces[subscriber];
             delete registryClaimedAtTimestamps[subscriber];
             subscribers.remove(subscriber);
+
+            emit SubscriptionRemoved(subscriber);
         }
         // If the user has subscriptions left, decrement the nonce by the number of deleted subscriptions
         else if (deleted != 0) {
@@ -495,28 +513,15 @@ contract Asset is Ownable, ReentrancyGuard, IAsset {
     function revokeSubscription(bytes32 subscriber) external onlyOwner nonReentrant {
         _removeSubscription(subscriber);
 
-        emit SubscriptionRevoked(subscriber);
-    }
-
-    function commitCancellation(string memory subscriberId) external returns (uint256 timestamp) {
-        timestamp = block.timestamp;
-
-        cancellations[keccak256(abi.encode(subscriberId, msg.sender))] = timestamp;
-
-        return timestamp;
+        uint256 nonce = nonces[subscriber];
+        bytes32 id = _hash(subscriber, nonce);
+        emit SubscriptionRevoked(subscriber, nonce, subscriptions[id].endTime);
     }
 
-    function cancelSubscription(string memory subscriberId, uint256 timestamp, bytes memory signature)
-        external
-        nonReentrant
-    {
-        bytes32 subscriber = keccak256(abi.encode(subscriberId, msg.sender));
-
-        if (cancellations[subscriber] != timestamp) {
-            revert InvalidCancellationCommitment();
-        }
+    function cancelSubscription(string memory subscriberId, bytes memory signature) external nonReentrant {
+        bytes32 subscriber = _hash(subscriberId, msg.sender);
 
-        bytes32 hash = keccak256(abi.encodePacked(block.chainid, address(this), timestamp, subscriber));
+        bytes32 hash = _hash(block.chainid, address(this), subscriber);
 
         address signer = ECDSA.recover(MessageHashUtils.toEthSignedMessageHash(hash), signature);
 
@@ -526,16 +531,27 @@ contract Asset is Ownable, ReentrancyGuard, IAsset {
 
         _removeSubscription(subscriber);
 
-        delete cancellations[subscriber];
-
-        emit SubscriptionCancelled(subscriber);
+        // If the user has subscriptions left, emit the SubscriptionCancelled event
+        uint256 nonce = nonces[subscriber];
+        bytes32 id = _hash(subscriber, nonce);
+        emit SubscriptionCancelled(subscriber, nonce, subscriptions[id].endTime);
     }
 
     function _hash(bytes32 a, uint256 b) internal pure returns (bytes32 result) {
         result = keccak256(abi.encode(a, b));
         return result;
     }
 
+    function _hash(string memory a, address b) internal pure returns (bytes32 result) {
+        result = keccak256(abi.encode(a, b));
+        return result;
+    }
+
+    function _hash(uint256 a, address b, bytes32 c) internal pure returns (bytes32 result) {
+        result = keccak256(abi.encodePacked(a, b, c));
+        return result;
+    }
+
     function _isOwner() internal view returns (bool) {
         return msg.sender == owner();
     }

src/IAsset.sol

@@ -85,14 +85,8 @@ interface IAsset {
     /// @param subscriber Subscriber hash whose subscription to revoke.
     function revokeSubscription(bytes32 subscriber) external;
 
-    /// @notice Commits cancellation intent for subscriber hash derived from `(subscriberId, msg.sender)`.
-    /// @param subscriberId Human-readable subscriber ID used in `keccak256(abi.encode(subscriberId, msg.sender))`.
-    /// @return timestamp The timestamp of the cancellation commitment.
-    function commitCancellation(string memory subscriberId) external returns (uint256 timestamp);
-
     /// @notice Cancels your subscription for subscriber hash derived from `(subscriberId, msg.sender)`.
     /// @param subscriberId Human-readable subscriber ID used in `keccak256(abi.encode(subscriberId, msg.sender))`.
-    /// @param timestamp The timestamp of the cancellation commitment.
     /// @param signature Signature by msg.sender over the cancellation payload.
-    function cancelSubscription(string memory subscriberId, uint256 timestamp, bytes memory signature) external;
+    function cancelSubscription(string memory subscriberId, bytes memory signature) external;
 }