chore: update with latest miden-base #1526
Conversation
SantiagoPittella
requested review from
Mirko-von-Leipzig and
PhilippGackstatter
PhilippGackstatter
left a comment
PhilippGackstatter
left a comment
There was a problem hiding this comment.
Looks good - left a few comments.
PhilippGackstatter
left a comment
PhilippGackstatter
left a comment
There was a problem hiding this comment.
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.
| #[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.
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 |
SantiagoPittella
restored the
santiagopittella-update-protocol-version
branch
This reverts commit 5478bad.
| /// Cache of events received from the mempool that predate corresponding network accounts. | ||
| /// Grouped by account prefix to allow targeted event delivery to actors upon creation. | ||
| predating_events: HashMap<NetworkAccountPrefix, IndexMap<TransactionId, Arc<MempoolEvent>>>, | ||
| predating_events: HashMap<NetworkAccountId, IndexMap<TransactionId, Arc<MempoolEvent>>>, |
There was a problem hiding this comment.
We should go through comments/names etc. to make sure we no longer use "prefix" terminology.
| note_type INTEGER NOT NULL, -- 1-Public (0b01), 2-Private (0b10), 3-Encrypted (0b11) | ||
| sender BLOB NOT NULL, | ||
| tag INTEGER NOT NULL, | ||
| is_single_target_network_note INTEGER NOT NULL, -- 1 if note has NetworkAccountTarget attachment, 0 otherwise |
There was a problem hiding this comment.
I would maybe replace this with something like network_note_type or network_note_group and use the following value:
0- not a network note (same as now).1- single account target network note (same as now).
In the future, this would allow us to to extend this to other type of network notes.
There was a problem hiding this comment.
We could also repurpose note_type I think so that we have:
enum NoteType {
Private,
/// Public, non-network
Public,
/// Public, single-target network
SingleTargetNetwork,
...
}or will these not remain orthogonal?
There was a problem hiding this comment.
Possible, but may complicate note metadata construction logic because we'd need to convert SingleTargetNetwork to Public.
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)