-
Notifications
You must be signed in to change notification settings - Fork 224
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): add provider #2279
Conversation
The latest updates on your projects. Learn more about Vercel for Git ↗︎
3 Skipped Deployments
|
uint128 providerFeeInWei = _state.providers[provider].feeInWei; | ||
uint256 gasFee = callbackGasLimit * providerFeeInWei; |
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.
as discussed, changed this to use provider fee to protect from future price jumps/drops
// Check if enough gas remains for callback + events/cleanup | ||
// We need extra gas beyond callbackGasLimit for: | ||
// 1. Emitting success/failure events | ||
// 2. Error handling in catch blocks | ||
// 3. State cleanup operations | ||
if (gasleft() < (req.callbackGasLimit * 3) / 2) { | ||
revert InsufficientGas(); | ||
} | ||
|
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.
removed this because if insufficient gas error occurs within callback then PriceUpdateCallbackFailed
will be emitted with "low-level error (possibly out of gas)"
and otherwise if insufficient gas for the executeCallback
function itself, it will revert with out-of-gas
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 thought what we agreed was that the user would not be involved in selecting provider. we instead give exclusivity to providers to update prices early; if they don't do it after a few seconds anyone can do it.
my understanding regarding to that is that we wanted to go with the if we decide to just go with the simple exclusivity option then yeah we can implement this now, i can add this in a following PR |
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.
LGTM!
No description provided.