Make better use of the type system #6

thomaseizinger · 2020-11-17T07:45:11Z

See commit messages.

This is helpful because we sometimes need to peek at the next value to deserialize the right thing.

The previous model was to flexible in that it allowed to express states that were not actually possible (like explicit value but confidential asset).

luckysori

Half-review

elements-fun/src/confidential.rs

luckysori · 2020-11-17T23:14:19Z

elements-fun/src/wally.rs

    tx: &crate::Transaction,
    index: usize,
    script_code: &bitcoin::Script,
-    value: &crate::confidential::Value,
+    value: &V, // TODO: Is this really the best way to do it?


❓ Should we introduce a ValueCommitment type?

It actually exists. Should we use it here?

Well, you can use this function for confidential and non-confidential values. The new model system doesn't (yet) offer anything here.

elements-fun/src/wally.rs

luckysori · 2020-11-17T23:19:13Z

elements-fun/src/transaction.rs

 use crate::issuance::AssetId;

+// TODO: Is `Default` really useful here?


❓ Why not? A null asset issuance is what I would expect when I need the rest of a transaction to be default.

luckysori · 2020-11-17T23:21:39Z

elements-fun/src/transaction.rs

+
+impl ExplicitAssetIssuance {
+    pub fn encoded_length(&self) -> usize {
+        32 + 32 + self.amount.encoded_length() + self.inflation_keys.encoded_length()


❓ Should we use a SECRET_KEY_LENGTH sort of constant (that may already exist) instead of magic numbers?

luckysori · 2020-11-17T23:22:44Z

elements-fun/src/transaction.rs

+pub struct NullAssetIssuance {
+    /// Zero for a new asset issuance; otherwise a blinding factor for the input
+    pub asset_blinding_nonce: [u8; 32],
+    /// Freeform entropy field
+    pub asset_entropy: [u8; 32],
+}


❓ Can you explain why a null asset issuance has any fields at all? Doesn't make sense to me given the name.

Well, the data is still there I think, like I need to deserialize it. Will have to do some testing to see if this can be improved!

It's just weird that there has to be any data at all if the transaction does not issue an asset. Unless this data is unrelated to asset issuances?

luckysori · 2020-11-17T23:24:06Z

elements-fun/src/transaction.rs

+)]
+pub struct ConfidentialAssetIssuance {
+    /// Zero for a new asset issuance; otherwise a blinding factor for the input
+    pub asset_blinding_nonce: [u8; 32],


❓ Have you thought about using SecretKey for some of these fields? Would it make sense?

Haven't yet thought about it, wanted to get the big refactoring in first!

luckysori

This looks really good to me! I would consider upstreaming this. The worst that can happen is that they say no and we just continue with our own version of it.

luckysori · 2020-11-17T23:35:24Z

elements-fun/src/transaction.rs

 pub struct PeginData<'tx> {
    /// Reference to the pegin output on the mainchain
    pub outpoint: bitcoin::OutPoint,
    /// The value, in satoshis, of the pegin
    pub value: u64,
    /// Asset type being pegged in
-    pub asset: confidential::Asset,
+    pub asset: AssetId,


❓ Does this mean that this struct is always explicit?

Yes from my understanding yes!

luckysori · 2020-11-17T23:36:54Z

elements-fun/src/transaction.rs

+    derive(serde::Serialize, serde::Deserialize),
+    serde(crate = "serde_crate")
+)]
+pub struct NullTxOut {


❓ When does elements use null txouts? Just curious.

I don't fully understand this yet :D

luckysori · 2020-11-17T23:41:44Z

elements-fun/src/transaction.rs

+    derive(serde::Serialize, serde::Deserialize),
+    serde(crate = "serde_crate", transparent)
+)]
+pub struct ExplicitValue(pub u64);


❓ Should we be using bitcoin::Amount under the hood? Is that a horrible idea?

Not a horrible idea because all tokens have the same decimal points :)

luckysori · 2020-11-17T23:47:31Z

elements-fun/src/transaction.rs

+    pub fn unblind(&self, blinding_key: SecretKey) -> Result<UnblindedTxOut, UnblindError> {
+        let sender_ephemeral_pk = self.nonce.ok_or(UnblindError::MissingNonce)?.commitment();
+        let sender_ephemeral_pk = PublicKey::from_slice(&sender_ephemeral_pk)
+            .map_err(|_| UnblindError::InvalidPublicKey)?;
+
+        let (unblinded_asset, abf, vbf, value_out) = asset_unblind(
+            sender_ephemeral_pk,
+            blinding_key,
+            &self.witness.rangeproof,
+            self.value,
+            &self.script_pubkey,
+            self.asset,
+        )
+        .map_err(|_| UnblindError::Wally)?;
+
+        Ok(UnblindedTxOut {
+            asset: AssetId::from_slice(&unblinded_asset).unwrap(),
+            original_asset: self.asset,
+            asset_blinding_factor: abf,
+            value_blinding_factor: vbf,
+            value: value_out,
+        })
+    }
+}


🎉 This makes it all worth it! Awesome work.

elements-fun/src/transaction.rs

luckysori · 2020-11-17T23:53:16Z

elements-fun/src/transaction.rs


                    if !has_min {
                        min_value
                    } else if has_nonzero_range {
-                        bitcoin::consensus::deserialize::<u64>(&self.witness.rangeproof[2..10])
+                        bitcoin::consensus::deserialize::<u64>(&witness.rangeproof[2..10])


🤔 Imagine if we had a rangeproof struct.

It is all possible :D

luckysori · 2020-11-17T23:55:32Z

elements-fun/src/transaction.rs

+        // assert_eq!(
+        //     tx.input[0].asset_issuance,
+        //     AssetIssuance {
+        //         asset_blinding_nonce: [0; 32],
+        //         asset_entropy: [0; 32],
+        //         amount: confidential::Value::Confidential(
+        //             9,
+        //             [
+        //                 0x81, 0x65, 0x4e, 0xb5, 0xcc, 0xd9, 0x92, 0x7b, 0x8b, 0xea, 0x94, 0x99,
+        //                 0x7d, 0xce, 0x4a, 0xe8, 0x5b, 0x3d, 0x95, 0xa2, 0x07, 0x00, 0x38, 0x4f,
+        //                 0x0b, 0x8c, 0x1f, 0xe9, 0x95, 0x18, 0x06, 0x38
+        //             ],
+        //         ),
+        //         inflation_keys: confidential::Value::Null,
+        //     }
+        // );


🔧 I guess you meant to remove it.

Ups, no I didn't!

Will put it back!

thomaseizinger requested review from bonomat and luckysori November 17, 2020 07:45

thomaseizinger force-pushed the better-type-system branch from 122cc7d to b4af743 Compare November 17, 2020 23:24

thomaseizinger added 8 commits November 18, 2020 10:24

Fix clippy warnings

b559023

Cargo fmt the whole codebase

0058835

Make decoder always receive a buffer

56f5549

This is helpful because we sometimes need to peek at the next value to deserialize the right thing.

Introduce TxInWitness::encoded_length

42ae3d5

Introduce TxOutWitness::encoded_length

683f710

Introduce dedicated types for explicit and confidential aspects

c2ff20d

The previous model was to flexible in that it allowed to express states that were not actually possible (like explicit value but confidential asset).

Introduce ConfidentialTxOut::unblind

1b8b95b

Add TODO

b4af743

luckysori reviewed Nov 17, 2020

View reviewed changes

luckysori merged commit 084661a into comit-network:master Nov 17, 2020

luckysori reviewed Nov 17, 2020

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Make better use of the type system #6

Make better use of the type system #6

thomaseizinger commented Nov 17, 2020 •

edited

Loading

luckysori left a comment

luckysori Nov 17, 2020

luckysori Nov 17, 2020

thomaseizinger Nov 17, 2020 •

edited

Loading

luckysori Nov 17, 2020

luckysori Nov 17, 2020

luckysori Nov 17, 2020

thomaseizinger Nov 17, 2020

luckysori Nov 17, 2020

luckysori Nov 17, 2020

thomaseizinger Nov 17, 2020 •

edited

Loading

luckysori left a comment

luckysori Nov 17, 2020

thomaseizinger Nov 18, 2020

luckysori Nov 17, 2020

thomaseizinger Nov 18, 2020

luckysori Nov 17, 2020

thomaseizinger Nov 18, 2020

luckysori Nov 17, 2020

luckysori Nov 17, 2020

thomaseizinger Nov 18, 2020

luckysori Nov 17, 2020

thomaseizinger Nov 18, 2020

		use crate::issuance::AssetId;

		// TODO: Is `Default` really useful here?

Make better use of the type system #6

Make better use of the type system #6

Conversation

thomaseizinger commented Nov 17, 2020 • edited Loading

luckysori left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

thomaseizinger Nov 17, 2020 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

thomaseizinger Nov 17, 2020 • edited Loading

Choose a reason for hiding this comment

luckysori left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

thomaseizinger commented Nov 17, 2020 •

edited

Loading

thomaseizinger Nov 17, 2020 •

edited

Loading

thomaseizinger Nov 17, 2020 •

edited

Loading