Shared subscription#476
Conversation
There was a problem hiding this comment.
Pull request overview
This PR introduces a new “shared subscription” flow by adding a SharedSubscription fee model and a corresponding SharedAllowList authorizer, along with Solidity mocks and pytest coverage to validate subscription payments, slot accounting, and authorization gating.
Changes:
- Added
SharedSubscription(IFeeModel) contract implementing package-based subscription payments and per-authAdmin slot tracking. - Added
SharedAllowListcontract that ties authorization checks toSharedSubscription(per admin, per ritual). - Added dedicated Solidity mock contracts and pytest suites covering payment/renewal, withdrawals, ritual lifecycle gating, and allowlist authorization behavior.
Reviewed changes
Copilot reviewed 6 out of 6 changed files in this pull request and generated 6 comments.
Show a summary per file
| File | Description |
|---|---|
contracts/contracts/coordination/subscription/SharedSubscription.sol |
New shared subscription fee model contract (package-based fees + authAdmin slot tracking). |
contracts/contracts/coordination/SharedAllowList.sol |
New allowlist implementation integrating with SharedSubscription checks. |
contracts/test/SharedSubscriptionTestSet.sol |
Coordinator mock used to drive SharedSubscription behavior in tests. |
contracts/test/SharedAllowListTestSet.sol |
Mock subscription/coordinator used to unit-test SharedAllowList. |
tests/test_shared_subscription.py |
End-to-end tests for shared subscription payment, renewal/discount, withdrawal, and ritual hooks. |
tests/test_shared_allow_list.py |
Unit tests for shared allowlist authorization/deauthorization and fee-model gating. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| // prevent reusing same address | ||
| if (value) { | ||
| require( | ||
| authAdmins[lookupKey] == address(0), | ||
| "Address authorized by different admin" | ||
| ); | ||
| authAdmins[lookupKey] = msg.sender; | ||
| } else { | ||
| require( | ||
| authAdmins[lookupKey] == msg.sender, |
There was a problem hiding this comment.
The revert reason "Address authorized by different admin" is used even when the address was previously authorized by the same admin (or when deauthorizing an address that was never authorized). Consider using separate checks/messages for (a) already authorized, (b) authorized by someone else, and (c) not authorized, so callers can diagnose failures correctly.
| // prevent reusing same address | |
| if (value) { | |
| require( | |
| authAdmins[lookupKey] == address(0), | |
| "Address authorized by different admin" | |
| ); | |
| authAdmins[lookupKey] = msg.sender; | |
| } else { | |
| require( | |
| authAdmins[lookupKey] == msg.sender, | |
| address currentAuthAdmin = authAdmins[lookupKey]; | |
| // prevent reusing same address | |
| if (value) { | |
| require(currentAuthAdmin != msg.sender, "Address already authorized"); | |
| require( | |
| currentAuthAdmin == address(0), | |
| "Address authorized by different admin" | |
| ); | |
| authAdmins[lookupKey] = msg.sender; | |
| } else { | |
| require(currentAuthAdmin != address(0), "Address not authorized"); | |
| require( | |
| currentAuthAdmin == msg.sender, |
| uint256[3][10] public feePackages; | ||
| uint32 public activeRitualId; | ||
| mapping(address authAdmin => Billing billingInfo) public billing; | ||
| address public adopter; |
There was a problem hiding this comment.
Is this an address effectively controlled by NuCo to manage the global ritual? Is the use of "adopter" a generalization of that?
| assert subscription.feePackages(0, 1) == 15 | ||
| assert subscription.feePackages(1, 1) == 500 |
There was a problem hiding this comment.
Can you do the same here as you did below for fee rates?
| assert subscription.feePackages(0, 1) == 15 | |
| assert subscription.feePackages(1, 1) == 500 | |
| assert subscription.feePackages(0, 1) == FEE_PACKAGES[0][1] | |
| assert subscription.feePackages(1, 1) == FEE_PACKAGES[1][1] |
| subscription.getEncryptorFeeRate(15, 90 * ONE_DAY) | ||
|
|
||
| fees = subscription.encryptorFees(FEE_PACKAGES[0][2], 15, 14 * ONE_DAY) | ||
| assert round(fees / 10**18, 0) == 150 # 150 USDC |
There was a problem hiding this comment.
Just of note: Doesn't matter for the test since it uses its own ERC20, but the comment mentions USDC which only uses 6 decimal places and not 18.
Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com> Co-authored-by: Derek Pierre <derek.pierre@gmail.com>
Type of PR:
Required reviews:
What this does:
Issues fixed/closed:
Why it's needed:
Notes for reviewers: