Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
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 pulse contracts #2090
feat(pulse): add pulse contracts #2090
Changes from 27 commits
911887f
d05d869
f017f29
19ae7ea
fbbae70
061dfb2
ccaab98
27f665b
035938f
a7a4d23
183658e
e957c79
54d310e
2ca191a
c455937
b313728
8f64744
bce0770
9d5cd7e
81e4535
cda5a2b
604b113
dedca30
d748162
5399eb0
7cd8328
9e046dc
1f82d5e
File filter
Filter by extension
Conversations
Jump to
There are no files selected for viewing
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 we should have
provider
but if you don't haveprovider
i don't think you need this parameter either.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.
what is this for :?
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.
this is also referenced from the entropy contract -- which is to optimize for gas costs, by pre-filling the storage slots with non-zero values during initialization we pay the high gas cost once during deployment and hence subsequent writes to these slots will cost less 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.
Hmm thinking about it, now that we don't emit the ids and don't store them (we store the hash of it), how argus is supposed to understand it :? parse the transactions?
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 we should still emit the event with the priceId and the trade-off of higher gas costs for event emission is necessary because the system can't function without Argus having access to the price IDs, we can keep the storage the hash still so that it remains optimized, what do you think?
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 50% overhead is too much, the overhead is mostly constant.
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.
why do you need this check? Why not just run the rest of the code, and then either you run out of gas or you don't?
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 it just provides better UX by failing early with a clear error message, also actual gas paid is gas used
there might be a situation where:
callbackGasLimit
topulseCallback
gasleft()
is alsocallbackGasLimit
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 don't think you want these catches either. If either of these errors happens for a transient reason, it means the callback can't be called in the future.
In entropy if a callback fails, we leave the request open. It could get fulfilled later, or it could just sit there forever.
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.
(this is actually important. In practice, we have callbacks that error on the first try and then succeed on a later retry)
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.
Oh you also need to use the Check-Effects-Interactions pattern here, meaning the
clearRequest
call (the effect) needs to happen before cross-contract calls (interactions). There's a whole class of security vulnerabilities if you don't do the interactions last, because the cross-contract call can call back into the Pulse contract and then weird stuff can happen.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.
the reason why we have this try-catch is because @m30m said that in entropy it's difficult to know if its the user error or our error so logging this will provide better UX, we can document that the callback contract should not fail, and if it fails we wont retry
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.
It's good to emit something (for debugging purposes), just be aware that it might be expensive to do this in ethereum mainnet.
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.
agreed but apart from debugging this is also useful for off-chain indexing/integration so i think the benefits outweigh the costs
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'm wondering how this works in Entropy. Do we update pythFee regularly based on gasPrice? @m30m might know better but similar products charge fee as % of the tx fee.
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.
There is also one DoS attack vector in our price feeds use case regarding gas prices. People can ask for price updates anytime in the future and it won't be fullfilled immediately (opposed to Entropy) and this might make
tx.gasprice
a worse estimate on normal usage (only a few seconds after that) or can be taken advantage of to make hundreds of requests in the future when the gas price is lower.My recommendation is to not allow a publish time that is more than 1min in the future. Or charge higher fees as it goes more into the future.
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 that's a good point, i think not allowing a publish time more than 1 min in the future is a reasonable solution
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.
In Entropy, the keeper updates its fee over time.
I think the thing to do here is to give the keeper a configurable
feePerGas
parameter instead oftx.gasprice
. The keeper then sets both a base fee and thefeePerGas
so it's a linearax+b
pricing model and then we can figure out how to do this properly off chain.