pulse-scheduler: init ethereum contracts, subscriptions, updating prices #2572
Conversation
|
The latest updates on your projects. Learn more about Vercel for Git ↗︎
2 Skipped Deployments
|
tejasbadadare
changed the title
tejasbadadare
changed the title
- Updated price feed parsing to allow for a timestamp range of [-10s, now]. - Introduced validation for update conditions, including checks for heartbeat and price deviation. - Added new error handling for outdated timestamps and unmet update conditions. - Refactored subscription status to use a uint256 for last updated timestamp. - Expanded test coverage for update conditions and validation scenarios.
…b/pulse-scheduler/init
There was a problem hiding this comment.
looks like a good start to me. Feel free to address my comments in follow up PRs, as this is pretty big and it's generally fine to merge as is.
Please do let @ali-bahjati take a look before merging though.
|
|
||
| contract SchedulerState { | ||
| // Maximum number of price feeds per subscription | ||
| uint8 public constant MAX_PRICE_IDS = 10; |
There was a problem hiding this comment.
is this a reasonable limit? Seems like lending protocols could want more than 10 feeds ?
There was a problem hiding this comment.
Yeah it's an arbitrary number, will raise it to something higher
| _state.subscriptionNumber = 1; | ||
| } | ||
|
|
||
| function addSubscription( |
There was a problem hiding this comment.
suggest making this payable and having it call addFunds automatically. If you don't do that, then users will need to either do 2 transactions to create & fund a subscription (or use a multicall contract, which are available on some chains but not others...)
| // We will validate the trigger conditions and timestamps ourselves | ||
| // using the returned PriceFeeds. | ||
| uint64 maxPublishTime = SafeCast.toUint64(block.timestamp); | ||
| uint64 minPublishTime = maxPublishTime - 10 seconds; |
There was a problem hiding this comment.
i'm not sure 10 seconds is enough. Either way, you probably want this value to be configurable by the admin authority.
There was a problem hiding this comment.
Yeah true, will set it to 1 minute as default to allow for retries/flaky RPCs which might result in the tx landing later than expected.
| validPriceId = true; | ||
| break; | ||
| } | ||
| } |
There was a problem hiding this comment.
fyi instead of doing this loop, you can also check if requestedFeeds[i] is all zero below. (or check 1 field of it which is guaranteed to be nonzero in a valid update).
That approach also works better in the rare case where someone makes a subscription then tries to read it before it's ever updated.
| { | ||
| // TODO: This is gonna be expensive because we're iterating through | ||
| // all subscriptions, including deactivated ones. But because its a view | ||
| // function maybe it's not bad? We can optimize this. |
There was a problem hiding this comment.
yeah there may be some maximum size limit on the result (?) may need to paginate this or something like that
I also worry that there's a potential vulnerability here where someone goes and creates a billion subscriptions. If there are too many subscriptions, then maybe the keeper can't find the active ones, and that causes the service to go down.
| emit SubscriptionUpdated(subscriptionId); | ||
| } | ||
|
|
||
| function deactivateSubscription( |
There was a problem hiding this comment.
should this be a special case of updateSubscription -- then you can also re-activate deactivated subscriptions?
There was a problem hiding this comment.
Hm, when writing this I had considered deactivating a subscription to be a one-way operation. But it is better UX to allow someone to pause their subscription but maintain the balance and settings, so they can resume later. To make it a special case of updateSubscription, we'd need to move isActive from SubscriptionStatus to SubscriptionParams, which maybe feels a little strange from a data model perspective. I considered adding a updateSubscriptionIsActive(isActive: bool), but I think i prefer just one unified updateSubscription(). Will make that change
| scheduler.updatePriceFeeds(subscriptionId, updateData2, priceIds); | ||
| } | ||
|
|
||
| function testUpdatePriceFeedsRevertsOnUpdateConditionsNotMet_Deviation() |
There was a problem hiding this comment.
what's with the _ in the name here and in the method above
Summary
Initialize the Pulse Scheduler Ethereum contracts with a base layer of functionality. Follows the structure/boilerplate of the existing Pulse contracts.
Rationale
Add state, functions, and tests for:
How has this been tested?
Up next