Mint burn API only, stubbed implementation #2712

sevanspowell · 2021-06-15T06:02:01Z

Issue Number

Overview

This PR modifies the cardano-wallet API to support minting and burning. Minting and burning are not actually implemented, just stubbed. This was done to minimize the amount of context the reviewer needs to keep in mind when reviewing - we are just reviewing the form of the new minting and burning endpoints.

Add a new "mint" endpoint under the "assets" API group.
Add a number of new API types to represent the data the user must submit with a mint/burn request, and the data they receive back.
Adjust swagger specification to match new API types.
Adds Arbitrary instances for API types for property tests.
Adds a stub for the minting and burning implementation.

sevanspowell · 2021-06-15T08:56:08Z

Believe test failures are flaky tests.

sevanspowell · 2021-06-15T08:58:55Z

lib/core/src/Cardano/Wallet/Api.hs

+    -- :> "assets"
+    -- :> "mint"
+    -- This has been changed to prevent overlapping routes:
+    :> "assets-mint"


I'm not happy with this, does anyone know of a way to prevent servant from overlapping the routes?

The other routes in this namespace are of the form:

/assets/$policyId/...

So when it sees:

/assets/mint

It tries to treat "mint" as a policy ID and fails - even though this is a POST endpoint and the rest are GET.

Hi @sevanspowell

It tries to treat "mint" as a policy ID and fails - even though this is a POST endpoint and the rest are GET.

Does the order in which the endpoints appear in the Servant type definition make a difference? What happens if you try the following order:

type Assets = ListAssets :<|> MintBurnAssets :<|> GetAsset :<|> GetAssetDefault ``

Hey @jonathanknowles, the order in which the endpoints appear does not seem to matter in my experiments. I'll try and generate a failing test to better demonstrate what's going on.

I've got a bit stuck trying to demonstrate this with a test, but I've been able to reproduce it by CURLing the running wallet.

The order of the Servant type definition makes no difference to the outcome.

What I've done (suggested by @rvl), is to just make the mint/burn endpoint:

POST /wallets/.../assets

The rdoc was complaining about duplicate keys in swagger.yaml. I've fixed it in 6d17d33.

jonathanknowles

Hi @sevanspowell

Thanks for making this PR. 👍🏻

I have a question: just for my own enlightenment, is there an overview of the use cases and requirements that the API in this PR is designed to satisfy? (One that I can read through and get a general idea of how the user is intended to use this API.)

For example: I noticed that the ApiMintBurnData can take a series of mint-burn operations. I'm just wondering what the intended use case is for this. (So I can educate myself and get a better idea of how to evaluate the API in PR.)

Thanks again!

jonathanknowles · 2021-06-16T06:25:48Z

lib/core/src/Cardano/Wallet/Api.hs

+-- | https://input-output-hk.github.io/cardano-wallet/api/#operation/mintBurnToken
+type MintBurnAsset n = "wallets"


Suggestion: make this plural (to capture the general case):

Suggested change

-- | https://input-output-hk.github.io/cardano-wallet/api/#operation/mintBurnToken

type MintBurnAsset n = "wallets"

-- | https://input-output-hk.github.io/cardano-wallet/api/#operation/mintBurnAssets

type MintBurnAssets n = "wallets"

jonathanknowles · 2021-06-16T06:33:50Z

lib/core/src/Cardano/Wallet/Api.hs

+    -- :> "assets"
+    -- :> "mint"
+    -- This has been changed to prevent overlapping routes:
+    :> "assets-mint"


Hi @sevanspowell

It tries to treat "mint" as a policy ID and fails - even though this is a POST endpoint and the rest are GET.

Does the order in which the endpoints appear in the Servant type definition make a difference? What happens if you try the following order:

type Assets = ListAssets :<|> MintBurnAssets :<|> GetAsset :<|> GetAssetDefault ``

sevanspowell · 2021-06-17T03:47:20Z

I think we originally saw two primary use cases for this feature:

To add minting and burning to our cardano-wallet test suite
To allow users to mint and burn using the cardano-wallet abstraction

The first use case is largely undefined, so for now all we will say is that it is beneficial to have minting and burning present in our test suites.

Regarding the second use case, minting and burning is currently achieved using the cardano-cli. A detailed example of minting an asset using cardano-cli can be found here.

Simplified, the important points are:

Assets are minted and burned under a "policy"
- For example, the simplest policy is: "only transactions signed by this
  key" may mint or burn tokens.
The typical process for minting/burning is:
1. Generate keys for the minting policy.
2. Create a policy script that allows only those keys to mint/burn assets.
3. Build a transaction that mints an asset under that policy script, with an
  asset name.
4. Sign the transaction with the policy script and the minting keys.
5. Submit the transaction.

A user of cardano-wallet typically wants to make use of HD wallets and so doesn't want to manually create keys for minting and burning, we have formed this API to meet that expectation.

The user doesn't refer to an asset via it's policy script, instead we ask the user for an index into a key derivation path (the monetary policy index), from this index we derive the keys for the policy, create a policy script from those keys, then build and submit the minting or burning transaction.

We do not try and support more complex policy scripts at this time (for example, the user could create a policy script that's can only mint after a given slot), which simplifies the requirements of this API.

I imagine a typical use-case of this API would be to mint a token to sell as an NFT:

export ASSET_NAME=$(echo my-nft | basenc --base16)
export RECV_ADDR="one of my addresses"

POST wallets/{walletID}/assets/mint 
  { "mint_burn": {
      "asset_name": "$ASSET_NAME",
      "monetary_policy_index": "0",
      "operations": [
        {
          "mint": {
            "amount": {
              "quantity": 1,
              "unit": "assets"
            }
            , "receiving_address": "$RECV_ADDR"
          }
        }
      ],
      "passphrase": "..."
    }
 }

One of the first things a user would want to do with their NFT is add metadata for it using offchain-metadata-tools:

export SUBJECT=$(resp from prev post | jq .subject)
export POLICY=$(resp from prev post | jq .policy)
token-metadata-creator entry $SUBJECT \
  --name "Quid" \
  --description "The un-offical currency of Great Britain." \
jq '.policy = "$POLICY"' $SUBJECT.json.draft

Next the user will need to attest to the metadata properties:

token-metadata-creator entry $SUBJECT -a policy/policy.skey

Unfortunately, this use case will fail here, we don't have access to the policy keys! They were abstracted away by cardano-wallet. So at this stage the usefulness of this API to the user is questionable.

Perhaps this could be handled in some way by modifying offchain-metadata-tools and/or cardano-wallet.

piotr-iohk · 2021-06-17T10:56:19Z

I imagine a typical use-case of this API would be to mint a token to sell as an NFT:
...
Unfortunately, this use case will fail here, we don't have access to the policy keys! They were abstracted away by cardano-wallet. So at this stage the usefulness of this API to the user is questionable.

I believe that despite not being able to put token metadata in the metadata server, it is still possible to use this feature as to mint NFTs. AFAIK NFT creators are adding some metadata on-chain to a minting transaction. There is a standard that's being discussed -> https://github.com/Berry-Pool/CIPs/blob/master/CIP-NFTMetadataStandard.md. In any case I think it would be good to state that limitation into release notes and swagger. I've added a note into swagger in f2bef0c.

piotr-iohk · 2021-06-17T19:30:48Z

specifications/api/swagger.yaml

+      allOf:
+        - *ApiAssetQuantity
+
+    ApiMintBurnOperation: &ApiMintBurnOperation


If I'm not mistaken operation is not quite correctly formatted in API doc:

It is:

"operations": [ { "mint": { "receiving_address": "addr1sjck9mdmfyhzvjhydcjllgj9vjvl522w0573ncustrrr2rg7h9azg4cyqd36yyd48t5ut72hgld0fg2xfvz82xgwh7wal6g2xt8n996s3xvu5g", "amount": { "quantity": 14, "unit": "assets" } }, "burn": { "quantity": 14, "unit": "assets" } } ]

but shouldn't it be:

[ { "mint": { "receiving_address": "addr_test1qq223dyrt96yx79rz3yda2vs6xjx28ts2ax39hp8cxq2p0shpgwhzykzha0z6w27gxsps40gurvxjt2vxh26009q5wqqxpj2ey", "amount": { "quantity": 14, "unit": "assets" } } }, { "burn": { "quantity": 14, "unit": "assets" } } ]

i.e. "mint" and "burn" in separate {} ?

Good eye @piotr-iohk, I've fixed this in a6b9895.

Thanks for adding the warning about metadata too!

See new redoc output:

Nice, thanks!

Question: Are multiple mint or burn going to be allowed in single transaction?

"operations": [] is an Array object, which implicates that it will be the case, just want to make sure.
On the other hand there is Array one of in the redoc, so it might be slightly confusing, what do you think? Maybe if there is only one mint or burn allowed they should be inside "operations": {} not "operations": []?

We planned for multiple mint/burn operations in a single transaction. It's something supported by cardano-node and it wasn't hard to allow.

rvl

Sweet

rvl · 2021-07-13T07:24:40Z

lib/core/src/Cardano/Wallet/Api/Types.hs

+    , subject             :: !(ApiT W.TokenFingerprint)
+    -- ^ The subject of the asset minted/burnt. This is useful to users wishing
+    -- to attach metadata to their asset.
+    }


How about including the script in the response? Is that feasible?

I think so. We could return the JSON representation of the script, or the CBOR, but I think JSON is more appropriate?

Addressed in 619ba48.

rvl · 2021-07-13T07:30:19Z

lib/core/src/Cardano/Wallet/Api/Types.hs

+    , assetName           :: !(ApiT W.TokenName)
+    -- ^ The name of the asset to mint/burn.
+    , operations          :: !(NonEmpty (ApiMintBurnOperation n))
+    -- ^ The set of minting and burning operations to perform.


This doesn't seem quite right to me. There could be contradictory elements in the operations list.

What about this?

Suggested change

, assetName :: !(ApiT W.TokenName)

-- ^ The name of the asset to mint/burn.

, operations :: !(NonEmpty (ApiMintBurnOperation n))

-- ^ The set of minting and burning operations to perform.

, assetNames :: !(Map (ApiT W.TokenName) (ApiMintBurnOperation n))

-- ^ A mint/burn amount for assets of the policy script

It's not quite what it seems. The operations contain no reference to what they are operating on (it's either mint 3 or burn 4, there is no reference to the asset).

The caller can choose one monetary policy index, one asset name, then any number of mint/burn operations for that asset.

I should probably allow the user to operate on multiple assets:

data PostMintBurnAssetData (n :: NetworkDiscriminant) = PostMintBurnAssetData - { mintBurn :: !(ApiMintBurnData n) + { mintBurn :: !(NonEmpty (ApiMintBurnData n)) -- ^ Minting and burning request.

And then offer a singular operation for each asset, sorta like it's done in Tx construction:

data ApiMintBurnData (n :: NetworkDiscriminant) = ApiMintBurnData { monetaryPolicyIndex :: !(Maybe (ApiT DerivationIndex)) -- ^ The key derivation index to use to construct the policy. , assetName :: !(ApiT W.TokenName) -- ^ The name of the asset to mint/burn. - , operations :: !(NonEmpty (ApiMintBurnOperation n)) + , operation :: !(ApiMintBurnOperation n) } deriving (Eq, Generic, Show)

I've made this change in e7adcde.

Oh... But if the user is allowed to mint multiple assets in one API call I'll have to rework the response... a sec.

Addressed in 619ba48.

rvl · 2021-07-13T07:33:51Z

specifications/api/swagger.yaml

+
+        **⚠️ WARNING ⚠️**
+
+        Please note that due to the fact that there is no physical access to policy keys


Kinda ... but the user can use cardano-address to derive the policy key themselves according to the forthcoming CIP.

We could also add an endpoint which returns policy keys ... in future.

rvl · 2021-07-13T07:34:35Z

specifications/api/swagger.yaml

+      description: |
+        <p align="right">status: <strong>stable</strong></p>
+
+        Mint and burn assets from the wallet.


It would be worth explaining a little bit about how the script is made -- i.e. how policy keys are chosen.

Slightly improved this with b245fe4.

rvl · 2021-07-13T07:41:32Z

specifications/api/swagger.yaml

+      tags: ["Assets"]
+      summary: Mint/Burn
+      description: |
+        <p align="right">status: <strong>stable</strong></p>


Suggested change

status: stable

status: under development

Addressed in 5780469.

rvl · 2021-07-13T07:45:08Z

lib/core/src/Cardano/Wallet/Api.hs

+-- | https://input-output-hk.github.io/cardano-wallet/api/#operation/mintBurnAssets
+type MintBurnAssets n = "wallets"
+    :> Capture "walletId" (ApiT WalletId)
+    :> "assets"


Did you resolve the overlapping API path issue?
If not, don't fret too much about ugly URLs - because we will soon be rolling PostMintBurnAssetDataT into the transaction creation endpoint.

Yep, resolved overlapping API path issue.

rvl

Good stuff, thanks.

rvl · 2021-07-16T13:02:10Z

I added some minor rework, squashed, and rebased.

bors r+

2712: Mint burn API only, stubbed implementation r=rvl a=sevanspowell # Issue Number ADP-346 ADP-862 # Overview This PR modifies the cardano-wallet API to support minting and burning. Minting and burning are not actually implemented, just stubbed. This was done to minimize the amount of context the reviewer needs to keep in mind when reviewing - we are just reviewing the form of the new minting and burning endpoints. - Add a new "mint" endpoint under the "assets" API group. - Add a number of new API types to represent the data the user must submit with a mint/burn request, and the data they receive back. - Adjust swagger specification to match new API types. - Adds Arbitrary instances for API types for property tests. - Adds a stub for the minting and burning implementation. Co-authored-by: Samuel Evans-Powell <[email protected]>

iohk-bors · 2021-07-16T13:45:41Z

Build failed:

buildkite/cardano-wallet

#expected - hlint warning, doh.

- Add a new "mint" endpoint under the "assets" API group. - Add a number of new API types to represent the data the user must submit with a mint/burn request, and the data they receive back. - Adjust swagger specification to match new API types. - Adds Arbitrary instances for API types for property tests. - Adds a stub for the minting and burning implementation.

rvl · 2021-07-18T08:11:46Z

bors r+

2712: Mint burn API only, stubbed implementation r=rvl a=sevanspowell # Issue Number ADP-346 ADP-862 # Overview This PR modifies the cardano-wallet API to support minting and burning. Minting and burning are not actually implemented, just stubbed. This was done to minimize the amount of context the reviewer needs to keep in mind when reviewing - we are just reviewing the form of the new minting and burning endpoints. - Add a new "mint" endpoint under the "assets" API group. - Add a number of new API types to represent the data the user must submit with a mint/burn request, and the data they receive back. - Adjust swagger specification to match new API types. - Adds Arbitrary instances for API types for property tests. - Adds a stub for the minting and burning implementation. Co-authored-by: Samuel Evans-Powell <[email protected]>

iohk-bors · 2021-07-18T09:23:59Z

Build failed:

buildkite/cardano-wallet

#2695

sevanspowell · 2021-07-19T02:46:24Z

bors r+

iohk-bors · 2021-07-19T03:40:40Z

Build succeeded:

sevanspowell self-assigned this Jun 15, 2021

sevanspowell changed the title ~~Mint burn API 1~~ Mint burn API only, stubbed implementation Jun 15, 2021

sevanspowell force-pushed the sevanspowell/ADP-346/mint-burn-api branch 3 times, most recently from 245872e to c4a2458 Compare June 15, 2021 08:02

sevanspowell commented Jun 15, 2021

View reviewed changes

sevanspowell marked this pull request as ready for review June 15, 2021 08:59

sevanspowell requested a review from jonathanknowles June 16, 2021 01:56

jonathanknowles reviewed Jun 16, 2021

View reviewed changes

piotr-iohk reviewed Jun 17, 2021

View reviewed changes

sevanspowell mentioned this pull request Jun 18, 2021

Modify coin selection algorithm to support minting and burning #2725

Merged

1 task

jonathanknowles force-pushed the sevanspowell/ADP-346/mint-burn-api branch from fb3abfc to 7c0636d Compare June 29, 2021 07:27

sevanspowell force-pushed the sevanspowell/ADP-346/mint-burn-api branch from 529a11b to 6cd32fe Compare July 12, 2021 05:43

rvl suggested changes Jul 13, 2021

View reviewed changes

rvl approved these changes Jul 16, 2021

View reviewed changes

rvl force-pushed the sevanspowell/ADP-346/mint-burn-api branch from 9a21b55 to 9489033 Compare July 16, 2021 12:55

sevanspowell added 2 commits July 18, 2021 16:10

Regenerate Golden JSON files

0cc598d

rvl force-pushed the sevanspowell/ADP-346/mint-burn-api branch from 9489033 to 0cc598d Compare July 18, 2021 08:11

iohk-bors bot merged commit 914668d into master Jul 19, 2021

iohk-bors bot deleted the sevanspowell/ADP-346/mint-burn-api branch July 19, 2021 03:40

WilliamKingNoel-Bot pushed a commit that referenced this pull request Jul 19, 2021

docs: Merge #2712

55d1075

		-- \| https://input-output-hk.github.io/cardano-wallet/api/#operation/mintBurnToken
		type MintBurnAsset n = "wallets"


		⚠️ WARNING ⚠️

		Please note that due to the fact that there is no physical access to policy keys

	<p align="right">status: <strong>stable</strong></p>
	<p align="right">status: <strong>under development</strong></p>

Mint burn API only, stubbed implementation #2712

Mint burn API only, stubbed implementation #2712

Conversation

sevanspowell commented Jun 15, 2021 • edited Loading

Issue Number

Overview

sevanspowell commented Jun 15, 2021

sevanspowell Jun 15, 2021 • 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

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

sevanspowell commented Jun 17, 2021

piotr-iohk commented Jun 17, 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

rvl 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

sevanspowell Jul 14, 2021 • 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

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

rvl left a comment

Choose a reason for hiding this comment

rvl commented Jul 16, 2021

iohk-bors bot commented Jul 16, 2021 • edited by rvl Loading

rvl commented Jul 18, 2021

iohk-bors bot commented Jul 18, 2021 • edited by rvl Loading

sevanspowell commented Jul 19, 2021

iohk-bors bot commented Jul 19, 2021

sevanspowell commented Jun 15, 2021 •

edited

Loading

sevanspowell Jun 15, 2021 •

edited

Loading

sevanspowell Jul 14, 2021 •

edited

Loading

iohk-bors bot commented Jul 16, 2021 •

edited by rvl

Loading

iohk-bors bot commented Jul 18, 2021 •

edited by rvl

Loading