feat: add StandardNote constructor from note script#2411
Conversation
PhilippGackstatter
left a comment
There was a problem hiding this comment.
Looks good to me! Left a few small comments.
| pub const PUBLIC: u8 = 0b01; | ||
| pub const PRIVATE: u8 = 0b10; |
There was a problem hiding this comment.
Can we make these associated constants, e.g. NoteType::PUBLIC? I think this is more readable than free-floating constants.
| /// Returns a [`StandardNote`] instance based on the note script of the provided [`Note`]. | ||
| /// Returns `None` if the provided note is not a standard note. | ||
| pub fn from_note(note: &Note) -> Option<Self> { |
There was a problem hiding this comment.
I think we should remove from_note and replace it with from_script_root, or maybe have from_script(&NoteScript) additionally for more type safety. Semantically, we're not converting a full Note into a StandardNote, only its script root / note script, and so passing the full Note is unnecessarily restrictive, which basically prompted this PR. May be easy to do in this PR, but if not, can also be done separately.
| /// Returns the name of this [`StandardNote`] variant as a string. | ||
| pub fn name(&self) -> String { |
There was a problem hiding this comment.
| /// Returns the name of this [`StandardNote`] variant as a string. | |
| pub fn name(&self) -> String { | |
| /// Returns the name of this [`StandardNote`] variant as a string. | |
| pub fn name(&self) -> &'static str { |
Nit: I think we should be able to always return a reference to a string and avoid the extra allocation.
This PR:
StandardNote::from_script_root(root: Word)to identify standard notes directly from a script root, and refactorsfrom_noteto delegate to itStandardNote::name(&self)to return the variant name as a stringNoteTypemasks (PUBLIC,PRIVATE) public and re-exports them frommiden_protocol::note