-
Notifications
You must be signed in to change notification settings - Fork 89
chore: update with latest miden-base #1526
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: update with latest miden-base #1526
Conversation
PhilippGackstatter
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 - left a few comments.
PhilippGackstatter
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 to me from a protocol perspective.
I think the network account ID should eventually contain at least 64 or more bits of the account ID - ideally the full one. Anything less than 64 is not guaranteed to be unique, but probably not critical to do before 0.13 testnet.
| #[error("note does not have a valid NetworkAccountTarget attachment: {0}")] | ||
| InvalidAttachment(String), |
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.
| #[error("note does not have a valid NetworkAccountTarget attachment: {0}")] | |
| InvalidAttachment(String), | |
| #[error("note does not have a valid NetworkAccountTarget attachment: {0}")] | |
| InvalidAttachment(#[source] NetworkAccountTargetError), |
Nit: Should this use asource error instead of stringifying it?
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.
NetworkAccountTargetError is not publicly exposed. In miden-base we have:
mod network_account_target;
pub use network_account_target::NetworkAccountTarget;
Should we make the whole module public?
|
@SantiagoPittella if you could mark comments as |
Sure! I usually let the reviewer to decide that. I marked the comments that I consider addressed. There is one remaining #1526 (comment) |
I've gone back and forth and this myself as well. I should probably right some thoughts down at some point a bit more formally :D |
This reverts commit 5478bad.
closes #1522
THis PR update with the latest miden-base changing, affecting primary the network note detection.
I wanted to test this with the network monitor and its counter increment network transaction, but I started to receive the same error that I mentioned in #1490:
Update: fixed it by making
get_vault_asset_witnessesreturn an empty vector as suggested in #1493 (comment)