-
Notifications
You must be signed in to change notification settings - Fork 89
chore: miden base update followups #1563
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
chore: miden base update followups #1563
Conversation
bobbinth
left a comment
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! I left a couple of comments inline - but nothing that's blocking this PR.
I do think we potentially have a bigger issue though that we should address in a follow-up PR:
It seems like we are still relying on note tag to find notes for a network account. Technically, this shouldn't work because network notes shouldn't use tags to encode target accounts (cc @PhilippGackstatter to confirm). Instead, they should be using note attachment data which contains full ID of the target account.
This has a couple of implications:
- First,
select_unconsumed_network_notes_by_tag()should really beselect_unconsumed_network_notes_for_account()and should takeaccount_idas a parameter instead of thetag. - Second, in addition (or maybe instead of)
network_note_type, we probably want to have something liketarget_account_idfield that is populated only for single-target network notes. Then, the query would be filtering against this ID rather than the tag.
| pub(crate) async fn select_unconsumed_network_notes( | ||
| &self, | ||
| network_account_prefix: u32, | ||
| block_num: BlockNumber, | ||
| page: Page, | ||
| ) -> Result<(Vec<NoteRecord>, Page)> { |
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.
This may be a bigger refactoring, and so, probably not for this PR - but we should be using account ID here instead of network_account_prefix. Do we have an issue to change this already? If not, let's create one.
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.
We have #1549 which covers this
Yes, we should not be relying on the note tag at all for network notes anymore and should use the |
As in, this will still work temporarily, or this won't work at all and must be fixed before deploying to testnet? |
Must be fixed before testnet. Relying on the note tag does not work anymore at all. |
642a584 to
12c1ee4
Compare
|
@SantiagoPittella I accidentally pushed to this/your branch (one additional commit), and force pushed back to your original state. Nothing should have changed 🤞 |
In this PR I:
v0.13.1.NetworkAccountTargetErrorfrom miden-base for attachment errors (chore: update with latest miden-base #1526 (comment) & chore: makeNetworkAccountTargetErrorpublic miden-base#2319 ).NetworkNoteTypefor database storage and removeis_single_target_network_note.