refactor: split input output note register structs #199

mFragaBA · 2024-03-04T13:17:44Z

addresses #197. This PR achieves 2 things:

I suggest checking this PR in 3 steps:

Check the files sqlite_store/notes.rs, store/mod.rs, store/store.sql
- first check that all input/output note functions and structs have their own implementation and are not shared between the two
- then see the changes of the inner structure of XNoteRecord
Lastly check the other files as almost all (if not all) changes are made to accomodate to the new note record interface

Summary of changes

It splits the InputNoteRecord into InputNoteRecord and OutputNoteRecord structs. While they store similar fields, they support slightly different fields and as discussed on Split/Refactor NoteRecord related logic to handle possible flows #197, this approach would be more flexible to refactor in the future. As a consecuence, we now have separate:
- SerializedInputNoteData and SerializedOutputNoteData
- SerializedInputNoteParts and SerializedOutputNoteParts
- parse_input_note_columns and parse_output_columns functions
- parse_input_note and parse_output_note functions
- serialize_input_note and serialize_output_note functions
For both XNoteRecord structs, we replace the Note with its fields as some of them might not be known depending on the flow. As a consecuence:
- For some fields we stored String or Vec<u8> since the actual type does not implement serde's Serialize and Deserialize traits. We added the necessary casting. It should be discussed whether we want the interface of the struct to return these String or Vec or we want to return the proper types (perhaps we can keep it as is but add From implementations).
- The serialize_X_note, parse_X_note_column and parse_X_note functions are now slightly different since we need to adjust to the new note record fields
- Uses of InputNoteRecord in the client and the cli had to be accordingly updated
- I also added a Struct for the note status NoteStatus. While it's quite declarative, i don't like that some queries now do for example INSERT ... status = '"Committed"' ... though to account for serde's way of serializing the enum as a json string. Any suggestions are more than welcome

…ragaBA-split-input-notes-table

This reverts commit bf46c0a.

…ragaBA-split-input-notes-table

I still have pending polishing the changes (abstracting some deserialization and removing debugs, etc.) and we should also add some kind of validation as well as having the proper NULL / NOT NULL columns (now everything is nullable)

…ragaBA-split-input-notes-table

…onMiden/miden-client into mFragaBA-serialize-fields-as-json

There were two different kind of errors: * When we queried for less fields than what parse_input_notes_columns expected (this happened on the function to get peding input note nullifiers) * When we serialized the note status. Apparently serde serializes the note as `\"Variant\"` instead of `variant`.

…ragaBA-serialize-fields-as-json

…gonMiden/miden-client into mFragaBA-split-input-output-note-structs

…ragaBA-serialize-fields-as-json

…gonMiden/miden-client into mFragaBA-split-input-output-note-structs

…ragaBA-serialize-fields-as-json

…gonMiden/miden-client into mFragaBA-split-input-output-note-structs

…ragaBA-split-input-output-note-structs

igamigo

LGTM! I left a couple of comments and questions. I think with the differentiation now of how input notes and output notes are treated, we might want to make sure we have tests covering the new code.

igamigo · 2024-03-13T20:14:46Z

src/store/mod.rs

+        match value {
+            0 => NoteStatus::Pending,
+            1 => NoteStatus::Committed,
+            _ => NoteStatus::Consumed,


Perhaps this should be a TryFrom implementation? I think it's worth it to match only for 2 here, and error out if it's something else.

Done! I used the existing DeserializationError for its error type

igamigo · 2024-03-13T20:16:07Z

src/store/mod.rs

    inclusion_proof: Option<NoteInclusionProof>,
+    metadata: Option<NoteMetadata>,
+    recipient: Digest,
+    status: NoteStatus,
 }

 impl InputNoteRecord {


Since these structs are getting large with the impl blocks, should we move them to their own files?

I like the idea!

change made on 21fc72e

igamigo · 2024-03-13T20:19:14Z

src/store/mod.rs

+                );
+                Ok(InputNote::new(note, proof.clone()))
+            }
+            // TODO: should we use a better Error for these two?


I think these errors look OK. An alternative could be to make it a specific variant for ClientError, since the problem might be indicate that the client has not synced.

the message within the string is OK, but the error variant InvalidOriginIndex is what seems off to me. But perhaps for now it's enough

igamigo · 2024-03-13T20:22:41Z

src/store/sqlite_store/notes.rs

+    let note_metadata: Option<NoteMetadata> = if let Some(metadata_as_json_str) = note_metadata {
+        Some(
+            serde_json::from_str(&metadata_as_json_str)
+                .map_err(StoreError::JsonDataDeserializationError)?,
+        )
+    } else {
+        None
+    };


I think this could be written a little bit more succinctly like this:

let note_metadata: Option<NoteMetadata> = note_metadata.map(serde_json::from_str...)

I did that at first but the problem is that I wouldn't be able to use the ? to propagate the error

igamigo · 2024-03-13T20:25:19Z

src/store/sqlite_store/notes.rs

+            // FIXME: This removal is to accomodate a problem with how the node constructs paths where
+            // they are constructed using note ID instead of authentication hash, so for now we remove the first
+            // node here.
+            //
+            // Note: once removed we can also stop creating a new `NoteInclusionProof`
+            //
+            // See: https://github.com/0xPolygonMiden/miden-node/blob/main/store/src/state.rs#L274


@bobbinth we have been dragging this FIXME for a while now, is this getting tracked somewhere? I recall it being mentioned a couple of times but I just quickly searched for it in related issues and did not find anything

It is supposed to be a part of 0xPolygonMiden/miden-base#464 - but I haven't been able to finish it yet.

src/store/sqlite_store/notes.rs

src/store/sqlite_store/store.sql

…ragaBA-split-input-output-note-structs

igamigo

LGTM!

* split input notes * make enum for notes tables * rename recipients variable and add more NoteTable uses * run fmt * change to_string for to_hex * correct nullable fields and reorder fields * remove unnecesarry note filter * remove unnecessary commit_height field * move related rows into metadata row for notes * remove dbg statements * switch to_string for to_hex * Revert "move related rows into metadata row for notes" This reverts commit bf46c0a. * restore filtering by pending notes * store grouped json fields * add json fields with serialization/deserialization I still have pending polishing the changes (abstracting some deserialization and removing debugs, etc.) and we should also add some kind of validation as well as having the proper NULL / NOT NULL columns (now everything is nullable) * remove dbg calls and set nullable/non-nullable cols accordingly * apply clippy suggestion * add helper functions to work with json values returned as strings * add documentation for handling json with sqlite * fix incorrect function use to fetch output notes * ignore doc tests * add json validity constraints * use structs to represent json columns * remove dbg statements * use propper serialization * use existing NoteMetadata instead of NoteRecordMetadata * also remove NoteRecordInclusionProof and use NoteInclusionProof instead * add implementations for output notes * change input note record struct TODO: fix tests * Fix test failures There were two different kind of errors: * When we queried for less fields than what parse_input_notes_columns expected (this happened on the function to get peding input note nullifiers) * When we serialized the note status. Apparently serde serializes the note as `\"Variant\"` instead of `variant`. * update doc comments * cleanup struct * remove dbg statements * fix serialization for output notes * change from to tryfrom for notestatus * move note record structs to its own module * remove todo comment * remove quotes from note status

mFragaBA and others added 30 commits January 29, 2024 18:15

split input notes

e86d00e

make enum for notes tables

550ba44

rename recipients variable and add more NoteTable uses

6cf5d30

Merge branch 'main' of github.com:0xPolygonMiden/miden-client into mF…

3cd914e

…ragaBA-split-input-notes-table

run fmt

0d548d9

Merge branch 'main' of github.com:0xPolygonMiden/miden-client into mF…

e589a31

…ragaBA-split-input-notes-table

change to_string for to_hex

b65fbd5

correct nullable fields and reorder fields

e9336db

remove unnecesarry note filter

3c8567d

remove unnecessary commit_height field

3fa58c7

move related rows into metadata row for notes

bf46c0a

Merge branch 'main' of github.com:0xPolygonMiden/miden-client into mF…

3ca4b43

…ragaBA-split-input-notes-table

Merge branch 'main' of github.com:0xPolygonMiden/miden-client into mF…

287a8e0

…ragaBA-split-input-notes-table

remove dbg statements

73b5bbc

switch to_string for to_hex

d4485d2

Revert "move related rows into metadata row for notes"

b01acf2

This reverts commit bf46c0a.

Merge branch 'main' of github.com:0xPolygonMiden/miden-client into mF…

d91569c

…ragaBA-split-input-notes-table

restore filtering by pending notes

91cbe66

Merge branch 'main' of github.com:0xPolygonMiden/miden-client into mF…

c7f98d1

…ragaBA-split-input-notes-table

Merge branch 'main' of github.com:0xPolygonMiden/miden-client into mF…

1e021ce

…ragaBA-split-input-notes-table

Merge branch 'main' of github.com:0xPolygonMiden/miden-client into mF…

dcfe797

…ragaBA-split-input-notes-table

store grouped json fields

e43d379

add json fields with serialization/deserialization

c2d7dc9

I still have pending polishing the changes (abstracting some deserialization and removing debugs, etc.) and we should also add some kind of validation as well as having the proper NULL / NOT NULL columns (now everything is nullable)

remove dbg calls and set nullable/non-nullable cols accordingly

d5e1dd3

Merge branch 'main' of github.com:0xPolygonMiden/miden-client into mF…

bc49e8c

…ragaBA-split-input-notes-table

Merge branch 'mFragaBA-split-input-notes-table' of github.com:0xPolyg…

ed8e965

…onMiden/miden-client into mFragaBA-serialize-fields-as-json

apply clippy suggestion

8686b66

add helper functions to work with json values returned as strings

2f94ba7

add documentation for handling json with sqlite

21ae1d6

fix incorrect function use to fetch output notes

6bf494d

mFragaBA changed the base branch from main to mFragaBA-serialize-fields-as-json March 5, 2024 13:47

mFragaBA changed the title ~~refactor: split input output note structs~~ refactor: split input output note register structs Mar 5, 2024

mFragaBA added 7 commits March 5, 2024 12:20

update doc comments

15fd102

cleanup struct

051fa0a

remove dbg statements

8cae5d3

Merge branch 'main' of github.com:0xPolygonMiden/miden-client into mF…

36235ee

…ragaBA-serialize-fields-as-json

Merge branch 'mFragaBA-serialize-fields-as-json' of github.com:0xPoly…

a1cb379

…gonMiden/miden-client into mFragaBA-split-input-output-note-structs

Merge branch 'main' of github.com:0xPolygonMiden/miden-client into mF…

eec0161

…ragaBA-serialize-fields-as-json

Merge branch 'mFragaBA-serialize-fields-as-json' of github.com:0xPoly…

85ec9f2

…gonMiden/miden-client into mFragaBA-split-input-output-note-structs

mFragaBA marked this pull request as ready for review March 5, 2024 18:30

mFragaBA added 3 commits March 6, 2024 16:37

Merge branch 'main' of github.com:0xPolygonMiden/miden-client into mF…

0a1554b

…ragaBA-serialize-fields-as-json

Merge branch 'mFragaBA-serialize-fields-as-json' of github.com:0xPoly…

6f8ccb7

…gonMiden/miden-client into mFragaBA-split-input-output-note-structs

fix serialization for output notes

e3a6c1f

Base automatically changed from mFragaBA-serialize-fields-as-json to main March 13, 2024 15:12

Merge branch 'main' of github.com:0xPolygonMiden/miden-client into mF…

b5574b1

…ragaBA-split-input-output-note-structs

mFragaBA force-pushed the mFragaBA-split-input-output-note-structs branch from 9626b2f to b5574b1 Compare March 13, 2024 16:46

igamigo approved these changes Mar 13, 2024

View reviewed changes

mFragaBA added 5 commits March 14, 2024 09:56

change from to tryfrom for notestatus

1c6ea50

move note record structs to its own module

21fc72e

remove todo comment

cff949d

remove quotes from note status

ac94858

Merge branch 'main' of github.com:0xPolygonMiden/miden-client into mF…

f20798d

…ragaBA-split-input-output-note-structs

bobbinth changed the base branch from main to next March 15, 2024 18:36

Merge branch 'main' of github.com:0xPolygonMiden/miden-client into mF…

a7d4636

…ragaBA-split-input-output-note-structs

igamigo approved these changes Mar 15, 2024

View reviewed changes

igamigo merged commit f05a3f8 into next Mar 15, 2024
6 checks passed

igamigo deleted the mFragaBA-split-input-output-note-structs branch March 15, 2024 20:27

igamigo mentioned this pull request Mar 18, 2024

Split/Refactor NoteRecord related logic to handle possible flows #197

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

refactor: split input output note register structs #199

refactor: split input output note register structs #199

mFragaBA commented Mar 4, 2024 •

edited

Loading

igamigo left a comment

igamigo Mar 13, 2024

mFragaBA Mar 14, 2024

igamigo Mar 13, 2024

mFragaBA Mar 14, 2024

mFragaBA Mar 14, 2024

igamigo Mar 13, 2024

mFragaBA Mar 14, 2024

igamigo Mar 13, 2024

mFragaBA Mar 13, 2024

igamigo Mar 13, 2024

bobbinth Mar 13, 2024

igamigo left a comment

refactor: split input output note register structs #199

refactor: split input output note register structs #199

Conversation

mFragaBA commented Mar 4, 2024 • edited Loading

Summary of changes

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

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

mFragaBA commented Mar 4, 2024 •

edited

Loading