`ManagedAllowList` #255

piotr-roslaniec · 2024-04-15T10:26:46Z

Type of PR:

Feature

Required reviews:

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:

Fixes #...

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:

Rebased over [WIP] Upfront payment contracts #253

wip: change of plans was a mistakerino

contracts/contracts/coordination/SubscriptionManager.sol

piotr-roslaniec · 2024-04-22T10:20:58Z

Hi @cygnusv, @vzotova - I asked for a review of this draft to make sure I'm on the right path with everything. Please let me know what you think.

vzotova

couple high level and style questions:

contracts/contracts/coordination/GlobalAllowList.sol

contracts/contracts/coordination/ManagedAllowList.sol

vzotova · 2024-04-22T22:57:41Z

contracts/contracts/coordination/ManagedAllowList.sol

+            // If we want to authorize an address, we need to check if the authorization cap has been exceeded
+            // If we want to deauthorize an address, we don't need to check the authorization cap
+            require(
+                !value ||


can we extract check of value before loop?

Yes. But I'm not sure about the business logic here. Should deauthorizations count towards billable authActions? I'm going to assume yes for the time being and remove the require on value. CC: @cygnusv

contracts/contracts/coordination/ManagedAllowList.sol

contracts/contracts/coordination/Subscription.sol

contracts/contracts/coordination/ManagedAllowList.sol

contracts/contracts/lib/LookupKey.sol

contracts/contracts/coordination/Subscription.sol

piotr-roslaniec · 2024-04-24T13:04:03Z

Requesting review from @arjunhassard and @cygnusv to make sure this matches business logic as outlined in #257

cygnusv

Thanks for this @piotr-roslaniec! I think this is a great starting point also for the Subscription stuff laid out in #264

derekpierre · 2024-05-15T17:32:17Z

contracts/contracts/coordination/Subscription.sol

+    }
+
+    /**
+     * @notice Calculates the available amount of fees that can be withdrawn be the beneficiary


Suggested change

* @notice Calculates the available amount of fees that can be withdrawn be the beneficiary

* @notice Calculates the available amount of fees that can be withdrawn by the beneficiary

derekpierre · 2024-05-15T17:44:03Z

contracts/contracts/lib/LookupKey.sol

+     * @param encryptor The address of the encryptor
+     * @return The key used to lookup authorizations
+     */
+    function lookupKey(uint32 ritualId, address encryptor) internal pure returns (bytes32) {


I don't think encryptor applies here anymore since this method is also used for administrator addresses, spender addresses etc. Should we use a more generic name eg. anAddress or associatedAddress or ...

It's true it's not strictly accurate here, but the hierarchy of roles/names is getting quite unwieldy. Maybe we can rename encryptor --> encryptAdmin and then rename this admin or anAdmin.

Since this is a library code and is re-used in a couple of places, perhaps we should rename it to just _address. encryptor and admin are terms from specific contexts.

derekpierre · 2024-05-15T17:45:57Z

scripts/deploy_managed_allow_list.py

+    mnaged_allow_list = deployer.deploy(project.ManagedAllowList)
+    deployments = [mnaged_allow_list]


Suggested change

mnaged_allow_list = deployer.deploy(project.ManagedAllowList)

deployments = [mnaged_allow_list]

managed_allow_list = deployer.deploy(project.ManagedAllowList)

deployments = [managed_allow_list]

derekpierre · 2024-05-15T18:40:48Z

contracts/contracts/coordination/ManagedAllowList.sol

+        for (uint256 i = 0; i < addresses.length; i++) {
+            require(
+                authActions[ritualId] <
+                    subscription.authorizationActionsCap(ritualId, addresses[i]),
+                "Authorization cap exceeded"


I may be misunderstanding, but this logic is a little unclear to me:

Should we only be checking caps if value is True i.e. we are authorizing? If deauthorizing (i.e. value is False), there are no caps.

I'm unclear what we are checking in the require statement. canSetAuthorizations already checks whether the sender is a valid admin for the ritual - great, but I assumed here you need to check 2 things (not in a loop):

the remaining allowance for the admin making the authorization request (i.e. sender) allows for the set of encryptor addresses to be authorized, so:

require(getAllowance(ritualId, msg.sender) >= addresses.length, "Administrator cap exceeded")

That the total cap across all administrators for a specific ritual is not being exceeded, so:

require(authActions[ritualId] >= addresses.length, "Ritual-wide cap exceeded")

My calls may be wrong above ^, but the pseudocode I'm looking for is:
i. remaining allowance for administrator for ritual >= the number of encryptor addresses to authorize
ii. remaining allowed actions for ritual (based on cap) >= the number of encryptor addresses to authorize

the remaining allowance for the admin making the authorization request (i.e. sender) allows for the set of encryptor addresses to be authorized, so:

This maybe needs to call a partner-defined contract, so they can write their own logic for limits placed on authAdmins?

Just to give more context, I wasn't sure how to use value here: Should setting authorization to false (so, removing authorization) count towards billable actions?

We may treat adding and removing authorization differently, but I think it's application-specific (the frequency of actions, but also the application-specific meaning of authorizing/deauthorizing)

derekpierre · 2024-05-15T18:42:29Z

contracts/contracts/coordination/GlobalAllowList.sol

            emit AddressAuthorizationSet(ritualId, addresses[i], value);
        }
+
+        authActions[ritualId] += addresses.length;


Is incrementing only applicable if value is True? If value is False should this be decrementing instead?

For this version, the goal was to charge per action, regardless if it's authorizing or deauthorizing. But more recent variations proposed to some adopters will use something different (e.g. charging for authorized slots). I think we can leave it as is because it's only a starting point anyway, and the more recent design of subscription contracts will refactor everything

👍 I see, so every action (authorize or deauthorize) is a charge.

I'm confused about the relationship between value and charging for authorizations/deauthorizations.

Doesn't * @param value The authorization status imply that this is just a boolean check as to whether the ritualID is active? (Separately, what is the evidence that confirms this?)

As @cygnusv mentions, we're not charging for authorizations/deauthorizations, these are unlimited and can be executed at will – and the cohortAdmin/sponsor pays the gas, possibly passing that cost to an authAdmin (btw, do we have an estimate for how much this will be per action? how much is saved by batching?)

I guess #253 will cover the rules of adding/removing encryptor credits (or 'slots') and we can leave the option of charging for authorizations/deauthorizations in some later version.

contracts/contracts/coordination/ManagedAllowList.sol

arjunhassard

Excellent work, think you can claim to be at least somewhat-versed in Solidity now.

Still quite a few things to clarify and decide, particularly with respect to #253-driven refactoring. I suggest we do a PR walkthrough, maybe on Friday?

arjunhassard · 2024-05-15T20:42:58Z

contracts/contracts/coordination/GlobalAllowList.sol

-        require(coordinator.isRitualActive(ritualId), "Only active rituals can add authorizations");
+        require(coordinator.isRitualActive(ritualId), "Only active rituals can set authorizations");
+
+        require(addresses.length <= MAX_AUTH_ACTIONS, "Too many addresses");


Is this a maximum per batch of authorizations?

Yes, per one setAuthorizations call

arjunhassard · 2024-05-15T20:52:42Z

contracts/contracts/coordination/GlobalAllowList.sol

            emit AddressAuthorizationSet(ritualId, addresses[i], value);
        }
+
+        authActions[ritualId] += addresses.length;


I'm confused about the relationship between value and charging for authorizations/deauthorizations.

Doesn't * @param value The authorization status imply that this is just a boolean check as to whether the ritualID is active? (Separately, what is the evidence that confirms this?)

As @cygnusv mentions, we're not charging for authorizations/deauthorizations, these are unlimited and can be executed at will – and the cohortAdmin/sponsor pays the gas, possibly passing that cost to an authAdmin (btw, do we have an estimate for how much this will be per action? how much is saved by batching?)

I guess #253 will cover the rules of adding/removing encryptor credits (or 'slots') and we can leave the option of charging for authorizations/deauthorizations in some later version.

arjunhassard · 2024-05-15T20:55:52Z

contracts/contracts/coordination/ManagedAllowList.sol

+        for (uint256 i = 0; i < addresses.length; i++) {
+            require(
+                authActions[ritualId] <
+                    subscription.authorizationActionsCap(ritualId, addresses[i]),
+                "Authorization cap exceeded"


the remaining allowance for the admin making the authorization request (i.e. sender) allows for the set of encryptor addresses to be authorized, so:

This maybe needs to call a partner-defined contract, so they can write their own logic for limits placed on authAdmins?

arjunhassard · 2024-05-15T21:02:45Z

contracts/contracts/lib/LookupKey.sol

+     * @param encryptor The address of the encryptor
+     * @return The key used to lookup authorizations
+     */
+    function lookupKey(uint32 ritualId, address encryptor) internal pure returns (bytes32) {


It's true it's not strictly accurate here, but the hierarchy of roles/names is getting quite unwieldy. Maybe we can rename encryptor --> encryptAdmin and then rename this admin or anAdmin.

cygnusv · 2024-05-16T11:41:57Z

@arjunhassard @derekpierre The work @vzotova is doing on #264 will be based directly on this (as you can see in #265, Vicky is already working on top of this PR). The plan we laid out is that this is good foundational work, but that will require a lot of changes in light of the recent design for Subscriptions and the newer requirements for the AllowList contract. I propose we merge this now, either as-is or with minor changes (like renames).

piotr-roslaniec force-pushed the managed-allow-list branch 4 times, most recently from 75a4eb9 to 639e38a Compare April 15, 2024 18:55

cygnusv and others added 7 commits April 15, 2024 21:35

Add simple hooks to GlobalAllowList to allow simple extensibility

5526814

wip: managed allow list

8693901

wip: add some tests

0205594

wip: change of plans

f9ba98d

wip: change of plans was a mistakerino

Generalize modifier to set authorizations in GlobalAllowList

8130996

ManagedAllowList now inherits from GlobalAllowList

6eca6cc

Rename allowances in ManagedAllowList

720e83c

piotr-roslaniec force-pushed the managed-allow-list branch from 639e38a to 91f5f21 Compare April 16, 2024 09:37

cygnusv added 2 commits April 16, 2024 11:37

Fix tests for ManagedAllowList

9e42114

Draft for Subscriptions and how can be used with AllowLists

ee85f5e

arjunhassard mentioned this pull request Apr 17, 2024

Extend allow logic hierarchy & subscription manager to include 'authorization administrator' role (Managed Allow List) #257

Open

vzotova reviewed Apr 17, 2024

View reviewed changes

contracts/contracts/coordination/SubscriptionManager.sol Outdated Show resolved Hide resolved

piotr-roslaniec added 2 commits April 18, 2024 09:46

wip: change of plans was a mistakerino

ab6db0a

wip: subscription manager draft

e3d996f

piotr-roslaniec force-pushed the managed-allow-list branch from 91f5f21 to e3d996f Compare April 18, 2024 08:02

piotr-roslaniec added 4 commits April 18, 2024 13:24

wip: add way too much comments

d82db48

wip: more tests, yes yes

c5d981f

wip: test test test

070ca81

wip: fix solhint

d684c6c

piotr-roslaniec force-pushed the managed-allow-list branch from 377caca to d684c6c Compare April 19, 2024 13:58

wip: add subscription tests draft

8cef8bb

piotr-roslaniec force-pushed the managed-allow-list branch from 6fce84b to 8cef8bb Compare April 19, 2024 14:33

wip: more subscription tests

e387159

piotr-roslaniec requested review from cygnusv and vzotova April 22, 2024 10:20

vzotova reviewed Apr 22, 2024

View reviewed changes

apply pr suggestions

08fe8a4

vzotova reviewed Apr 23, 2024

View reviewed changes

contracts/contracts/coordination/ManagedAllowList.sol Outdated Show resolved Hide resolved

vzotova reviewed Apr 23, 2024

View reviewed changes

contracts/contracts/lib/LookupKey.sol Outdated Show resolved Hide resolved

apply pr suggestions

dbe1ea2

piotr-roslaniec force-pushed the managed-allow-list branch from b8c8ba6 to dbe1ea2 Compare April 23, 2024 12:45

vzotova reviewed Apr 23, 2024

View reviewed changes

contracts/contracts/coordination/Subscription.sol Show resolved Hide resolved

apply pr suggestions

98dc648

piotr-roslaniec force-pushed the managed-allow-list branch from 54d150c to 98dc648 Compare April 23, 2024 16:42

piotr-roslaniec marked this pull request as ready for review April 24, 2024 13:03

piotr-roslaniec requested a review from arjunhassard April 24, 2024 13:03

arjunhassard assigned arjunhassard, vzotova, piotr-roslaniec and cygnusv May 7, 2024

cygnusv approved these changes May 10, 2024

View reviewed changes

piotr-roslaniec added 2 commits May 15, 2024 14:10

fix solhint failing silently on ci

138d716

remove solidity oopsies

7da09ad

derekpierre reviewed May 15, 2024

View reviewed changes

arjunhassard reviewed May 15, 2024

View reviewed changes

vzotova changed the base branch from main to epic-subscription May 17, 2024 15:19

vzotova mentioned this pull request May 17, 2024

Extract fees logic from Coordinator #265

Merged

7 tasks

vzotova merged commit df671df into nucypher:epic-subscription May 17, 2024
2 checks passed

cygnusv mentioned this pull request May 24, 2024

[WIP] Upfront payment contracts #253

Closed

7 tasks

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

`ManagedAllowList` #255

`ManagedAllowList` #255

piotr-roslaniec commented Apr 15, 2024 •

edited

Loading

piotr-roslaniec commented Apr 22, 2024

vzotova left a comment

vzotova Apr 22, 2024

piotr-roslaniec Apr 23, 2024

piotr-roslaniec commented Apr 24, 2024

cygnusv left a comment

derekpierre May 15, 2024

derekpierre May 15, 2024

arjunhassard May 15, 2024

piotr-roslaniec May 16, 2024

derekpierre May 15, 2024

derekpierre May 15, 2024 •

edited

Loading

derekpierre May 15, 2024 •

edited

Loading

arjunhassard May 15, 2024

piotr-roslaniec May 16, 2024 •

edited

Loading

derekpierre May 15, 2024

cygnusv May 15, 2024

derekpierre May 15, 2024

arjunhassard May 15, 2024

arjunhassard left a comment

arjunhassard May 15, 2024

piotr-roslaniec May 16, 2024

arjunhassard May 15, 2024

arjunhassard May 15, 2024

arjunhassard May 15, 2024

cygnusv commented May 16, 2024

	* @notice Calculates the available amount of fees that can be withdrawn be the beneficiary
	* @notice Calculates the available amount of fees that can be withdrawn by the beneficiary

		mnaged_allow_list = deployer.deploy(project.ManagedAllowList)
		deployments = [mnaged_allow_list]

ManagedAllowList #255

ManagedAllowList #255

Conversation

piotr-roslaniec commented Apr 15, 2024 • edited Loading

piotr-roslaniec commented Apr 22, 2024

vzotova left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

piotr-roslaniec commented Apr 24, 2024

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

derekpierre May 15, 2024 • edited Loading

Choose a reason for hiding this comment

derekpierre May 15, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

piotr-roslaniec May 16, 2024 • edited Loading

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

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

cygnusv commented May 16, 2024

`ManagedAllowList` #255

`ManagedAllowList` #255

piotr-roslaniec commented Apr 15, 2024 •

edited

Loading

derekpierre May 15, 2024 •

edited

Loading

derekpierre May 15, 2024 •

edited

Loading

piotr-roslaniec May 16, 2024 •

edited

Loading