feat: add .is_network_note() helper method for Note#2365
feat: add .is_network_note() helper method for Note#2365PhilippGackstatter merged 14 commits intonextfrom
.is_network_note() helper method for Note#2365Conversation
3b1b042 to
c1aea73
Compare
There was a problem hiding this comment.
Pull request overview
This PR introduces a new AccountTargetNetworkNote type and NetworkNoteExt trait to make it easier to determine if a Note is a network note and to access network-specific properties. This addresses issue #2362 by providing a cleaner, more discoverable API compared to the previous approach of using NetworkAccountTarget::try_from(note.metadata().attachment()).
Changes:
- Added
AccountTargetNetworkNote<'a>struct providing a view over notes withNetworkAccountTargetattachments - Added
NetworkNoteExttrait withis_network_note()andas_account_target_network_note()convenience methods - Added integration test
test_network_note_unwrap()verifying the new API works correctly
Reviewed changes
Copilot reviewed 4 out of 4 changed files in this pull request and generated 4 comments.
| File | Description |
|---|---|
| crates/miden-standards/src/note/network_note.rs | New module defining AccountTargetNetworkNote struct and NetworkNoteExt trait for working with network notes |
| crates/miden-standards/src/note/mod.rs | Exports the new AccountTargetNetworkNote and NetworkNoteExt types |
| crates/miden-testing/src/kernel_tests/tx/test_output_note.rs | Adds integration test verifying the new network note API |
| CHANGELOG.md | Documents the addition of the NetworkNote type in release 0.14.0 |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
NetworkNote newtype.is_network_note() helper method for Note
PhilippGackstatter
left a comment
There was a problem hiding this comment.
Looks good to me! Sorry for the delay!
Left a small comment re usability of this type in the node. I think copilot makes some good points as well.
| pub struct AccountTargetNetworkNote<'a> { | ||
| note: &'a Note, |
There was a problem hiding this comment.
I'd probably make this own the underlying note and provide unwrap functionality like AccountTargetNetworkNote::into_note(self) -> Note and AccountTargetNetworkNote::as_note(&self) -> &Note. NetworkNoteExt::as_account_target_network_note would become into_*.
I think without this the node would probably not be able to use the wrapper (and maybe that's fine?) cc @Mirko-von-Leipzig @sergerad .
There was a problem hiding this comment.
Agreed - I think wrapping a full note (rather than just a reference) would make this more flexible. AFAIK, it most relevant contexts, we'd be converting a Note into AccountTargetNetworkNote. But also, if that's not the case, cloning here wouldn't be too bad either (and if we do want to avoid cloning, using Arc could be a better option).
bobbinth
left a comment
There was a problem hiding this comment.
Looks good! Thank you! I left a couple of comments inline.
| pub struct AccountTargetNetworkNote<'a> { | ||
| note: &'a Note, |
There was a problem hiding this comment.
Agreed - I think wrapping a full note (rather than just a reference) would make this more flexible. AFAIK, it most relevant contexts, we'd be converting a Note into AccountTargetNetworkNote. But also, if that's not the case, cloning here wouldn't be too bad either (and if we do want to avoid cloning, using Arc could be a better option).
| /// other types of network notes may exist (e.g., SWAP notes that can be consumed by network | ||
| /// accounts but are not targeted at a specific one). | ||
| #[derive(Clone, Debug, PartialEq, Eq)] | ||
| pub struct AccountTargetNetworkNote<'a> { |
There was a problem hiding this comment.
I don't love the name AccountTargetNetworkNote, but also don't have much better suggestions. Maybe something like AccountBoundNetworkNote?
There was a problem hiding this comment.
Do you think SingleTargetNetworkNote is a better name? As @PhilippGackstatter wrote above, we will later on have network notes which could theoretically be filled by multiple network accounts (although not sure how that would work, but that is a separate discussion).
|
@PhilippGackstatter @bobbinth I addressed all of your comments: Although already approved, would appreciate one more quick round of review to verify updates, and to think about potentially a better name for If everything looks good, feel free to merge! |
bobbinth
left a comment
There was a problem hiding this comment.
All looks good! Thank you!
I do slightly prefer AccountBoundNetworkNote - but I wouldn't make the change in this PR. Let's open an issue to discuss potential name changes.
PhilippGackstatter
left a comment
There was a problem hiding this comment.
Thank you, looks great!
This PR adds a new
NetworkNotetype which makes it easy to determine if aNoteis a network note or not.Resolves: #2362