feat: verify imported note existance in the chain #297

tomyrd · 2024-04-25T17:08:18Z

closes #277

Added the option to verify a note when importing it to the client. If the verification passes we try to add an inclusion proof to the imported note if possible.

Also we now check for local uncommited notes in every sync. So if the note was imported but it's not relevant for tracked account it can also be updated.

mFragaBA

Not a final review

mFragaBA · 2024-04-26T13:08:44Z

CHANGELOG.md

@@ -1,5 +1,6 @@
 # Changelog

+* Added an option to verify note existance in the chain before importing.


Suggested change

* Added an option to verify note existance in the chain before importing.

* Added an option to verify note existence in the chain before importing.

Nice catch. The error name was also wrong so I fixed it everywhere.

src/cli/input_notes.rs

mFragaBA · 2024-04-26T13:14:23Z

src/client/notes.rs

@@ -29,7 +29,57 @@ impl<N: NodeRpcClient, R: FeltRng, S: Store> Client<N, R, S> {
    // --------------------------------------------------------------------------------------------

    /// Imports a new input note into the client's store.
-    pub fn import_input_note(&mut self, note: InputNoteRecord) -> Result<(), ClientError> {
+    pub async fn import_input_note(


The method doc comment should Also mention posible errors and explain the verify flag

mFragaBA · 2024-04-26T13:30:04Z

src/client/sync.rs

+        let uncommited_note_tags: Vec<NoteTag> = self
+            .store
+            .get_input_notes(NoteFilter::Pending)?
+            .iter()
+            .filter(|note| note.metadata().is_some())
+            .map(|note| note.metadata().expect("Notes should have metadata after filter").tag())
+            .collect();
+
+        let note_tags = [account_note_tags, stored_note_tags, uncommited_note_tags].concat();


I'm wondering whether we should pass this through a map to remove possible duplicates

Added a temporary fix using HashMap. Will change it to BTreeSet when NoteTag implements Ord.

src/client/sync.rs

src/client/notes.rs

mFragaBA · 2024-04-26T18:22:48Z

tests/integration/main.rs

+#[tokio::test]
+async fn test_import_pending_notes() {


I think we should also try consuming the created notes to make sure the inclusion proofs paths are usable

Done. Added a line that consumes the note. The test doesn't panic so the inclusion proof is usable.

igamigo

LGTM. Left a couple comments that should be simple to address

src/client/notes.rs

igamigo · 2024-05-02T19:42:15Z

src/client/sync.rs

@@ -1,4 +1,4 @@
-use alloc::collections::BTreeSet;
+use std::collections::{BTreeSet, HashMap};


This collections (BTreeSet and BTreeMap) should come from alloc to not depend on std types.

Co-authored-by: igamigo <[email protected]>

mFragaBA · 2024-05-03T18:47:30Z

src/client/sync.rs

+        //TODO: Use BTreeSet to remove duplicates more efficiently once `Ord` is implemented for `NoteTag`
+        let note_tags: Vec<NoteTag> = [account_note_tags, stored_note_tags, uncommited_note_tags]
+            .concat()
+            .into_iter()
+            .map(|tag| (tag.to_string(), tag))
+            .collect::<HashMap<String, NoteTag>>()
+            .values()
+            .cloned()
+            .collect();


now that the related PR from base got merged, will we do this in a follow-up PR?

as discussed offline, we'll do this on the branch that we'll use as a base for the release (that will point to base's next)

Added an issue for this change (#320).

tests/integration/main.rs

docs/cli-reference.md

Co-authored-by: Martin Fraga <[email protected]>

mFragaBA

LGTM! Tested locally, no issues found.

* feat: export notes from output notes table (converted to input notes) * refactor: rewrite test to display new behavior * Add note details command when importing note * Add account details command when creating new account * Erase TODO, fixed in PR #297 * Add more feedback to `tx new` command * feat: search both input and output notes when looking for notes * refactor: rename input_notes file to notes * Add more information to `SyncStatus` * Add tests for new sync details * Add changes to CHANGELOG * Rename `SyncNewDetails` and add docs * address review comments * fix: handle corner cases on show command * Fix suggestions * Remove unnecessary message * Move binary name to constant * Move binary name inline * Change `add` to `combine_with` * Change transaction output notes message --------- Co-authored-by: Martin Fraga <[email protected]>

* Add verify flag to import note command * Add inclusion proof to note if possible * Include uncommited notes tags to sync * Add tests for * Add change to CHANGELOG * Fix typo in error name * Change flag to be `no_verify` instead of `verify` * Improve doc comment for `import_input_note` * Remove note tag duplicates before request * Combine `filter` and `map` for uncommited note tags * Add explanation for inclusion proof check * Consume note in integration test to check proof validity * Fix typo in documentation Co-authored-by: igamigo <[email protected]> * Use BTreeSet from alloc * Remove git artifacts * Fix cli-reference doc Co-authored-by: Martin Fraga <[email protected]> * Fix comment in testing Co-authored-by: Martin Fraga <[email protected]> --------- Co-authored-by: igamigo <[email protected]> Co-authored-by: Martin Fraga <[email protected]>

* feat: export notes from output notes table (converted to input notes) * refactor: rewrite test to display new behavior * Add note details command when importing note * Add account details command when creating new account * Erase TODO, fixed in PR #297 * Add more feedback to `tx new` command * feat: search both input and output notes when looking for notes * refactor: rename input_notes file to notes * Add more information to `SyncStatus` * Add tests for new sync details * Add changes to CHANGELOG * Rename `SyncNewDetails` and add docs * address review comments * fix: handle corner cases on show command * Fix suggestions * Remove unnecessary message * Move binary name to constant * Move binary name inline * Change `add` to `combine_with` * Change transaction output notes message --------- Co-authored-by: Martin Fraga <[email protected]>

tomyrd added 4 commits April 25, 2024 10:28

Add verify flag to import note command

01aa3e2

Add inclusion proof to note if possible

d8261e1

Include uncommited notes tags to sync

b89a8b7

Add tests for

7514da0

tomyrd linked an issue Apr 25, 2024 that may be closed by this pull request

Verify a note exists when importing #277

Closed

Add change to CHANGELOG

a40e7e1

tomyrd marked this pull request as ready for review April 25, 2024 17:17

mFragaBA reviewed Apr 26, 2024

View reviewed changes

mFragaBA suggested changes Apr 26, 2024

View reviewed changes

tomyrd added 6 commits April 29, 2024 14:21

Fix typo in error name

0b13bda

Change flag to be no_verify instead of verify

7c3c239

Improve doc comment for import_input_note

e5d8193

Remove note tag duplicates before request

1aca550

Combine filter and map for uncommited note tags

210b861

Add explanation for inclusion proof check

220c80d

tomyrd mentioned this pull request Apr 29, 2024

Get block authentication information for past blocks #306

Closed

Consume note in integration test to check proof validity

8a24a67

tomyrd force-pushed the tomyrd-import-note-existance-check branch from 29d8800 to 8a24a67 Compare April 29, 2024 21:27

Merge branch 'next' into tomyrd-import-note-existance-check

153b5ad

tomyrd requested a review from mFragaBA April 29, 2024 21:33

Merge branch 'next' into tomyrd-import-note-existance-check

c85068a

igamigo approved these changes May 2, 2024

View reviewed changes

tomyrd and others added 4 commits May 3, 2024 11:14

Fix typo in documentation

590ad66

Co-authored-by: igamigo <[email protected]>

Use BTreeSet from alloc

eed7383

Merge branch 'next' into tomyrd-import-note-existance-check

fb42d29

Remove git artifacts

9c5b4f8

mFragaBA reviewed May 3, 2024

View reviewed changes

tests/integration/main.rs Outdated Show resolved Hide resolved

mFragaBA reviewed May 3, 2024

View reviewed changes

docs/cli-reference.md Outdated Show resolved Hide resolved

tomyrd mentioned this pull request May 3, 2024

Use BTreeSet to check for duplicate note tags #320

Closed

tomyrd and others added 2 commits May 3, 2024 16:05

Fix cli-reference doc

f306b8b

Co-authored-by: Martin Fraga <[email protected]>

Fix comment in testing

ac1008d

Co-authored-by: Martin Fraga <[email protected]>

mFragaBA approved these changes May 3, 2024

View reviewed changes

igamigo merged commit 99c1721 into next May 3, 2024
6 checks passed

igamigo deleted the tomyrd-import-note-existance-check branch May 3, 2024 19:23

mFragaBA mentioned this pull request May 3, 2024

Verify a note exists when importing #277

Closed

tomyrd added a commit that referenced this pull request May 6, 2024

Erase TODO, fixed in PR #297

b716947

tomyrd mentioned this pull request Jun 4, 2024

Implement NoteFile importing/exporting #371

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat: verify imported note existance in the chain #297

feat: verify imported note existance in the chain #297

tomyrd commented Apr 25, 2024

mFragaBA left a comment

mFragaBA Apr 26, 2024

tomyrd Apr 29, 2024

mFragaBA Apr 26, 2024

tomyrd Apr 29, 2024

mFragaBA Apr 26, 2024

tomyrd Apr 29, 2024

mFragaBA Apr 26, 2024

tomyrd Apr 29, 2024

igamigo left a comment

igamigo May 2, 2024

tomyrd May 3, 2024

mFragaBA May 3, 2024

mFragaBA May 3, 2024

tomyrd May 3, 2024

mFragaBA left a comment

		@@ -1,5 +1,6 @@
		# Changelog

		* Added an option to verify note existance in the chain before importing.

	* Added an option to verify note existance in the chain before importing.
	* Added an option to verify note existence in the chain before importing.

		@@ -1,4 +1,4 @@
		use alloc::collections::BTreeSet;
		use std::collections::{BTreeSet, HashMap};

feat: verify imported note existance in the chain #297

feat: verify imported note existance in the chain #297

Conversation

tomyrd commented Apr 25, 2024

mFragaBA 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

Choose a reason for hiding this comment

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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mFragaBA left a comment

Choose a reason for hiding this comment