refactor: batch note request call to rpc_api #1617
Conversation
juan518munoz
added
the
no changelog
juan518munoz
force-pushed
the
jmunoz-batch-note-request-to-node
branch
from
605e1d9 to
602a4b7
Compare
| for (note_id, note) in notes_by_id { | ||
| if let Some(note) = note { | ||
| if let InputNoteState::Expected(ExpectedNoteState { tag: Some(tag), .. }) = | ||
| note.state() | ||
| { | ||
| self.insert_note_tag(NoteTagRecord::with_note_source(*tag, note_id)).await?; | ||
| } | ||
| self.store.upsert_input_notes(&[note]).await?; | ||
| } | ||
| } |
There was a problem hiding this comment.
I think you should be able to do this once at the end, no? Instead of for every type
There was a problem hiding this comment.
Correct, we can declare an empty vector and extend it with the results as we retrieve them.
Addressed in d782650
| /// ([`NOTE_TAG_LIMIT`](crate::rpc::NOTE_TAG_LIMIT)). | ||
| pub async fn import_notes( | ||
| &mut self, | ||
| note_files: Vec<NoteFile>, |
There was a problem hiding this comment.
Seems like we already collect three vectors in the function itself, so maybe this could take a slice or an iterator so as to not have to make the caller collect into another list?
| NoteFile::NoteWithProof(note, _) => note.id(), | ||
| }; | ||
|
|
||
| let previous_note = self.get_input_note(id).await?; |
There was a problem hiding this comment.
I think we can get many notes at once before the loop
| }); | ||
|
|
||
| let committed_note_data = if let Some(tag) = tag { | ||
| self.check_expected_note(after_block_num, tag, note_record.details()).await? |
There was a problem hiding this comment.
As we talked offline, we should be able to check many tags at once here.
|
|
||
| match committed_note_data { | ||
| Some((metadata, inclusion_proof)) => { | ||
| let mut current_partial_mmr = self.store.get_current_partial_mmr().await?; |
There was a problem hiding this comment.
Ideally we only build the MMR once
| if let Some(block_height) = self | ||
| .rpc_api | ||
| .get_nullifier_commit_height( | ||
| ¬e_record.nullifier(), | ||
| inclusion_proof.location().block_num(), | ||
| ) | ||
| .await? |
There was a problem hiding this comment.
I think we could create a
async fn get_nullifiers_commit_height(
&self,
nullifier: &[Nullifier],
block_num: BlockNumber,
) -> BTreeMap<Nullifier, BlockNumber>
that could sync many nullifiers at once
igamigo
left a comment
igamigo
left a comment
There was a problem hiding this comment.
LGTM, thanks @SantiagoPittella for merging the latest changes! I left some comments, mostly minor (we can disregard these), but one related to returning the IDs of all the notes that were upserted instead of the ones that were passed by the user which would be great to address before merging
|
|
||
| let sync_height = self.get_sync_height().await?; | ||
| // Import fetched notes | ||
| let mut note_requests = vec![]; |
There was a problem hiding this comment.
nit: we can use Vec::with_capacity(notes.len()) here
| let prefixes: Vec<u16> = | ||
| requested_nullifiers.iter().map(crate::note::Nullifier::prefix).collect(); |
There was a problem hiding this comment.
We could make this function take BTreeSet<Nullifier> instead of &[Nullifier] to enforce no prefix duplicates (otherwise, we need to do this check in the call site)
crates/rust-client/src/rpc/mod.rs
Outdated
| /// The default implementation of this method uses | ||
| /// [`NodeRpcClient::sync_nullifiers`]. | ||
| async fn get_nullifier_commit_height( | ||
| async fn get_nullifiers_commit_height( |
There was a problem hiding this comment.
nit: I think get_nullifier_commit_heights to emphasize the thing we are querying for (or maybe get_commit_heights_for_nullifiers?)
crates/rust-client/src/rpc/mod.rs
Outdated
| &self, | ||
| nullifier: &Nullifier, | ||
| requested_nullifiers: &[Nullifier], | ||
| block_num: BlockNumber, |
There was a problem hiding this comment.
minor nit: I think we could name this block_from so that we make it more explicit (although the docs already mention what it's for)
| pub async fn import_notes( | ||
| &mut self, | ||
| note_files: &[NoteFile], | ||
| ) -> Result<Vec<NoteId>, ClientError> { |
There was a problem hiding this comment.
We should probably explain that if one NoteFile is invalid (or any note is in Processing state) the whole function will error and no notes would be imported.
Though I wonder if we would prefer import the ones that can be imported and return a list of imported NoteIds (if we do this, let's do it on a separate PR/issue).
There was a problem hiding this comment.
I explain it in the docs,.
| } | ||
|
|
||
| Ok(id) | ||
| Ok(note_ids) |
There was a problem hiding this comment.
We should return the IDs of notes that got imported. import_* methods return Option because some may not be found, so really here we should return the IDs for every note in imported_notes (and update the documentation). I think this also allows us to get rid of the last note_ids.clone()
Closes #1596