feat(pulse): add pulse contracts #2090
Conversation
|
The latest updates on your projects. Learn more about Vercel for Git ↗︎
2 Skipped Deployments
|
…k functions to IPulse interface
| // Check if enough gas remains for the callback plus 50% overhead for cross-contract call (uses integer arithmetic to avoid floating point) | ||
| if (gasleft() < (req.callbackGasLimit * 3) / 2) { | ||
| revert InsufficientGas(); | ||
| } |
There was a problem hiding this comment.
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.
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:
- user provides exactly
callbackGasLimittopulseCallback gasleft()is alsocallbackGasLimit- we don't have enough gas for events/cleanup
- transaction fails with generic out-of-gas error
| function getFee( | ||
| uint256 callbackGasLimit | ||
| ) public view override returns (uint128 feeAmount) { | ||
| uint128 baseFee = _state.pythFeeInWei; | ||
| uint256 gasFee = callbackGasLimit * tx.gasprice; | ||
| feeAmount = baseFee + SafeCast.toUint128(gasFee); | ||
| } |
There was a problem hiding this comment.
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 of tx.gasprice. The keeper then sets both a base fee and the feePerGas so it's a linear ax+b pricing model and then we can figure out how to do this properly off chain.
target_chains/ethereum/contracts/contracts/pulse/PulseState.sol
Outdated
Show resolved
Hide resolved
| interface IPulseConsumer { | ||
| function pulseCallback( | ||
| uint64 sequenceNumber, | ||
| address updater, |
There was a problem hiding this comment.
I think we should have provider but if you don't have provider i don't think you need this parameter either.
| req.requester, | ||
| "low-level error (possibly out of gas)" | ||
| ); | ||
| } |
There was a problem hiding this comment.
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.
(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.
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.
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
| SafeCast.toUint64(req.publishTime) | ||
| ); | ||
|
|
||
| // Check if enough gas remains for the callback plus 50% overhead for cross-contract call (uses integer arithmetic to avoid floating point) |
There was a problem hiding this comment.
i think 50% overhead is too much, the overhead is mostly constant.
No description provided.