Move transaction builder and transparent types to anoma#1
Move transaction builder and transparent types to anoma#1Scar26 wants to merge 8 commits intomurisi:murisi/masp-incentivefrom
Conversation
Scar26
force-pushed
the
scar/masp-incentives
branch
from
f620cac to
e15cb2c
Compare
murisi
left a comment
murisi
left a comment
There was a problem hiding this comment.
Thanks for this PR! I think your movement of some Transaction construction code makes sense. I wonder whether we should do more of this...
| pub struct AllowedConversion { | ||
| /// The asset type that the note represents | ||
| assets: Amount, | ||
| pub assets: Amount, |
There was a problem hiding this comment.
Wouldn't making the assets field public would allow users of this interface to break its invariants? Specifically, I'm scared that people could modify assets whilst leaving generator unchanged which would mean that assets would cease to correspond to generator. I guess you probably want just a getter function...
| Clone, Debug, PartialEq, BorshSerialize, BorshDeserialize, Serialize, Deserialize, Eq, Hash | ||
| )] | ||
| pub struct Amount(BTreeMap<AssetType, i64>); | ||
| pub struct Amount(pub(crate) BTreeMap<AssetType, i64>); |
There was a problem hiding this comment.
One of the invariants of Amount is that its BTreeMap never contains an entry whose value equals 0. Forcing this canonicity simplifies the implementation of Eq and asset_types() for Amount. I'm scared that increasing the visibility of Amount's BTreeMap might allow users in the crate to put Amount into an invalid state. Maybe we could instead add a constructor accepting a BTreeMap?
| /// A Zcash transaction. | ||
| #[derive(Debug, Serialize, Deserialize, Clone, Hash, Eq, PartialOrd)] | ||
| pub struct Transaction { | ||
| pub struct Transaction<Ti: TxIn, To: TxOut> { |
There was a problem hiding this comment.
I can see how a generically parameterized Transaction might be of greater use to other parties, but on the hand in Anoma we only instantiate this struct in one way (right?) and I would guess that transaction formats/encodings are likely to be unique to each blockchain project.
I liked how you moved builder.rs to the anoma/anoma repo, I wonder if we could just move the entire transaction folder (apart from Amount) there if possible? I think that doing this might increase this branch's likelihood of approval from the maintainers (given that this crate's main purpose is to provide cryptographic circuits)...
There was a problem hiding this comment.
True for our use in anoma, we don't really need generic arguments for transparent inputs and outputs. To that effect I can also see moving Transaction to anoma as it's also tied to how we structure transactions in anoma. But I think it would still make sense to keep spend, output and convert descriptions in masp as they are part of the spec and necessary for any third party integration. Thoughts?
There was a problem hiding this comment.
Your idea of keeping spend, output, and convert in the MASP crate is another valid approach that also makes sense. I'd say, do whatever yields the simplest code.
| AssetType::new(b"XAN").unwrap() | ||
| } | ||
| #[test] | ||
| fn test_homomorphism() { |
There was a problem hiding this comment.
Where do we now test that Amounts operations (for example AddAsign) do indeed preserve the homomorphism (i.e. that the commitment of assets always equals generator)? I guess we just need to make sure that this memorization optimization preserves the correctness of the original implementation...
| ); | ||
| } | ||
| #[test] | ||
| fn test_serialization() { |
There was a problem hiding this comment.
We should probably also check that the serialization and deserialization functions are indeed inverses of each other somewhere...
| /// Attempt to construct an asset type from an existing asset identifier | ||
| pub fn from_identifier(identifier: &[u8; ASSET_IDENTIFIER_LENGTH]) -> Option<AssetType> { | ||
| // Attempt to hash to point | ||
| println!("{:?}", identifier); |
There was a problem hiding this comment.
I'm not sure about printing to STDOUT here... This might cause issues (i.e. messing up UI or formatting) when this function is used in the anoma crate.
| } | ||
|
|
||
| impl AllowedConversion { | ||
| pub fn new(values: Vec<(AssetType, i64)>) -> Self { |
There was a problem hiding this comment.
What happens if this vector contains the same AssetType twice? In the case that the behavior makes conceptual sense, maybe we should document it? If the behavior doesn't make sense, maybe we should use an alternative type for values that does not allow duplicate keys (AssetTypes). Do whatever is simplest...
Signed-off-by: Scar26 <mmatty26@gmail.com>
Signed-off-by: Scar26 <mmatty26@gmail.com>
Signed-off-by: Scar26 <mmatty26@gmail.com>
Signed-off-by: Scar26 <mmatty26@gmail.com>
Signed-off-by: Scar26 <mmatty26@gmail.com>
Add getter for AllowedConversion.assets Add constructor for Amount
Signed-off-by: Scar26 <mmatty26@gmail.com>
Signed-off-by: Scar26 <mmatty26@gmail.com>
Scar26
force-pushed
the
scar/masp-incentives
branch
from
9ff2ae2 to
04e85e0
Compare
2 participants
No description provided.