feat: check if note is consumed for `NoteFile::NoteId` and `NoteFile::NoteWithProof` #449

tomyrd · 2024-07-25T20:56:30Z

Deals with points 1 and 2 from this comment.

This PR uses the new CheckNullifiersByPrefix node endpoint to deal with imported notes that are already consumed.

When a note file of types NoteFile::NoteId and NoteFile::NoteWithProof is imported the client checks if the nullifier is consumed in the node. If that's the case then the note is stored as NoteStatus::Consumed.

igamigo

LGTM! Left some minor blocking comments.

igamigo · 2024-08-01T19:01:19Z

Makefile

@@ -10,7 +10,7 @@ FEATURES_CLIENT="testing, concurrent"
 FEATURES_CLI="testing, concurrent"
 NODE_FEATURES_TESTING="testing"
 WARNINGS=RUSTDOCFLAGS="-D warnings"
-NODE_BRANCH="main"
+NODE_BRANCH="tomyrd-get-nullifiers-by-prefix"


I think we should be ok to move this to next again, right?

igamigo · 2024-08-01T19:02:14Z

bin/miden-cli/src/commands/import.rs

-    client: &mut Client<N, R, S, A>,
-    filename: PathBuf,
-) -> Result<NoteId, String> {
+pub async fn read_note_file(filename: PathBuf) -> Result<NoteFile, String> {


nit: Should we remove pub from this?

crates/rust-client/src/notes/mod.rs

igamigo · 2024-08-01T19:08:13Z

crates/rust-client/src/rpc/tonic_client/mod.rs

+    async fn check_nullifiers_by_prefix(
+        &mut self,
+        prefixes: &[u16],
+    ) -> Result<Vec<(miden_objects::notes::Nullifier, u32)>, RpcError> {


Very minor nit: I'd import miden_objects::notes::Nullifier in this mod in order to not make this return type too explicit here.

SantiagoPittella

lgtm

bobbinth

Looks good! Thank you! Not a super-detailed review - but I did leave some comments inline.

crates/rust-client/Cargo.toml

crates/rust-client/src/rpc/web_tonic_client/mod.rs

crates/rust-client/src/notes/mod.rs

igamigo · 2024-08-07T20:00:57Z

crates/rust-client/src/sync/mod.rs

@@ -614,3 +614,7 @@ impl<N: NodeRpcClient, R: FeltRng, S: Store, A: TransactionAuthenticator> Client
        Ok(())
    }
 }
+
+pub fn get_nullifier_prefix(nullifier: &Nullifier) -> u16 {


Does this need to be pub?

No, pub(crate) should be enough

igamigo · 2024-08-07T20:04:35Z

crates/rust-client/src/store/sqlite_store/notes.rs

@@ -47,6 +47,7 @@ type SerializedInputNoteData = (
    Option<String>,
    bool,
    Option<u32>,
+    Option<u64>,


Block number is u32 so this should likely be u32

igamigo · 2024-08-07T20:08:19Z

crates/rust-client/src/store/note_record/input_note_record.rs

+                block_height: inclusion_proof.location().block_num() as u64,
+            }
+        } else {
+            NoteStatus::Expected { created_at: 0 }


Looking at this, probably the created_at field should be Option<u32> instead of u32

igamigo · 2024-08-07T20:08:34Z

crates/rust-client/src/store/note_record/input_note_record.rs

@@ -216,11 +216,19 @@ impl From<Note> for InputNoteRecord {

 impl From<InputNote> for InputNoteRecord {
    fn from(recorded_note: InputNote) -> Self {
+        let status = if let Some(inclusion_proof) = recorded_note.proof() {
+            NoteStatus::Committed {
+                block_height: inclusion_proof.location().block_num() as u64,


Similar to the other comment, this should be u32

igamigo · 2024-08-07T20:21:54Z

crates/rust-client/src/notes/import.rs

+            inclusion_details.merkle_path.clone(),
+        )?;
+
+        let tracked_note = maybe_await!(self.get_input_note(id));


nit: This tracked_note sounds misleading now that we specifically say "tracked" notes to refer to notes that have a tag that the client is tracking, perhaps it should be store_note or similar

igamigo · 2024-08-07T20:24:01Z

crates/rust-client/src/notes/import.rs

+        }
+    }
+
+    async fn nullifier_block_num(


nit: I'd rename this to get_nullifier_block_num()

bobbinth

Looks good! Thank you. I left some comments inline.

crates/rust-client/src/rpc/web_tonic_client/mod.rs

crates/rust-client/src/notes/import.rs

igamigo · 2024-08-14T15:04:08Z

crates/rust-client/src/rpc/tonic_client/mod.rs

+            .iter()
+            .map(|nul| {
+                let nullifier = nul.nullifier.clone().ok_or(RpcError::ExpectedFieldMissing(
+                    "CheckNullifiersByPrefix response should have a `nullifier`".to_string(),


nit: We usually just write the name of the missing field here

SantiagoPittella

LGTM, please address the comment tho.

CHANGELOG.md

bobbinth

Looks good! Thank you! I left a few small comments/questions inline - but overall, I think this is good to merge.

crates/rust-client/src/notes/import.rs

crates/rust-client/src/rpc/mod.rs

crates/rust-client/src/sync/tags.rs

crates/rust-client/src/store/note_record/mod.rs

crates/rust-client/src/notes/import.rs

igamigo

LGTM

tomyrd force-pushed the tomyrd-import-past-notes branch 3 times, most recently from d5efcf9 to 939716e Compare July 26, 2024 18:17

tomyrd marked this pull request as ready for review July 26, 2024 18:24

tomyrd changed the title ~~feat: check if note is consumed for NoteFile::NoteId and NoteFile::NoteWith~~ feat: check if note is consumed for NoteFile::NoteId and NoteFile::NoteWithProof Jul 26, 2024

tomyrd mentioned this pull request Jul 26, 2024

Note importing process follow-ups #405

Open

igamigo approved these changes Aug 1, 2024

View reviewed changes

SantiagoPittella approved these changes Aug 2, 2024

View reviewed changes

bobbinth reviewed Aug 3, 2024

View reviewed changes

tomyrd force-pushed the tomyrd-import-past-notes branch from 939716e to 1240721 Compare August 5, 2024 18:14

igamigo approved these changes Aug 7, 2024

View reviewed changes

igamigo requested a review from bobbinth August 8, 2024 18:57

bobbinth requested changes Aug 8, 2024

View reviewed changes

tomyrd mentioned this pull request Aug 9, 2024

refactor: reduce code duplication between tonic and web-tonic #474

Open

SantiagoPittella requested a review from bobbinth August 12, 2024 16:05

SantiagoPittella force-pushed the tomyrd-import-past-notes branch 2 times, most recently from 7d76387 to 0791c27 Compare August 13, 2024 15:05

igamigo mentioned this pull request Aug 13, 2024

feat: import committed notes from older blocks that current #472

Merged

4 tasks

igamigo reviewed Aug 14, 2024

View reviewed changes

tomyrd force-pushed the tomyrd-import-past-notes branch from 2e4100d to 124e543 Compare August 14, 2024 18:17

SantiagoPittella approved these changes Aug 14, 2024

View reviewed changes

CHANGELOG.md Show resolved Hide resolved

bobbinth approved these changes Aug 18, 2024

View reviewed changes

tomyrd added 8 commits August 19, 2024 10:55

feat: update rpc client

1b702d2

feat: improve error message when importing from cli

1036901

feat: check for nullifier when importing note

3fcb4d5

fix: remove unwraps

d58180b

fix: integration tests

8b5f95c

feat: add integration tests

84672dd

update: CHANGELOG

25b992d

fix: change suggestions

09c3111

tomyrd and others added 13 commits August 19, 2024 10:57

feat: implement check_nullifiers_by_prefix in web tonic

82b3a3a

feat: encapsulate nullifier prefix logic

91468dd

refactor: move note import logic to its own file

6558d6b

feat: change block_height to u32

3878493

refactor: rename suggestions

3ca51c8

feat: change created_at to Option<u64>

d536d29

fix: small suggestion changes

c6c6cf2

feat: move get_nullifier_block_num to NodeRpcClient

8db4a44

feat: implement get_tracked_note_tags

2a230d7

feat: improve error message when import note already exists

9c23ea1

update note details with struct

c1d554b

Rename functions

af100a5

fix: error message

9f15ac4

tomyrd force-pushed the tomyrd-import-past-notes branch from 685373f to 9f15ac4 Compare August 19, 2024 14:22

tomyrd mentioned this pull request Aug 19, 2024

feat: make insert_input_note() update the note if it already exists #481

Open

feat: fetch MMR data for the NoteWithProof case

91be2e2

This was referenced Aug 19, 2024

chore: add comment wrapping to format #483

Merged

feat: merge get_tracked_note_tags into a single query #484

Open

feat: NoteWithProof follow ups #485

Open

feat: get the current time inside the Client #486

Open

igamigo approved these changes Aug 21, 2024

View reviewed changes

igamigo merged commit 7a6734e into next Aug 21, 2024
13 checks passed

igamigo deleted the tomyrd-import-past-notes branch August 21, 2024 14:18

tomyrd mentioned this pull request Aug 21, 2024

Refactor import_note in Client #393

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat: check if note is consumed for `NoteFile::NoteId` and `NoteFile::NoteWithProof` #449

feat: check if note is consumed for `NoteFile::NoteId` and `NoteFile::NoteWithProof` #449

tomyrd commented Jul 25, 2024 •

edited

Loading

igamigo left a comment

igamigo Aug 1, 2024

igamigo Aug 1, 2024

igamigo Aug 1, 2024

SantiagoPittella left a comment

bobbinth left a comment

igamigo Aug 7, 2024

tomyrd Aug 8, 2024

igamigo Aug 7, 2024

igamigo Aug 7, 2024

igamigo Aug 7, 2024

igamigo Aug 7, 2024

igamigo Aug 7, 2024

bobbinth left a comment

igamigo Aug 14, 2024

SantiagoPittella left a comment

bobbinth left a comment

igamigo left a comment

feat: check if note is consumed for NoteFile::NoteId and NoteFile::NoteWithProof #449

feat: check if note is consumed for NoteFile::NoteId and NoteFile::NoteWithProof #449

Conversation

tomyrd commented Jul 25, 2024 • edited Loading

igamigo left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

SantiagoPittella left a comment

Choose a reason for hiding this comment

bobbinth left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

bobbinth left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

SantiagoPittella left a comment

Choose a reason for hiding this comment

bobbinth left a comment

Choose a reason for hiding this comment

igamigo left a comment

Choose a reason for hiding this comment

feat: check if note is consumed for `NoteFile::NoteId` and `NoteFile::NoteWithProof` #449

feat: check if note is consumed for `NoteFile::NoteId` and `NoteFile::NoteWithProof` #449

tomyrd commented Jul 25, 2024 •

edited

Loading