feat: add transaction types to mina-tx-type crate#3481
feat: add transaction types to mina-tx-type crate#3481dannywillems wants to merge 3 commits intomasterfrom
Conversation
29007f7 to
228aa33
Compare
228aa33 to
2de270c
Compare
| #[derive(Debug, Clone, PartialEq, Eq)] | ||
| pub struct Update { | ||
| /// zkApp state fields (8 field elements). | ||
| pub app_state: [SetOrKeep<Fp>; 8], |
There was a problem hiding this comment.
TODO: add a feature flag for berkeley and a feature flag for mesa, as it changes between both upgrades.
Add the remaining Mina transaction types to the mina-tx-type crate: currency (Balance, Nonce, Slot, SlotSpan, Length, TxnVersion), common utility types (SetOrKeep, OrIgnore, ClosedInterval, OneOrTwo), primitives (TokenId, Memo, AccountId, VotingFor, ZkAppUri, TokenSymbol), permissions (AuthRequired, Permissions), signed commands (payments and delegations), preconditions (network, account, epoch), zkApp commands (account updates, verification keys, events, actions), fee transfers, and top-level Transaction/UserCommand enums.
2de270c to
ab738f8
Compare
richardpringle
left a comment
There was a problem hiding this comment.
I don't understand what all these common types are used for. I didn't see a single pattern match on the enums. There's a lot of code in here that's a maintenance burden if you ask me.
There was a problem hiding this comment.
FYI, I'm reviewing this like it's net new code. If it's moved code, that seems fine, I guess.
| #[derive(Debug, Clone, PartialEq, Eq)] | ||
| pub enum SetOrKeep<T> { | ||
| /// Set the field to this value. | ||
| Set(T), | ||
| /// Keep the field unchanged. | ||
| Keep, | ||
| } |
There was a problem hiding this comment.
This is effectively a duplicate of an Option<T> and I therefore don't see any of its value. What can it do that an Option<T> cannot?
There was a problem hiding this comment.
It does replicate the OCaml structure available here.
I agree it is isomorphic to Option<T>.
I am not against changing it to something else later.
| Check(T), | ||
| /// Ignore this field (no constraint). | ||
| Ignore, | ||
| } |
There was a problem hiding this comment.
Same thing for this. It's the same as both Option<T> and SetOrKeep<T>
| pub lower: T, | ||
| /// The upper bound (inclusive). | ||
| pub upper: T, | ||
| } |
There was a problem hiding this comment.
This type also already exists in the standard library:
https://doc.rust-lang.org/stable/std/ops/struct.RangeInclusive.html
| One(T), | ||
| /// Exactly two elements. | ||
| Two(T, T), | ||
| } |
There was a problem hiding this comment.
I think it would be better to use standard types here too. (T, Option<T>) accomplishes the same thing.
| /// A hash precondition (check exact value or ignore). | ||
| pub type HashCheck<T> = OrIgnore<T>; | ||
|
|
||
| /// An equality-check precondition (check exact value or ignore). | ||
| pub type EqCheck<T> = OrIgnore<T>; |
There was a problem hiding this comment.
Why are these here? Type aliases aren't meant to rename other types like this. EqCheck and OrIgnore names imply this single type should not be used for the same thing, but it can, because theres aren't two types, it's one type with two completely different names.
|
|
||
| impl_number_u32!(Length, "A blockchain length (block height)."); | ||
|
|
||
| impl_number_u32!(TxnVersion, "A transaction version number."); |
There was a problem hiding this comment.
Same comments about the other stuff. I think it would be much better to replace all of this with new-types and just use the underlying values. Dependent code on these type should still mostly work. Even in places where it doesn't, it should only take small fixes
| /// Format: byte 0 = tag (0x01 for user), byte 1 = length, | ||
| /// bytes 2..34 = content (padded with zeros). | ||
| #[derive(Debug, Clone, PartialEq, Eq, Hash)] | ||
| pub struct Memo(pub [u8; 34]); |
There was a problem hiding this comment.
For something like this it's better to have
use bytemuck::{Pod, Zeroable};
/// A transaction memo (34 bytes).
///
/// Format: byte 0 = tag (0x01 for user), byte 1 = length,
/// bytes 2..34 = content (padded with zeros).
///
/// # Example
///
/// ```
/// use bytemuck::{bytes_of, from_bytes};
/// # // Mocking the struct location for the doctest
/// # use playground::Memo;
///
/// // 1. Represent raw data (e.g., from a network stream)
/// let mut raw_bytes = [0u8; 34];
/// raw_bytes[0] = 0x01; // Tag
/// raw_bytes[1] = 0x05; // Length
/// raw_bytes[2..7].copy_from_slice(b"Hello"); // Content
///
/// // 2. Zero-cost cast from bytes to &Memo
/// // This cannot fail here because the size (34) and alignment (1) match.
/// let memo: &Memo = from_bytes(&raw_bytes);
///
/// assert_eq!(memo.tag, 1);
/// assert_eq!(memo.length, 5);
/// assert_eq!(&memo.content[..5], b"Hello");
///
/// // 3. Zero-cost cast back to bytes
/// let bytes: &[u8] = bytes_of(memo);
/// assert_eq!(bytes.len(), 34);
/// ```
#[derive(Debug, Clone, Copy, PartialEq, Eq, Hash, Pod, Zeroable)]
#[repr(C)]
pub struct Memo {
/// Byte 0: Tag (0x01 for user)
pub tag: u8,
/// Byte 1: Length of the content
pub length: u8,
/// Bytes 2..34: Content (padded with zeros)
pub content: [u8; 32],
}| /// The public key of the new delegate. | ||
| new_delegate: CompressedPubKey, | ||
| }, | ||
| } |
There was a problem hiding this comment.
Why an enum with a single variant? That might make sense if we wanted to add #[non_exhaustive] to future proof things, but that isn't being done here.
In a follow-up, we will add hashing, regtest, etc. We go step by step.
Summary
Transaction/UserCommandenums to themina-tx-typecrateBalance,Nonce,Slot,SlotSpan,Length,TxnVersion), common utilities (SetOrKeep,OrIgnore,ClosedInterval,OneOrTwo), primitives (TokenId,Memo,AccountId), permissions (AuthRequired,Permissions), and preconditions (NetworkPreconditions,AccountPrecondition)no_stdcompatible with no circuit traits ormina-p2p-messagesdependency, onlymina-signerandmina-curvesBuilds on top of #3427 which introduced the crate with
Coinbaseand currency types.Test plan
cargo check -p mina-tx-typepassescargo clippy -p mina-tx-type -- -D warningspasses cleancargo test -p mina-tx-typepasses (doc test)