Repository navigation
refactor(events): restructure filecoin and eth events. - #7608
akaladarshi wants to merge 3 commits into
Conversation
…om them A tipset's events are collected once, numbered in message order, with each emitter resolved synchronously against the post-execution state. The block logs bloom is a fold over the Ethereum projection, so the second emitter resolver and event shaper in bloom.rs are gone.
|
Important Draft PR not reviewedDraft PRs are not automatically reviewed by default.
To automatically review draft PRs, update your CodeRabbit configuration: reviews:
auto_review:
drafts: trueComment |
LesnyRumcajs
left a comment
LesnyRumcajs
left a comment
There was a problem hiding this comment.
The direction seems sound, I also don't love our sprawl of free functions but they're often the most optimal ones (hello C). Couple of things to think about before going forward with this:
- It doesn't seem to me lots of this logic should live under
rpc/methods/eth; more likesrc/eth/*. Ideally,rpc/*would contain logic that is strictly there for RPC handling. As you mentioned, tipset events are a chain-level artifact. - Performance - it seems to me that there might be a lot of redundant work with building those structs. We'd need A LOT of care not to introduce a regression in affected methods.
- Error handling - it's also unclear to me whether this design preserves error handling logic.
In general, it'd be worth having a broader discussion with multiple alternatives so that we can have a consensus why a given one is the best one.
Also, as a broader refactoring on user-critical logic, this would need first a good coverage net (not sure what's the % at the moment); refer to Working Effectively with Legacy Code from Michael Feathers. I really wouldn't like to have regressions for the sake of nicer (subjectively) code. At the same time, if refactoring could open vistas for improved performance, I'm convinced everyone would be on board with that.
A good first step in doing some refactoring would be just moving things around (without ANY logic changes); I already mentioned it elsewhere, but the chain logic should be minimal in rpc/*. Afterwards, with proper coverage on high-level methods we could tinker around if it makes sense.
| use fvm_ipld_encoding::IPLD_RAW; | ||
|
|
||
| /// The Ethereum logs of one tipset, grouped by message, in tipset order. | ||
| pub struct BlockLogs { |
There was a problem hiding this comment.
| pub struct BlockLogs { | |
| pub struct EthBlockLogs { |
This is longer but arguably more descriptive.
| /// Collects the logs of an executed tipset. | ||
| pub fn collect( | ||
| state_manager: &StateManager, | ||
| tipset: &Tipset, | ||
| executed: &ExecutedTipset, | ||
| ) -> anyhow::Result<Self> { | ||
| Self::from_events(&TipsetEvents::collect(state_manager, tipset, executed)) | ||
| } |
There was a problem hiding this comment.
I think it's a bit awkward as an associated function - seems like there should be a method on TipsetEvents to convert to BlockLogs instead.
|
|
||
| impl TipsetEvents { | ||
| /// Collects the events of an executed tipset. | ||
| pub fn collect( |
There was a problem hiding this comment.
| pub fn collect( | |
| pub fn from_tipset( |
| tipset: &Tipset, | ||
| executed: &ExecutedTipset, |
There was a problem hiding this comment.
having both tipset and executed to pass is a bit awkward; ExecutedTipsetseems more likeTipsetExecutionResult`. I understand it's what we have at the moment, but it's not an ideal name.
|
|
||
| /// Global cache first, then the finality-deep state (cached globally when reorg-stable), | ||
| /// then the post-execution state, the only one holding an actor created in this tipset. | ||
| fn resolve_uncached(&self, emitter: ActorID) -> Option<Address> { |
There was a problem hiding this comment.
Don't we have this logic somewhere already?
This branch has not been deployed
2 participants
Summary of changes
Changes introduced in this pull request:
TipsetEventsallows collection of all the event produce in a single tipset.BlockLogsholds all theEthLogproduced by each message in a block.Reference issue to close (if applicable)
Closes
Other information and links
Change checklist
Outside contributions