dex: Generate candlestick data and provide RPCs for access #4398
Conversation
crates/core/component/dex/src/component/candlestick/candlestick.rs
Outdated
Show resolved
Hide resolved
| let mut direct_volume = 0.0; | ||
|
|
||
| // Create summary data on a per-trading pair basis. | ||
| for execution in block_executions { |
There was a problem hiding this comment.
Q: do we know that every block_executions that appears here will be nonempty? In other words, do we have to worry about whether our sentinel values like f64::INFINITY would leak through into the data we produce?
My assumption is that we wouldn't end up with an empty block_executions but wanted to check.
There was a problem hiding this comment.
I don't believe it should be possible but I can add some checks to the record_position_execution and record_swap_execution to make sure that none are included.
There was a problem hiding this comment.
penumbra/crates/core/component/dex/src/component/position_manager.rs
Lines 375 to 376 in d6299e9
There was a problem hiding this comment.
It does look like it's possible for no-op swap executions to be recorded, so an additional check there would be useful.
| // Update the candlestick tracking | ||
| self.record_position_execution(&prev_state, &new_state, &context) | ||
| .await?; | ||
|
|
There was a problem hiding this comment.
Q: would it make sense to combine the candlestick recording with the event emission, either by moving this up to 377 where the event is emitted, or by moving the event emission inside the candlestick call, so that we always record the candlestick data and the event together?
There was a problem hiding this comment.
I considered this however the event emission and candlestick creation are different concerns and the position_execution method is supposed to be the sole method tracking position executions and tying the various subsystems together so this seemed most design-legible to me.
There was a problem hiding this comment.
Hmm, the way I had thought about it is that the candlestick RPC is basically a very special-purpose embedded indexer (and the reason we want to put it in the fullnode by default is so that any frontend can assume it will be there). Either way, though, I think it's fine.
zbuc
changed the title
crates/core/component/dex/src/component/candlestick/chandelier.rs
Outdated
Show resolved
Hide resolved
crates/core/component/dex/src/component/router/route_and_fill.rs
Outdated
Show resolved
Hide resolved
|
Some suggestions, will take it for a spin trying to generate price data |
|
I am rebasing this now fyalli |
…air rather than contextual trading pair
This reverts commit 577f653.
| }; | ||
| let mut block_executions = self.block_executions_by_pair(&trading_pair).clone(); | ||
|
|
||
| let execution_price = U128x128::ratio(swap.output.amount, swap.input.amount) |
Describe your changes
This adds support for tracking candlestick data to the dex component via a new
Chandeliertrait.Chandelierare called which will extract and accumulate data into an orderedVecstored in the object storeCandlestickDataand saves it in non-verifiable storage under the keydex/candlesticks/{pair}/{height:020}so that they can be iterated over in orderIssue ticket number and link
#4399
Checklist before requesting a review
If this code contains consensus-breaking changes, I have added the "consensus-breaking" label. Otherwise, I declare my belief that there are not consensus-breaking changes, for the following reason: