feat(pulse): add governance instructions, admin transfer capability #2608
Conversation
|
The latest updates on your projects. Learn more about Vercel for Git ↗︎ 6 Skipped Deployments
|
tejasbadadare
changed the title
| // Check minimum balance if number of feeds increases and subscription remains active | ||
| if ( | ||
| willBeActive && | ||
| newParams.priceIds.length > currentParams.priceIds.length |
There was a problem hiding this comment.
I think it's better if you do this check regardless. Otherwise I can increase feeds and deactivate, and then later activate it to bypass it right?
There was a problem hiding this comment.
yeahh was debating this, but one thing i was thinking was we shouldn't enforce this for people trying to deactivate or reduce their feeds. maybe they are low on balance and trying to reduce their burn rate. ultimately the minimum balance thing serves 2 purposes: disincentivize the attack vector of flooding the system with subscriptions, and help the user maintain enough balance in their account to ensure uninterrupted operation.
also, we do enforce the check when activating, so the scenario you mentioned wouldn't be a problem.
| scheduler.addFunds{value: requiredFunds}(subscriptionId); | ||
|
|
||
| // Verification 2: Update should now succeed | ||
| vm.expectEmit(); | ||
| emit SubscriptionUpdated(subscriptionId); | ||
| scheduler.updateSubscription(subscriptionId, newParams); |
There was a problem hiding this comment.
I recommend you let updateSubscription be payable as well so people can update it in 1 go (now they need to add funds first, and then update it)
There was a problem hiding this comment.
agreed, had the same thought :) thought it would be too much of an related change to include in this PR, so was planning to add it in another one. but i'll add it in here now that you've brought it up.
@ali-bahjati You know, I had the same thought on this, but I took cues from Entropy here and assumed that there is a reason :) The executor will need to be the owner to be able to upgrade the contract, which makes the admin role perhaps redundant? But i did notice in git history for Entropy that folks wanted the admin role and for it to be transferable. I'll keep both in for now, but it's easy to remove. Let me know what you think. |
We resolved this offline, but for posterity: turns out we don't really remember why it was done this way in Entropy. the Owner and Admin roles are functionally the same because they are both held by Executor. Perhaps admin was intended to be some kind of "light governance" that can execute smaller administrative instructions, but not have the big hammer authority to upgrade the contract? In any case we'll leave it the same in Pulse for consistency and have Executor assume both roles (edited) |
2 participants
Summary
Rationale
upgradeContractsetMinimumBalancePerFeedsetSingleUpdateKeeperFeeInWeiHow has this been tested?