Support of mint/burn of assets in decode transaction #3025

paweljakubas · 2021-11-17T19:57:19Z

I have added support for mint/burn assets along with illustrative integration tests

I will add supports for certs with integration tests in next PR

Comments

Issue Number

jonathanknowles

Hi @paweljakubas

Two comments so far:

So far, we've used the terminology "burned" rather than "burnt", both in our API types and in our internal algorithms. I think it would be good to be consistent and continue to use "burned" everywhere, rather than having a mixture of the two.
There are two fields in ApiDecodedTransaction: assets_minted and assets_burned that have the same type ApiMintedBurnedAssets. Given that minted and burned assets are separated in this way, I think it's okay to say that ApiMintedBurnedAssets can only contain positive values. Indeed, this provides a nicer mapping to our internal types, which do not allow negative values. What do you think?

jonathanknowles · 2021-11-18T03:15:55Z

specifications/api/swagger.yaml

@@ -941,6 +941,19 @@ x-assetMint: &assetMint
        Positive values mean creation and negative values mean
        destruction.

+x-assetMintedBurnt: &assetMintedBurnt


Suggestion: use "burned", rather than "burnt".

Elsewhere, we have used the "burned" terminology; it's probably good to be consistent.

Suggested change

x-assetMintedBurnt: &assetMintedBurnt

x-assetMintedBurned: &assetMintedBurned

jonathanknowles · 2021-11-18T03:16:12Z

specifications/api/swagger.yaml

@@ -2381,6 +2394,16 @@ components:
      description: |
        Withdrawals that could be external or belong to the wallet.

+    ApiMintedBurntAssets: &ApiMintedBurntAssets


Suggested change

ApiMintedBurntAssets: &ApiMintedBurntAssets

ApiMintedBurnedAssets: &ApiMintedBurnedAssets

jonathanknowles · 2021-11-18T03:16:23Z

specifications/api/swagger.yaml

@@ -2381,6 +2394,16 @@ components:
      description: |
        Withdrawals that could be external or belong to the wallet.

+    ApiMintedBurntAssets: &ApiMintedBurntAssets
+      description: |
+        Assets minted (created) or burnt (destroyed)


Suggested change

Assets minted (created) or burnt (destroyed)

Assets minted (created) or burned (destroyed)

jonathanknowles · 2021-11-18T03:22:26Z

specifications/api/swagger.yaml

+        assets_minted: *ApiMintedBurntAssets
+        assets_burnt: *ApiMintedBurntAssets


Hi @paweljakubas

I'm wondering why are there two of these fields, when both of these fields are ApiMintedBurnedAssets?

Shouldn't it be something like:

assets_minted: *ApiMintedAssets assets_burned: *ApiBurnedAssets

jonathanknowles · 2021-11-18T03:25:29Z

specifications/api/swagger.yaml

+        Assets minted (created) or burnt (destroyed)
+        This amount contributes to the total transaction value.
+        Positive values denote creation of assets and negative values
+        denote the reverse.


Given that you have two fields of this type, I think it would be okay to only allow positive values here.

revised this fragments

paweljakubas · 2021-11-18T08:18:50Z

thanks @jonathanknowles for looking at this. First of all, let's address only minting/burning in this PR, and certs in separate PR. When it comes to positive values for both minting/burning I think it is ok to have this like that. We can change this during adding minting/burning support in wallet soon, though. Right now it is the least invasive to have this with decomposition into minting and burning using positive values.

jonathanknowles · 2021-11-18T08:40:40Z

specifications/api/swagger.yaml

+
+    ApiBurnedAssets: &ApiBurnedAssets
+      description: |
+        Assets burned (destroyed)


Suggested change

Assets burned (destroyed)

Assets burned (destroyed).

jonathanknowles · 2021-11-18T08:45:48Z

specifications/api/swagger.yaml

+    ApiMintedAssets: &ApiMintedAssets
+      description: |
+        Assets minted (created).
+        This amount contributes to the total transaction value.


Suggested change

This amount contributes to the total transaction value.

The act of minting assets provides value to a transaction.

jonathanknowles · 2021-11-18T08:46:05Z

specifications/api/swagger.yaml

+    ApiBurnedAssets: &ApiBurnedAssets
+      description: |
+        Assets burned (destroyed)
+        This amount contributes to the total transaction value.


Suggested change

This amount contributes to the total transaction value.

The act of burning assets consumes value from a transaction.

jonathanknowles · 2021-11-18T08:48:45Z

lib/core-integration/src/Test/Integration/Scenario/API/Shelley/TransactionsNew.hs

+                (unsafeFromHex $ T.encodeUtf8 cborHexWithBurning)
+
+        let (Right txBodyBurn) = Cardano.deserialiseFromTextEnvelope (Cardano.AsTxBody Cardano.AsAlonzoEra) textEnvelopeBurn
+        let cborHexBurn = T.decodeUtf8 $ hex $ Cardano.serialiseToCBOR (Cardano.makeSignedTransaction [] txBodyBurn)


(line length limit)

Suggested change

let cborHexBurn = T.decodeUtf8 $ hex $ Cardano.serialiseToCBOR (Cardano.makeSignedTransaction [] txBodyBurn)

let cborHexBurn = T.decodeUtf8

$ hex

$ Cardano.serialiseToCBOR

$ Cardano.makeSignedTransaction [] txBodyBurn

jonathanknowles · 2021-11-18T08:49:35Z

lib/core-integration/src/Test/Integration/Scenario/API/Shelley/TransactionsNew.hs

+                ""
+                (unsafeFromHex $ T.encodeUtf8 cborHexWithBurning)
+
+        let (Right txBodyBurn) = Cardano.deserialiseFromTextEnvelope (Cardano.AsTxBody Cardano.AsAlonzoEra) textEnvelopeBurn


(line length limit)

Suggested change

let (Right txBodyBurn) = Cardano.deserialiseFromTextEnvelope (Cardano.AsTxBody Cardano.AsAlonzoEra) textEnvelopeBurn

let Right txBodyBurn = Cardano.deserialiseFromTextEnvelope

(Cardano.AsTxBody Cardano.AsAlonzoEra)

textEnvelopeBurn

jonathanknowles · 2021-11-18T08:49:58Z

lib/core-integration/src/Test/Integration/Scenario/API/Shelley/TransactionsNew.hs

+                (unsafeFromHex $ T.encodeUtf8 cborHexWithMinting)
+
+        let (Right txBodyMint) = Cardano.deserialiseFromTextEnvelope (Cardano.AsTxBody Cardano.AsAlonzoEra) textEnvelopeMint
+        let cborHexMint = T.decodeUtf8 $ hex $ Cardano.serialiseToCBOR (Cardano.makeSignedTransaction [] txBodyMint)


(line length limit)

Suggested change

let cborHexMint = T.decodeUtf8 $ hex $ Cardano.serialiseToCBOR (Cardano.makeSignedTransaction [] txBodyMint)

let cborHexMint = T.decodeUtf8

$ hex

$ Cardano.serialiseToCBOR

$ Cardano.makeSignedTransaction [] txBodyMint

jonathanknowles · 2021-11-18T08:50:15Z

lib/core-integration/src/Test/Integration/Scenario/API/Shelley/TransactionsNew.hs

+                ""
+                (unsafeFromHex $ T.encodeUtf8 cborHexWithMinting)
+
+        let (Right txBodyMint) = Cardano.deserialiseFromTextEnvelope (Cardano.AsTxBody Cardano.AsAlonzoEra) textEnvelopeMint


(line length limit)

Suggested change

let (Right txBodyMint) = Cardano.deserialiseFromTextEnvelope (Cardano.AsTxBody Cardano.AsAlonzoEra) textEnvelopeMint

let Right txBodyMint = Cardano.deserialiseFromTextEnvelope

(Cardano.AsTxBody Cardano.AsAlonzoEra)

textEnvelopeMint

jonathanknowles · 2021-11-18T08:53:00Z

lib/core-integration/src/Test/Integration/Scenario/API/Shelley/TransactionsNew.hs

+        let tokens = TokenMap.fromNestedList
+                [(tokenPolicyId, NE.fromList [(UnsafeTokenName "HappyCoin", TokenQuantity 50000)])]


Suggestion: use TokenMap.singleton here:

Suggested change

let tokens = TokenMap.fromNestedList

[(tokenPolicyId, NE.fromList [(UnsafeTokenName "HappyCoin", TokenQuantity 50000)])]

let tokens = TokenMap.singleton

(AssetId tokenPolicyId (UnsafeTokenName "HappyCoin"))

(TokenQuantity 50_000)

jonathanknowles · 2021-11-18T08:54:04Z

lib/core-integration/src/Test/Integration/Scenario/API/Shelley/TransactionsNew.hs

+            (Link.decodeTransaction @'Shelley wa) Default decodeMintPayload
+        verify rTx
+            [ expectResponseCode HTTP.status202
+            , expectField (#fee . #getQuantity) (`shouldBe` 202725)


Suggestion: use NumericUnderscores for extra clarity.

Suggested change

, expectField (#fee . #getQuantity) (`shouldBe` 202725)

, expectField (#fee . #getQuantity) (`shouldBe` 202_725)

jonathanknowles · 2021-11-18T08:54:36Z

lib/core-integration/src/Test/Integration/Scenario/API/Shelley/TransactionsNew.hs

+            (Link.decodeTransaction @'Shelley wa) Default decodeBurnPayload
+        verify rTx'
+            [ expectResponseCode HTTP.status202
+            , expectField (#fee . #getQuantity) (`shouldBe` 202725)


Suggestion: use NumericUnderscores for extra clarity.

Suggested change

, expectField (#fee . #getQuantity) (`shouldBe` 202725)

, expectField (#fee . #getQuantity) (`shouldBe` 202_725)

paweljakubas · 2021-11-18T10:00:46Z

bors r+

iohk-bors · 2021-11-18T10:48:14Z

Build succeeded:

paweljakubas self-assigned this Nov 17, 2021

jonathanknowles reviewed Nov 18, 2021

View reviewed changes

paweljakubas added 7 commits November 18, 2021 09:32

add ApiMintBurntAssets to swagger

0c81aac

update ApiDecodedTransaction type and impl in Server

a7d9fb0

update core unit tests

3a11174

add minting integration test

d2120ee

use cardano-cli cbors properly in integration tests

d78f920

add more expectations

18703af

review improvements

e9cffcc

paweljakubas force-pushed the paweljakubas/adp-1245/mint-burn-certs-decode-transaction branch from 3572145 to e9cffcc Compare November 18, 2021 08:32

paweljakubas requested a review from jonathanknowles November 18, 2021 08:32

paweljakubas changed the title ~~Support of mint/burn of assets and certs in decode transaction~~ Support of mint/burn of assets in decode transaction Nov 18, 2021

paweljakubas marked this pull request as ready for review November 18, 2021 08:36

jonathanknowles reviewed Nov 18, 2021

View reviewed changes

next review refinements

96112b2

paweljakubas requested a review from jonathanknowles November 18, 2021 09:54

jonathanknowles approved these changes Nov 18, 2021

View reviewed changes

iohk-bors bot merged commit d768d8a into master Nov 18, 2021

iohk-bors bot deleted the paweljakubas/adp-1245/mint-burn-certs-decode-transaction branch November 18, 2021 10:48

WilliamKingNoel-Bot pushed a commit that referenced this pull request Nov 18, 2021

docs: Merge #3025

86cb625

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Support of mint/burn of assets in decode transaction #3025

Support of mint/burn of assets in decode transaction #3025

paweljakubas commented Nov 17, 2021 •

edited

Loading

jonathanknowles left a comment

jonathanknowles Nov 18, 2021

jonathanknowles Nov 18, 2021

jonathanknowles Nov 18, 2021

jonathanknowles Nov 18, 2021

paweljakubas Nov 18, 2021

jonathanknowles Nov 18, 2021

paweljakubas Nov 18, 2021

paweljakubas commented Nov 18, 2021

jonathanknowles Nov 18, 2021

jonathanknowles Nov 18, 2021

jonathanknowles Nov 18, 2021

jonathanknowles Nov 18, 2021

jonathanknowles Nov 18, 2021

jonathanknowles Nov 18, 2021

jonathanknowles Nov 18, 2021

jonathanknowles Nov 18, 2021

jonathanknowles Nov 18, 2021

jonathanknowles Nov 18, 2021

paweljakubas commented Nov 18, 2021

iohk-bors bot commented Nov 18, 2021

	x-assetMintedBurnt: &assetMintedBurnt
	x-assetMintedBurned: &assetMintedBurned

	ApiMintedBurntAssets: &ApiMintedBurntAssets
	ApiMintedBurnedAssets: &ApiMintedBurnedAssets

	Assets minted (created) or burnt (destroyed)
	Assets minted (created) or burned (destroyed)

		assets_minted: *ApiMintedBurntAssets
		assets_burnt: *ApiMintedBurntAssets

	This amount contributes to the total transaction value.
	The act of minting assets provides value to a transaction.

	This amount contributes to the total transaction value.
	The act of burning assets consumes value from a transaction.

		let tokens = TokenMap.fromNestedList
		[(tokenPolicyId, NE.fromList [(UnsafeTokenName "HappyCoin", TokenQuantity 50000)])]

-        let tokens = TokenMap.fromNestedList
-                [(tokenPolicyId, NE.fromList [(UnsafeTokenName "HappyCoin", TokenQuantity 50000)])]
+        let tokens = TokenMap.singleton
+                (AssetId tokenPolicyId (UnsafeTokenName "HappyCoin"))
+                (TokenQuantity 50_000)

	, expectField (#fee . #getQuantity) (`shouldBe` 202725)
	, expectField (#fee . #getQuantity) (`shouldBe` 202_725)

Support of mint/burn of assets in decode transaction #3025

Support of mint/burn of assets in decode transaction #3025

Conversation

paweljakubas commented Nov 17, 2021 • edited Loading

Comments

Issue Number

jonathanknowles 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

paweljakubas commented Nov 18, 2021

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

paweljakubas commented Nov 18, 2021

iohk-bors bot commented Nov 18, 2021

paweljakubas commented Nov 17, 2021 •

edited

Loading