StandardSubscription #339
StandardSubscription #339
Conversation
|
Since the name of the subscription contract has changed:
Maybe it makes sense to add a |
| @@ -1796,813 +1796,6 @@ | |||
| } | |||
| }, | |||
| "137": { | |||
| "BqETHSubscription": { | |||
There was a problem hiding this comment.
Do we need to save this entry in a separate bqeth.json file for deployment tracking purposes (with the rename of course), or is that tracked already somewhere else? Basically what you did for the marlin deployment in new-subscription-mainnet.json.
| @@ -0,0 +1,1039 @@ | |||
| { | |||
There was a problem hiding this comment.
Should we rename to marlin.json or something analogous?
There was a problem hiding this comment.
Also, do we need GlobalAllowList in this file since it is already tracked in mainnet.json? I guess it is a remnant from the fact that the deployment included a fresh deployment of GlobalAllowList - but for the purposes of the subscription registry tracking for marlin, isn't really needed.
| @@ -0,0 +1,65 @@ | |||
| deployment: | |||
There was a problem hiding this comment.
Should we call this file marlin-subscruption.yml?
There was a problem hiding this comment.
Not necessarily, there's nothing in here that's specific to them. It's just the updated fees and parameters for every potential new subscription. In this case, we include as well logic to deploy a new GlobalAllowList (coincidentally, this happened with BqETH too) but that can be removed when we deploy another new subscription.
There was a problem hiding this comment.
I guess we really did two separate actions in one script:
- deploy a new global allow list contract (only needed this time, but not unless there are future changes to global allow list).
- deploy a new subscription contract based on parameters.
Moving forward, a "new subscription" would only use the 2nd part with some potential tweaks to the values eg. duration. Just trying to think about our code maintenance moving forward - this params file and the associated script would need to remove the global allow list component the next time we do a subscription.
Not something to be done for this PR but some follow-up reorg can be done afterward eg.
- separate params/script for updating global allow list implementation
- update "new subscription files" to not include anything with global allow list so that is more of a base that can be used for other adopters.
| @@ -91,6 +91,20 @@ def global_allow_list(project, deployer, coordinator): | |||
| return contract | |||
|
|
|||
|
|
|||
| @pytest.fixture() | |||
| def upgradeable_global_allow_list(project, deployer, coordinator, oz_dependency): | |||
There was a problem hiding this comment.
Do we envision using an upgradeable global allow list moving forward by default? If so, should we just update the original global_allow_list fixture to use a proxied instance?
There was a problem hiding this comment.
I'd keep original global allow list intact and transform this upgradeable to the managed allow list after it will be ready.
wdyt?
There was a problem hiding this comment.
If so, should we just update the original global_allow_list fixture to use a proxied instance?
In tests I keep both versions and parametrize the test.
There was a problem hiding this comment.
In tests I keep both versions and parametrize the test.
Yep, just thinking that we don't really need both since the testing of either is the same, but not an issue if you want to keep both.
From the test's perspective, whether proxied or not doesn't functionally matter - but a proxied contract is what is deployed so we would match what is deployed.
We duplicate this fixture in other places too which we should proxy:
There is an issue for cleaning up duplicated fixtures across nucypher-contracts, and this can be addressed then - #284
There was a problem hiding this comment.
Agreed, let's address that as part of #284
|
Let's address artifacts reorganization in a subsequent PR. I don't have enough clarity of what's the best way moving forward, particularly since I think there's reliance from code in nucypher (see nucypher/nucypher#3503) and nucypher-contracts (e.g. https://github.com/nucypher/nucypher-contracts/blob/main/scripts/manage_subscription.py) on the presence of specific entries in the registry. To be clear, I'm not advocating to keep these in the registry (I also agree that the registry should only be used to keep protocol specific contracts), but these dependencies should be removed first. |
Type of PR:
Required reviews:
What this does:
BqETHSubscriptiontoStandardSubscriptionGlobalAllowListis now upgradeableGlobalAllowListand newStandardSusbcriptionto Polygon Mainnet (including proxies andProxyAdmins)Issues fixed/closed:
Why it's needed:
Notes for reviewers: