-
Notifications
You must be signed in to change notification settings - Fork 40
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: Implement NoteMetadata
protobuf message and NoteType
enum
#338
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thank you! Looks good! I left a few comments inline. Most are minor - but three is one in which I wonder if using enum for note type was actually a good idea.
crates/proto/proto/note.proto
Outdated
enum NoteType { | ||
PUBLIC = 0; | ||
OFF_CHAIN = 1; | ||
ENCRYPTED = 2; | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Should these values be aligned with values here. If so, I wonder if there is a good way to keep them consistent (maybe using enum here was not such a good idea?).
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yeah, not sure about this. There cannot be protobuf enums that don't start with 0 so it's either creating a phantom variant, creating a custom mapping function (from protobuf's note type to base note type), or just removing it altogether and using a fixed32
. For making sure they stay synced I can add a test that asserts this.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
On second thought it's not worth the added complexity if we can just convert the note type number. Removed in favor of a fixed32
.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Agreed! I came to the same conclusion. The only question - should we use int32
instead? (this way, encoding would take 1 byte instead of 4, but otherwise should be the same).
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Changed to uint32
. Seemed similar in terms of encoding low values but marginally more correct for the use case.
There was a problem hiding this 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! In addition to the small comment I left inline, I think we should change the type of note_type
in protobuf message from fixed32
to int32
.
Closes #315
Closes #314