feat: add miden-objects with canonical Protobuf representations - #3707
Conversation
6114646 to
0e98371
Compare
0e98371 to
5db9107
Compare
| message AccountHeader { | ||
| AccountId account_id = 1; | ||
| primitives.Digest vault_root = 2; | ||
| primitives.Digest storage_commitment = 3; | ||
| primitives.Digest code_commitment = 4; | ||
| uint64 nonce = 5; | ||
| } |
There was a problem hiding this comment.
Would this also need to have the AccountHeader::VERSION_1? It is private for now, but we can make it public, or even make it a field of the Rust struct AccountHeader it it helps.
If so, the same probably applies to PartialNoteMetadata, AssetId and BlockHeader.
There was a problem hiding this comment.
What's the use case for the version? Is it accounted for in the commitment?
Protobuf messages can be extended with new fields without breaking compatibility, so some changes can be modeled in Protobuf without having that explicit version number.
Semantic changes to these types might require a more complicated approach though. Even in that case I don't think adding a version field to this message would make sense. I'd rather do something like this instead:
message AccountHeader {
oneof version {
AccountHeaderV0 v0 = 1;
AccountHeaderV1 v1 = 2;
}
}
message AccountHeaderV0 {
// exact representation of V0
}
message AccountHeaderV1 {
// exact representation of V1
}There was a problem hiding this comment.
What's the use case for the version? Is it accounted for in the commitment?
The purpose is to indicate changes to the structure (what protobuf seems to handle already). Presumably only field additions, but can't say for sure. It's really hard to predict how things will evolve. Basically, I don't know of any planned changes to the account header, but that doesn't mean there won't ever be any.
The version is committed to, yes.
There was a problem hiding this comment.
@Mirko-von-Leipzig do you think the oneof above would be better here? It's definitely more future-proof but adds some complexity to the protocol.
There was a problem hiding this comment.
I would almost always opt for oneof. Doubly-so if we are uncertain about what kind of change we are introducing.
One alternative is that we can separate the account struct version from the schema version, but that seems like a bad idea.
There was a problem hiding this comment.
One thing I do want to clarify is that we are not versioning just the account header here - we are versioning a full account (this is why I think PartialAccount should also get a version). This is a bit more clear for NoteMetadata - there too we are versioning the full note - not just the metadata field. So, if we do want to keep the oneof, it would have to be at the Note level rather than the NoteMetadata level.
Yep, that's a good point: top-level objects should be versioned. That also means that in the case of notes we should version both Note and NoteHeader (because we're using NoteHeader for output notes in transactions).
That's probably the best argument against the oneof representation?
There was a problem hiding this comment.
I think NoteHeader is versioned implicitly because it contains NoteMetadata. But yeah, expressing this relationship with oneof may be challenging.
There was a problem hiding this comment.
I think my fear with the version field is that the flat message gradually becomes a union of every version’s fields. Requiredness and valid field combinations then depend on the version, so the schema itself permits invalid cross-version combinations.
Conversion would also need to support practically all versions if we're to use the protobuf representations in long-term storage.
There was a problem hiding this comment.
Yes, agreed that this is a concern. My main hope is that the number of versions will be pretty small and we won't end up with too many invalid combinations (if any). And if we do end up with some, we'd be able to handle them at conversion time.
There was a problem hiding this comment.
OK, I've removed the oneof messages and have added version fields instead.
PhilippGackstatter
left a comment
There was a problem hiding this comment.
Only a partial review so far, but left a few suggestions. Overall looking good!
| fn from(value: &AccountWitness) -> Self { | ||
| Self { | ||
| account_id: Some(value.id().into()), | ||
| witness_id: Some(value.id().into()), | ||
| commitment: Some(value.state_commitment().into()), | ||
| path: Some(value.path().clone().into()), |
There was a problem hiding this comment.
nit: could we replace all the values with more precise names, e.g. witness? This makes things easier to read and an LLM does it in no time.
e436244 to
7b51099
Compare
7b51099 to
bd7e76f
Compare
4114482 to
05183a3
Compare
bobbinth
left a comment
There was a problem hiding this comment.
Looks good! Thank you! I mostly focused on reviewing protobuf code and skimmed over most other code. I left some comments inline - some may require alignment but most are pretty straight forward (I think).
bobbinth
left a comment
There was a problem hiding this comment.
I mostly reviewed the protobuf files - but looks good! Thank you!
The tests are failing now - I suspect this may be due to the v0.30 VM migration.
Yep, I've merged |
bobbinth
left a comment
There was a problem hiding this comment.
I mostly double-checked protobuf changes again - all looks good there! Thank you!
There was a problem hiding this comment.
Do we not need a ProvenBlock (or something like that) message? If we do - I'd add it in a follow-up PR (rather than this one).
| message PublicKey { | ||
| bytes encoded = 1; | ||
| } |
There was a problem hiding this comment.
We'll most likely have EdDSA and ECDSA over P256 curve soon - but all of these will be just variant + the actual key - so, the message structure could remain the same.
zeapoz
left a comment
There was a problem hiding this comment.
Looks good to me! I left two suggestion that I think are worth considering before merging, but I wouldn't consider them blocking
| // STORAGE PATCHES | ||
| // ================================================================================================ | ||
|
|
||
| const fn encode_storage_operation(operation: StoragePatchOperation) -> i32 { |
There was a problem hiding this comment.
What do you think about adding a small doc section to these mappping functions just to mention the fact that the integer representation is different between the domain types and the proto types? As an example, here we have:
Rust StoragePatchOperation (patch_operation.rs):
| Variant | u8 |
|---|---|
Create |
0 |
Update |
1 |
Remove |
2 |
Proto StoragePatchOperation (account.proto):
| Variant | Proto value |
|---|---|
STORAGE_PATCH_OPERATION_UNSPECIFIED |
0 |
STORAGE_PATCH_OPERATION_CREATE |
1 |
STORAGE_PATCH_OPERATION_UPDATE |
2 |
STORAGE_PATCH_OPERATION_REMOVE |
3 |
Since best practice for protobuf involve storing an explicit unspecified operation all other values shift by one, so I would suggest mentioning that the encoding is slightly different in the docs
These are the ones affected that I found:
StoragePatchOperation
AssetComposition
StorageSlotType
NoteType
Summary
Supports 0xMiden/node#2471 (0xMiden/node#2471).
The node’s gRPC migration replaces opaque, Miden-native byte payloads with structured Protobuf messages. These object schemas belong alongside the protocol definitions rather than inside an RPC implementation: keeping them in the node would force clients and services to duplicate generated types and protocol validation, allowing their representations to drift.
This PR introduces
miden-objectsas the canonical transport representation for protocol objects exchanged between clients and nodes. It does not replacemiden-protocol’s native serialization or define RPC services.Changes
Added structured schemas for field elements, words, accounts and account patches, notes and attachments, transactions and batches, block headers and bodies, partial blockchains, partial MMR, Merkle data, and signed blocks.
Added bidirectional conversions between generated messages and protocol types, with field-path-aware errors and validation delegated to protocol constructors from feat(protocol): validate batch and block data at construction #3706.
Kept execution proofs, MAST forests, public keys, and signatures in their canonical encodings behind typed Protobuf wrapper messages, avoiding raw bytes fields throughout higher-level messages.
Exposed a self-contained descriptor set and canonical Prost external paths so RPC crates can import these schemas without generating duplicate Rust object types.
Existing protocol serialization and commitment calculation are unchanged by this PR.