BqETH subscription (no encryptor fees yet) #272

vzotova · 2024-05-28T17:27:04Z

Type of PR:

Bugfix
Feature
Documentation
Other

Required reviews:

1
2
3

What this does:

High-level idea of the changes introduced in this PR.
List relevant API changes (if any), as well as related PRs and issues.

Issues fixed/closed:
Refs nucypher/tdec#169
Refs #262

Why it's needed:

Explain how this PR fits in the greater context of the NuCypher Network.
E.g., if this PR address a nucypher/productdev issue, let reviewers know!

Notes for reviewers:
Based on #271

derekpierre

Looks great - just some suggestions/comments.

contracts/contracts/coordination/BqETHSubscription.sol

Co-authored-by: Derek Pierre <[email protected]>

arjunhassard

Base fee logic looks good to me!

arjunhassard · 2024-06-10T16:23:46Z

contracts/contracts/coordination/BqETHSubscription.sol

+     * @notice Pays for a subscription
+     */
+    function payForSubscription() external {
+        // require(endOfSubscription == 0, "Subscription already payed");


Suggested change

// require(endOfSubscription == 0, "Subscription already payed");

// require(endOfSubscription == 0, "Subscription already paid");

arjunhassard · 2024-06-10T16:31:21Z

contracts/contracts/coordination/BqETHSubscription.sol

+    uint32 public constant INACTIVE_RITUAL_ID = type(uint32).max;
+
+    // TODO: DAO Treasury
+    // TODO: Should it be updatable?


We're still in an experimentation phase for fee models, even within the context of a single adopter. So we need a way in the near-term, without involving the DAO Treasury, to make structural changes to individual subscription contracts, the grace periods, and corresponding powers granted to / removed from cohort admins

in that case contract could be upgradable and belong not to dao but some multisig
other similar options is create setters/getters for all parameters that can be changed by multisig

For reference, #264 proposed that these contracts are upgradeable initially. Once the adopter and us are confortable, we can always bork upgradeability by transferring privileges to the zero address. I see that you added upgradeability in #277, so just commenting for completeness.

arjunhassard · 2024-06-10T16:41:37Z

contracts/contracts/coordination/ManagedAllowList.sol

@@ -39,9 +39,9 @@ contract ManagedAllowList is GlobalAllowList {
     */
    constructor(
        Coordinator _coordinator,
+        IFeeModel _feeModel,
        UpfrontSubscriptionWithEncryptorsCap _subscription


Is IFeeModel.sol where the logic for encryptor annual credits will live, or will it be on a per-adopter basis for now?

both statements are correct, IFeeModel implementation can have credits and such contracts will be on a per-adopter basis for now

derekpierre

🎸

cygnusv

First pass

cygnusv · 2024-06-19T10:17:08Z

contracts/contracts/coordination/Coordinator.sol

+
+    function extendRitual(uint32 ritualId, uint32 duration) external {
+        Ritual storage ritual = rituals[ritualId];
+        require(msg.sender == ritual.initiator, "Only initiator can extend ritual");


For the moment this is fine, but in the future we may want to relax this restriction (cc @arjunhassard ). See the last discussion in #93

Didn't we tell BqETH that anyone can extend the ritual?

cygnusv · 2024-06-19T11:00:50Z

contracts/contracts/coordination/FlatRateFeeModel.sol

+        uint32 duration
+    ) external override {
+        processPayment(initiator, ritualId, numberOfProviders, duration);
+    }


If the code is the same than processRitualPayment(), do you think we need a separate function?

yes, FlatRateFeeModel is just one fee model, for subscription is different

cygnusv · 2024-06-19T11:03:21Z

contracts/contracts/coordination/IFeeModel.sol

@@ -9,17 +9,35 @@ import "@openzeppelin/contracts/token/ERC20/IERC20.sol";
 * @notice IFeeModel
 */
 interface IFeeModel {
-    function currency() external view returns (IERC20);


Why removing the currency() interface here? Maybe it's because we're moving towards a credits system? In any case, functions to calculate cost are still defined with a reference currency token, so I'd keep this.

because FeeModel is more flexible, just imagine fee model without payment

cygnusv · 2024-06-19T11:08:15Z

contracts/contracts/coordination/BqETHSubscription.sol

+    uint32 public constant INACTIVE_RITUAL_ID = type(uint32).max;
+
+    // TODO: DAO Treasury
+    // TODO: Should it be updatable?


For reference, #264 proposed that these contracts are upgradeable initially. Once the adopter and us are confortable, we can always bork upgradeability by transferring privileges to the zero address. I see that you added upgradeability in #277, so just commenting for completeness.

cygnusv · 2024-06-19T11:21:32Z

contracts/contracts/coordination/BqETHSubscription.sol

+
+    // TODO: DAO Treasury
+    // TODO: Should it be updatable?
+    address public immutable beneficiary;


I'd rename this to treasury. Beneficiary is a loaded term from the staking contract.

In one of the next PR I've changed it to Ownable contract to be able to transfer this role, and I'll rename rest to treasury in that PR 👍

vzotova self-assigned this May 28, 2024

vzotova force-pushed the subscription-3 branch from d5caee6 to 6cdd758 Compare May 28, 2024 18:37

vzotova changed the title ~~[WIP] BqETH subscription~~ BqETH subscription Jun 5, 2024

vzotova marked this pull request as ready for review June 5, 2024 22:20

vzotova requested review from arjunhassard, cygnusv, theref, derekpierre and manumonti June 5, 2024 22:21

derekpierre reviewed Jun 7, 2024

View reviewed changes

vzotova force-pushed the subscription-3 branch from 314ce06 to ec840c2 Compare June 7, 2024 18:51

vzotova and others added 7 commits June 7, 2024 15:02

Adds fee model <-> allow list communication

64b28e1

Apply suggestions from code review

dfb6bc0

Co-authored-by: Derek Pierre <[email protected]>

Initial commit with BqETHSubscription contract

9620ac3

FeeModel: adds yellow and red period and extending ritual

7cae93d

Check access controller in subscription

538c07a

BqETHSubscription: fix bugs and add tests

7fd7116

BqETHSubscription: handling yellow period

4d94e32

vzotova force-pushed the subscription-3 branch 2 times, most recently from a2b139e to 37033a8 Compare June 7, 2024 19:17

Apply suggestions from code review

4db8af4

Co-authored-by: Derek Pierre <[email protected]>

vzotova force-pushed the subscription-3 branch from 37033a8 to 4db8af4 Compare June 7, 2024 19:21

vzotova mentioned this pull request Jun 7, 2024

Adds fee model <-> allow list communication #271

Closed

7 tasks

arjunhassard reviewed Jun 10, 2024

View reviewed changes

derekpierre approved these changes Jun 10, 2024

View reviewed changes

vzotova mentioned this pull request Jun 10, 2024

Encryptor fees and slots management in subscription #273

Merged

7 tasks

vzotova changed the title ~~BqETH subscription~~ BqETH subscription (no encryptor fees yet) Jun 11, 2024

cygnusv changed the base branch from epic-subscription to main June 19, 2024 10:02

cygnusv changed the base branch from main to epic-subscription June 19, 2024 10:03

cygnusv reviewed Jun 19, 2024

View reviewed changes

cygnusv approved these changes Jun 25, 2024

View reviewed changes

cygnusv merged commit 449290b into nucypher:epic-subscription Jun 25, 2024
2 checks passed

derekpierre mentioned this pull request Aug 12, 2024

[EPIC] New fee model architecture #270

Merged

23 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

BqETH subscription (no encryptor fees yet) #272

BqETH subscription (no encryptor fees yet) #272

vzotova commented May 28, 2024 •

edited

Loading

derekpierre left a comment

arjunhassard left a comment

arjunhassard Jun 10, 2024

arjunhassard Jun 10, 2024

vzotova Jun 10, 2024

cygnusv Jun 19, 2024

arjunhassard Jun 10, 2024

vzotova Jun 10, 2024

derekpierre left a comment

cygnusv left a comment

cygnusv Jun 19, 2024

arjunhassard Jul 1, 2024

cygnusv Jun 19, 2024

vzotova Jun 19, 2024

cygnusv Jun 19, 2024

vzotova Jun 19, 2024

cygnusv Jun 19, 2024

cygnusv Jun 19, 2024

vzotova Jun 19, 2024

	// require(endOfSubscription == 0, "Subscription already payed");
	// require(endOfSubscription == 0, "Subscription already paid");

BqETH subscription (no encryptor fees yet) #272

BqETH subscription (no encryptor fees yet) #272

Conversation

vzotova commented May 28, 2024 • edited Loading

derekpierre left a comment

Choose a reason for hiding this comment

arjunhassard left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

derekpierre left a comment

Choose a reason for hiding this comment

cygnusv left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

vzotova commented May 28, 2024 •

edited

Loading