Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

feat: Add NoteFile object #721

Merged
merged 11 commits into from
Jun 3, 2024
Merged

feat: Add NoteFile object #721

merged 11 commits into from
Jun 3, 2024

Conversation

tomyrd
Copy link
Collaborator

@tomyrd tomyrd commented May 31, 2024

closes #715

This PR adds a NoteFile object to represent serialized notes.

Copy link
Contributor

@bobbinth bobbinth left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good! Thank you! I left a couple of small comments inline. Once these are addressed, we can merge.

Also, let's update the changelog.

objects/src/notes/file.rs Outdated Show resolved Hide resolved
objects/src/notes/file.rs Outdated Show resolved Hide resolved
@bobbinth
Copy link
Contributor

Oh - one other potential improvement that came to mind: maybe we should implement From<NoteDetails> for NoteFile?

@igamigo
Copy link
Collaborator

igamigo commented May 31, 2024

If not too much of an issue we might want to test serialization as well, but it could come later.

@bobbinth
Copy link
Contributor

Based on the discussion in 0xPolygonMiden/miden-client#354 (comment), I wonder if we should add NoteId variant here (and add From<NoteId> implementation).

@tomyrd
Copy link
Collaborator Author

tomyrd commented May 31, 2024

Just read the issue discussion and had a question about this new variant. Is the idea to just serialize the NoteId? Something like NoteFile::IdOnly(NoteId)?

If that's the case, when would the rest of the information be filled in on the client? When importing or after a sync?

@bobbinth
Copy link
Contributor

Just read the issue discussion and had a question about this new variant. Is the idea to just serialize the NoteId? Something like NoteFile::IdOnly(NoteId)?

Yes, but I'd probably call the variant NoteId (and maybe then it makes sense to rename DetailsOnly to NoteDetails).

If that's the case, when would the rest of the information be filled in on the client? When importing or after a sync?

In this case, I'm thinking the client would make a request to the node to get the details, and if nothing comes back return an error.

Copy link
Contributor

@bobbinth bobbinth left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good! Thank you! I left a couple more small comments inline. Also, adding a simple test serialization/deserialization may be good.

Lastly, let's resolve the merge conflict for changelog.

objects/src/notes/file.rs Outdated Show resolved Hide resolved
objects/src/notes/file.rs Show resolved Hide resolved
Copy link
Contributor

@bobbinth bobbinth left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

All looks good! Thank you!

@bobbinth bobbinth merged commit 8cd8eb3 into next Jun 3, 2024
9 checks passed
@bobbinth bobbinth deleted the tomyrd-note-serialization branch June 3, 2024 17:30
bobbinth pushed a commit that referenced this pull request Jul 4, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Add type for serialized notes
3 participants