refactor(high-level): Don't require &mut Block to improve the API for developers
#93
Conversation
|
I talked to @frol, and we agreed that parsing Instead of parsing all the logs for the events, we will do it on demand when |
| .map(Into::into) | ||
| .collect(); | ||
| } | ||
| pub fn receipts(&self) -> impl Iterator<Item = &receipts::Receipt> { |
There was a problem hiding this comment.
Is there a strong need to return an iterator? I would just return a slice whenever possible:
| pub fn receipts(&self) -> impl Iterator<Item = &receipts::Receipt> { | |
| pub fn receipts(&self) -> &[receipts::Receipt] { | |
| &self.executed_receipts |
| &self, | ||
| account_id: &crate::near_indexer_primitives::types::AccountId, | ||
| ) -> Vec<events::Event> { | ||
| let account_id_clone = account_id.clone(); // Clone the account_id |
There was a problem hiding this comment.
You don't need to clone it at all, just use the account_id below
| /// Returns a Vec of [Events](crate::events::Event) emitted in the [Block] | ||
| pub fn events(&self) -> HashMap<super::ReceiptId, Vec<events::Event>> { |
There was a problem hiding this comment.
The doc and the return value are out of sync. I would keep the original return value (iterator)
| } | ||
| if let Some(events) = self.events.get(receipt_id) { | ||
| pub fn events_by_receipt_id(&self, receipt_id: &super::ReceiptId) -> Vec<events::Event> { | ||
| if let Some(events) = self.events().get(receipt_id) { |
There was a problem hiding this comment.
This code is faster to execute and easier to maintain:
self.receipt_by_id(receipt_id)
.map(|receipt| receipt.events())
.unwrap_or_default()| .values() | ||
| .flatten() | ||
| .filter(|event| event.is_emitted_by_contract(&account_id_clone)) | ||
| .map(Clone::clone) |
There was a problem hiding this comment.
Just want to point out that you turned .events() to return HashMap only to turn it into an iterator of values and flatten them... Revert the changes here, the old implementation was good (except the unnecessary cloning of account_id
Recently, I've received feedback about the usage of high-level updates.
It is impossible to use the framework like this:
The reason is that we require the
blockto be&mut Block. I did it because I attempted to keep theBlocksize lower. I intended to empty some fields (receipts,transactions,state_changes,etc.) and set the value to them on demand. For example, a developer doesblock.transactions(). We extract the transactions from the underlyingblock if the value is empty.streamer_message: StreamerMessage.While I still believe that idea makes some sense, this adds inconveniences to other places. It does not make sense to attempt to save a small amount of bytes and ruin the DevEx.
In this PR, I made the
Blockstructure to have all the data at the time of the conversion from theStreamerMessage. This allows me to eliminate the requirement to deal with&mut Blockand makes the code above possible to use without redundantclone().Feedback is welcome, meanwhile, I need to revisit the doc side for the leftover referrals to the
&mut Blockand correct them.