chore: minor cleanup flashblocks logic #357
Conversation
|
The latest updates on your projects. Learn more about Vercel for Git ↗︎ 1 Skipped Deployment
|
0xKitsune
changed the title
| @@ -193,7 +182,7 @@ pub struct FlashblocksService { | |||
| } | |||
|
|
|||
| impl FlashblocksService { | |||
| pub fn new(client: RpcClient, outbound_addr: SocketAddr) -> eyre::Result<Self> { | |||
| pub fn new(client: RpcClient, outbound_addr: SocketAddr) -> io::Result<Self> { | |||
There was a problem hiding this comment.
Why are we using io in here?
We should keep eyre (or better use anyhow)
FlashblocksService::new should not return specialized io error IMO
There was a problem hiding this comment.
I used io::Result here because the only fallible operation in FlashblocksService::new() is constructing the WebSocketPublisher which returns an io::Result. Since this is the sole error source, returning the specific error type is more explicit and avoids unnecessary dynamic error wrapping and keeps the API clear. Lmk if I am overlooking something but I do not see a downside in this case to using io::Result as it accurately reflects exactly what can fail.
With that being said I understand the argument for using eyre or anyhow for consistency or simplicity but I chose to go with the above approach to prefer being explicit. If there is a strong preference go with eyre, Im happy to adjust.
There was a problem hiding this comment.
Also main concern that if we add something to the new we will need to change return types.
At some point i was thinking of adding redis to keep traces ids between builder and boost. It that case i would need to change to general type again, because redis error won't cast into io (maybe it would)
| // Validate the index is contiguous | ||
| if payload.index != self.flashblocks.len() as u64 { | ||
| return Err(FlashblocksError::InvalidIndex); | ||
| } | ||
|
|
||
| // Check base payload rules | ||
| if payload.index == 0 { |
There was a problem hiding this comment.
I would like to add a test for this before rewriting it to make sure the migration works as expected. I can do this today.
This PR simplifies and cleans up components of the flashblocks service implementation:
FlashblockBuilder::extend().flat_mapcollectors with a singlefoldfor more efficient transaction/withdrawal aggregation.impl From<FlashblocksError> for RpcClientError.get_best_payloadto return a singleResult<Envelope>instead of anOptionwrapper.