- 
                Notifications
    
You must be signed in to change notification settings  - Fork 305
 
feat(pulse-scheduler): add permanent subscriptions, allow permissionless funding, clean up tests #2593
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
feat(pulse-scheduler): add permanent subscriptions, allow permissionless funding, clean up tests #2593
Conversation
| 
           The latest updates on your projects. Learn more about Vercel for Git ↗︎ 6 Skipped Deployments
  | 
    
| revert IllegalPermanentSubscriptionModification(); | ||
| } | ||
| } | ||
| } | 
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think you can also effectively disable a subscription by (1) changing the readerWhitelist or (2) making the gas config so limiting that no tx ever lands on chain?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
hmm I think you could also do it by changing the updateCriteria
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
yeah good points -- will disallow changing the whitelist or the config. truly "permanent."
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
just realized they can also just set isActive to false 🤦 updated
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I approved this but then realized there are some issues here
| for (uint i = 0; i < currentParams.priceIds.length; i++) { | ||
| bool found = false; | ||
| for (uint j = 0; j < newParams.priceIds.length; j++) { | 
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Heh, we're stuck doing an m x n linear search in Solidity?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
we cant create in-memory hashmaps so yeah, back to the basics 🫠
| currentParams.gasConfig.maxPriorityFeeMultiplierCapPct | ||
| ) { | ||
| revert IllegalPermanentSubscriptionModification(); | ||
| } | 
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think these gas parameters could cause some problems still. The chain usage will change over time, and these parameters may need to be tweaked. If you can't change them, then they could initially be set too low, and then the chain gets more congested and then you don't get updates.
Maybe we need to set these to infinity for the permanent subscriptions? not sure exactly what to do.
(this is a problem we can resolve later also, so I'm approving this PR)
Summary
Rationale
It's valuable for some consumers to lock down a subscription so that it can't be tampered by a manager. They may simply be using a vault manager hot wallet there, not a multisig. That wallet could potentially rug the subscription. But, other users may find it useful to be able to stop a subscription, e.g. someone subscribes to a memecoin that goes to 0 and they want to stop the sub. Thus, we add an optional
isPermanentflag to the subscription, which will disallow destructive actions when enabled. Destructive actions are defined as:No reason to lock down addFunds. Perhaps the community wants to sponsor a feed. It's also operationally simpler for a customer to use any wallet to fund.
How has this been tested?