Multi-asset API extensions #2447

rvl · 2021-01-14T09:22:49Z

Issue Number

ADP-604

Overview

Starting implementation of API spec in #2446.

listAssets endpoint
getAsset endpoint
Added ApiWalletAssetsBalance which provides the balance of non-ade assets.
Removed WalletBalance from Primitive.Types and replaced ApiT WalletBalance with ApiWalletBalance.
Some kind of integration test for these endpoints.
Remove Quantity "lovelace" a where possible, except for the API.
TokenName inner type is now ByteString, add length validation.

Comments

This branch is based on top of the PR API spec: First revision of multi-asset changes #2446 branch.

KtorZ · 2021-01-14T12:41:07Z

lib/core/src/Cardano/Wallet.hs

@@ -1905,7 +1905,10 @@ mkTxMeta ti' blockHeader wState tx cs expiry =
                , direction = if amtInps > amtOuts then Outgoing else Incoming
                , slotNo = blockHeader ^. #slotNo
                , blockHeight = blockHeader ^. #blockHeight
-                , amount = Quantity $ distance amtInps amtOuts
+                -- fixme: ADP-347 calculate asset balance too


The amount should remain unchanged and ada-only. We will not be providing this information for assets. It is less useful for assets since fees can only be paid in Ada. What assets are being sent can be figured out from the resolved inputs and outputs fully.

OK then. Nonetheless I thought it would be good to calculate a total of assets moved in or out of the wallet by the transaction. In mkTxMeta it's easier to know which tx outputs are "ours". Resolved inputs and outputs show the address only.

This feature is tracked by ADP-683.

KtorZ · 2021-01-14T12:46:07Z

lib/core/src/Cardano/Wallet/Primitive/Model.hs

@@ -376,7 +378,9 @@ prefilterBlock b u0 = runState $ do
        , direction = dir
        , slotNo = b ^. #header . #slotNo
        , blockHeight = b ^. #header . #blockHeight
-        , amount = Quantity amt
+        -- fixme: ADP-347
+        -- fixme: why on earth do we have both Coin and Quantity "lovelace" Natural?


At the very beginning, the Quantity type was only used in the Api as a way to generate pretty JSON. It has one day escaped its cage and spread over the rest of the codebase as a quick replacement for newtypes over numeric types.. So in some places we have Coin, in some Word64, in some Quantity "lovelace" Natural or Quantity "lovelace" Word64 or also Fee which is very much the same thing as Coin.

lib/core/src/Cardano/Wallet/Primitive/Types/Tx.hs

jonathanknowles · 2021-01-18T09:38:20Z

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

+instance FromJSON (ApiT W.TokenMap) where
+    parseJSON = fmap (ApiT . W.getFlat) . parseJSON
+instance ToJSON (ApiT W.TokenMap) where
+    toJSON = toJSON . W.Flat . getApiT


Hi @rvl

I'm just curious about the reason for choosing Flat here. Is there a reason to not use Nested?

(From recollection, the Daedalus team indicated that they preferred a nested representation, where assets were grouped by token policy.)

Apologies if I'm missing some context here which would make the reason for this choice obvious.

Nesting makes it more complicated to process. The fact that there's a hierarchy policy_id -> asset_name is actually an implementation detail. End-users care about tokens, and tokens are uniquely identified by a pair (policy_id, asset_name). Tokens with different names are different which is why in the end, a flat views of assets makes makes more sense in the interface.

Best place to comment on the API would be #2446. But it's one line of javascript to convert between flat and nested. EIther way would probably be fine.

jonathanknowles · 2021-01-18T09:44:39Z

lib/core/src/Cardano/Wallet/Primitive/Types/TokenPolicy.hs

@@ -81,3 +83,11 @@ instance ToText TokenName where

 instance FromText TokenName where
    fromText = pure . UnsafeTokenName


Related question: are we planning to change the type of TokenName from Text to ByteString (or perhaps ShortByteString), given that it can be any sequence of bytes between 0 bytes and 32 bytes in length (and therefore might not be representable by Text).

Of course, because Text is the wrong type.

lib/core/src/Cardano/Wallet/Primitive/Types/Tx.hs

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

jonathanknowles · 2021-01-18T09:48:50Z

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

@@ -2044,6 +2084,7 @@ mkApiTransaction ti txid mfee ins outs ws (meta, timestamp) txMeta setTimeRefere
        , inputs = [ApiTxInput (fmap toAddressAmount o) (ApiT i) | (i, o) <- ins]
        , outputs = toAddressAmount <$> outs
        , withdrawals = mkApiWithdrawal @n <$> Map.toList ws
+        , forge = mempty  -- fixme: ADP-604


Question: Should this be mint? And do we need to support this particular field as part of ADP-604?

Suggested change

, forge = mempty -- fixme: ADP-604

, mint = mempty -- fixme: ADP-604

Ok thanks - i'll use "mint" to match the terminology from Cardano.Api.
The API field is a placeholder to help Daedalus team with integration.

The correct place to review these names is in #2446 - because ApiTransaction just mirrors what's in swagger.yaml.

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

KtorZ · 2021-01-20T07:39:38Z

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

+    -> Handler [ApiAsset]
+listAssets ctx (ApiT wid) =
+    Handler $ ExceptT $ withDatabase df wid $ \db -> runHandler $ do
+        let wrk = hoistResource db (MsgFromWorker wid) ctx


Use withWorkerCtx instead:

withWorkerCtx @_ @s @k ctx wid liftE liftE $ \wrk -> liftHandler $ W.readWallet @_ @s @k wrk wid

withWorkerCtx comes with some error handling that is common to all wallets accesses.

Thanks, that's better.

KtorZ · 2021-01-20T07:45:31Z

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

+    Nothing -> case T.decodeUtf8' assetNameBytes of
+        Right "" -> hexText $ getHash $ W.unTokenPolicyId pid
+        Right text -> text
+        Left _ -> hexText assetNameBytes


I am not sure to get the rationale for the Nothing branch in this function. The assetName isn't something that is supposed to be user-facing or displayed to user.

I'd suggest to leave it null if there's no metadata (that is, simply leave it as metadata.name) or, to go with your first idea in the cryptpad spec: a bech32 encoded string comprised of the policyId | assetName, possibly hashed. Though, it's not really anymore displayable than the policyId and assetName.

Ok I have removed displayName for the time being.

KtorZ · 2021-01-20T07:48:20Z

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

+        AddressAmount
+            <$> v .: "address"
+            <*> (v .: "amount" >>= validateCoin)
+            <*> v .:? "assets" .!= mempty


KtorZ · 2021-01-20T07:51:47Z

lib/core/src/Cardano/Wallet/Primitive/Model.hs

        { status = InLedger
        , direction = dir
        , slotNo = b ^. #header . #slotNo
        , blockHeight = b ^. #header . #blockHeight
-        , amount = Quantity amt
+        , amount = amount


This should be okay-ish for Ada / Coin because we know that the max supply is 42B. But we should generally be careful when summing token quantities. Even if each token individually fit in a Word64, their sum might not. So for anything that regards folds or sums, we should stick to Natural IMO.

At the very least, if changing this to Coin, I'd like to leave a note in the code about exactly this.

I have added a note to Coin.hs.

In Model.hs, any sums are limited to a single transaction, so it's all fine for ada amounts, and token amounts use Natural.

I would not be against changing newtype Coin to use Natural or Integer, just to remove all doubt about overflow.

This should be removed once PR #2447 is merged.

rvl · 2021-01-25T09:31:43Z

Rebased and squashed...

bors r+

2447: Multi-asset API extensions r=rvl a=rvl ### Issue Number ADP-604 ### Overview Starting implementation of API spec in #2446. - [x] listAssets endpoint - [x] getAsset endpoint - [x] Added `ApiWalletAssetsBalance` which provides the balance of non-ade assets. - [x] Removed WalletBalance from Primitive.Types and replaced `ApiT WalletBalance` with `ApiWalletBalance`. - [ ] Some kind of integration test for these endpoints. - [x] Remove `Quantity "lovelace" a` where possible, except for the API. - [x] TokenName inner type is now ByteString, add length validation. ### Comments - This branch is based on top of the PR #2446 branch. Co-authored-by: KtorZ <matthias.benkort@gmail.com> Co-authored-by: Rodney Lorrimar <rodney.lorrimar@iohk.io>

iohk-bors · 2021-01-25T10:01:55Z

Build failed:

ci/hydra-build:required

Failures:

  src/Test/Integration/Scenario/API/Shelley/StakePools.hs:419:5: 
  1) API Specifications, SHELLEY_STAKE_POOLS, STAKE_POOLS_JOIN_02 - Cannot join already joined stake pool
       uncaught exception: RequestException
       DecodeFailure "{\"code\":\"network_query_failed\",\"message\":\"Unable to query the ledger at the moment. This error has been logged. Trying again in a bit might work.\"}"

  To rerun use: --match "/API Specifications/SHELLEY_STAKE_POOLS/STAKE_POOLS_JOIN_02 - Cannot join already joined stake pool/"

Randomized with seed 590232999

Finished in 998.9463 seconds
711 examples, 1 failure, 12 pending
builder for '/nix/store/6i117xq699x522a2z46950f1qn23425z-cardano-wallet-test-integration-2021.1.12-x86_64-unknown-linux-musl-check-x86_64-unknown-linux-musl.drv' failed with exit code 1

#2320

Anviking · 2021-01-25T11:11:37Z

bors r+

iohk-bors · 2021-01-25T11:26:23Z

Merge conflict.

Some reordering required to please the openapi-spec-validator.

Anviking · 2021-01-25T11:56:34Z

rebased

bors r+

2447: Multi-asset API extensions r=Anviking a=rvl ### Issue Number ADP-604 ### Overview Starting implementation of API spec in #2446. - [x] listAssets endpoint - [x] getAsset endpoint - [x] Added `ApiWalletAssetsBalance` which provides the balance of non-ade assets. - [x] Removed WalletBalance from Primitive.Types and replaced `ApiT WalletBalance` with `ApiWalletBalance`. - [ ] Some kind of integration test for these endpoints. - [x] Remove `Quantity "lovelace" a` where possible, except for the API. - [x] TokenName inner type is now ByteString, add length validation. ### Comments - This branch is based on top of the PR #2446 branch. Co-authored-by: KtorZ <matthias.benkort@gmail.com> Co-authored-by: Rodney Lorrimar <rodney.lorrimar@iohk.io>

This should be removed once PR #2447 is merged.

iohk-bors · 2021-01-25T12:51:18Z

Build failed:

ci/hydra-build:required

Failures:

  src/Test/Integration/Framework/DSL.hs:780:11:
  1) API Specifications, SHELLEY_TRANSACTIONS, TRANS_ESTIMATE_03b - we see result when we can't cover fee (with withdrawal)
       0 does not satisfy (> 0)
       While verifying (Status {statusCode = 200, statusMessage = "OK"},Right (ApiWallet {id = ApiT {getApiT = WalletId {getWalletId = e262ca635d5bafdeaeaf2c58f84027cf38e9c1e6}}, addressPoolGap = ApiT {getApiT = AddressPoolGap {getAddressPoolGap = 20}}, balance = ApiWalletBalance {available = Quantity {getQuantity = 100000000000}, total = Quantity {getQuantity = 100000000000}, reward = Quantity {getQuantity = 0}}, assets = ApiWalletAssetsBalance {available = ApiT {getApiT = TokenMap (fromList [])}, total = ApiT {getApiT = TokenMap (fromList [])}}, delegation = ApiWalletDelegation {active = ApiWalletDelegationNext {status = NotDelegating, target = Nothing, changesAt = Nothing}, next = []}, name = ApiT {getApiT = WalletName {getWalletName = "MIR Wallet"}}, passphrase = Just (ApiWalletPassphraseInfo {lastUpdatedAt = 2021-01-25 12:31:12.807534 UTC}), state = ApiT {getApiT = Ready}, tip = ApiBlockReference {absoluteSlotNumber = ApiT {getApiT = SlotNo 2899}, slotId = ApiSlotId {epochNumber = ApiT {getApiT = EpochNo {unEpochNo = 57}}, slotNumber = ApiT {getApiT = SlotInEpoch {unSlotInEpoch = 49}}}, time = 2021-01-25 12:32:50.8 UTC, block = ApiBlockInfo {height = Quantity {getQuantity = 1399}}}}))
       Waited longer than 90s to resolve action: "MIR wallet: wallet is 100% synced ".

  To rerun use: --match "/API Specifications/SHELLEY_TRANSACTIONS/TRANS_ESTIMATE_03b - we see result when we can't cover fee (with withdrawal)/"

  src/Test/Integration/Framework/DSL.hs:780:11:
  2) API Specifications, SHELLEY_TRANSACTIONS, SHELLEY_TX_REDEEM_01 - Can redeem rewards from self
       0 does not satisfy (> 0)
       While verifying (Status {statusCode = 200, statusMessage = "OK"},Right (ApiWallet {id = ApiT {getApiT = WalletId {getWalletId = 6a992466e5ec9d721993925a313e7898bc076e87}}, addressPoolGap = ApiT {getApiT = AddressPoolGap {getAddressPoolGap = 20}}, balance = ApiWalletBalance {available = Quantity {getQuantity = 100000000000}, total = Quantity {getQuantity = 100000000000}, reward = Quantity {getQuantity = 0}}, assets = ApiWalletAssetsBalance {available = ApiT {getApiT = TokenMap (fromList [])}, total = ApiT {getApiT = TokenMap (fromList [])}}, delegation = ApiWalletDelegation {active = ApiWalletDelegationNext {status = NotDelegating, target = Nothing, changesAt = Nothing}, next = []}, name = ApiT {getApiT = WalletName {getWalletName = "MIR Wallet"}}, passphrase = Just (ApiWalletPassphraseInfo {lastUpdatedAt = 2021-01-25 12:32:25.326288 UTC}), state = ApiT {getApiT = Ready}, tip = ApiBlockReference {absoluteSlotNumber = ApiT {getApiT = SlotNo 3251}, slotId = ApiSlotId {epochNumber = ApiT {getApiT = EpochNo {unEpochNo = 65}}, slotNumber = ApiT {getApiT = SlotInEpoch {unSlotInEpoch = 1}}}, time = 2021-01-25 12:34:01.2 UTC, block = ApiBlockInfo {height = Quantity {getQuantity = 1570}}}}))
       Waited longer than 90s to resolve action: "MIR wallet: wallet is 100% synced ".

Think this is reward-related:

#2467

Anviking · 2021-01-25T12:52:54Z

bors r+

2447: Multi-asset API extensions r=Anviking a=rvl ### Issue Number ADP-604 ### Overview Starting implementation of API spec in #2446. - [x] listAssets endpoint - [x] getAsset endpoint - [x] Added `ApiWalletAssetsBalance` which provides the balance of non-ade assets. - [x] Removed WalletBalance from Primitive.Types and replaced `ApiT WalletBalance` with `ApiWalletBalance`. - [ ] Some kind of integration test for these endpoints. - [x] Remove `Quantity "lovelace" a` where possible, except for the API. - [x] TokenName inner type is now ByteString, add length validation. ### Comments - This branch is based on top of the PR #2446 branch. Co-authored-by: KtorZ <matthias.benkort@gmail.com> Co-authored-by: Rodney Lorrimar <rodney.lorrimar@iohk.io>

iohk-bors · 2021-01-25T13:26:31Z

Build failed:

ci/hydra-build:required

  test/unit/Cardano/Wallet/DB/StateMachine.hs:1329:28:
  1) Cardano.Wallet.DB.Sqlite, Validate generators & shrinkers, Shrinker for CreateWallet
       Timed out.

#2393

KtorZ · 2021-01-25T13:31:03Z

bors retry

iohk-bors · 2021-01-25T14:08:01Z

Build succeeded:

rvl added the ADDING FEATURE Mark a PR as adding a new feature, for auto-generated CHANGELOG label Jan 14, 2021

rvl self-assigned this Jan 14, 2021

rvl force-pushed the rvl/adp-604/api-ma branch from e3a685a to 3611a2d Compare January 14, 2021 09:23

KtorZ reviewed Jan 14, 2021

View reviewed changes

lib/core/src/Cardano/Wallet/Primitive/Types/Tx.hs Outdated Show resolved Hide resolved

rvl mentioned this pull request Jan 15, 2021

API spec: First revision of multi-asset changes #2446

Closed

jonathanknowles reviewed Jan 18, 2021

View reviewed changes

lib/core/src/Cardano/Wallet/Primitive/Types/Tx.hs Show resolved Hide resolved

jonathanknowles reviewed Jan 18, 2021

View reviewed changes

lib/core/src/Cardano/Wallet/Api.hs Outdated Show resolved Hide resolved

jonathanknowles reviewed Jan 18, 2021

View reviewed changes

lib/core/src/Cardano/Wallet/Api/Types.hs Outdated Show resolved Hide resolved

rvl force-pushed the rvl/adp-604/api-ma branch 2 times, most recently from f4c5489 to fc15718 Compare January 19, 2021 13:01

KtorZ reviewed Jan 20, 2021

View reviewed changes

rvl force-pushed the rvl/adp-604/api-ma branch 2 times, most recently from f3d8bef to fc978b5 Compare January 22, 2021 22:36

jonathanknowles added a commit that referenced this pull request Jan 25, 2021

TEMPORARY WORKAROUND: Make TokenName a ByteString internally.

ade7870

This should be removed once PR #2447 is merged.

KtorZ approved these changes Jan 25, 2021

View reviewed changes

rvl force-pushed the rvl/adp-604/api-ma branch from fc978b5 to 2fbfaab Compare January 25, 2021 08:57

rvl marked this pull request as ready for review January 25, 2021 08:59

KtorZ and others added 12 commits January 25, 2021 12:45

swagger: First revision of multi-asset API

e7d51f2

swagger: Multi-asset API extensions

d6c3f4b

Some reordering required to please the openapi-spec-validator.

swagger: Update asset metadata schema

dc7de32

swagger: Try to do something about getting assets with empty names

516081a

swagger: remove display_name field

e182a92

API: Add assets to wallet balance

70376d3

Replace Coin with Quantity "lovelace" in a few places

5d0ce40

Add note about summing to Coin.hs

c30098b

Change TokenName to be a ByteString

6f2bcd6

Update golden test data for API specs

6b0a346

Remove putStrLn from integration tests

f110c84

Verify asset balances are zero in e2e tests

af2ce54

Anviking force-pushed the rvl/adp-604/api-ma branch from 2fbfaab to af2ce54 Compare January 25, 2021 11:55

jonathanknowles added a commit that referenced this pull request Jan 25, 2021

TEMPORARY WORKAROUND: Make TokenName a ByteString internally.

a9044eb

This should be removed once PR #2447 is merged.

jonathanknowles mentioned this pull request Jan 25, 2021

Add function computeMinimumAdaQuantity. #2461

Merged

iohk-bors bot merged commit df25b2c into master Jan 25, 2021

iohk-bors bot deleted the rvl/adp-604/api-ma branch January 25, 2021 14:08

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Multi-asset API extensions #2447

Multi-asset API extensions #2447

rvl commented Jan 14, 2021 •

edited

KtorZ Jan 14, 2021

rvl Jan 15, 2021

rvl Feb 19, 2021

KtorZ Jan 14, 2021

jonathanknowles Jan 18, 2021 •

edited

KtorZ Jan 18, 2021

rvl Jan 18, 2021

jonathanknowles Jan 18, 2021

rvl Jan 18, 2021

rvl Jan 19, 2021

jonathanknowles Jan 18, 2021

rvl Jan 19, 2021

KtorZ Jan 20, 2021

KtorZ Jan 20, 2021

rvl Jan 22, 2021

KtorZ Jan 20, 2021

rvl Jan 22, 2021

KtorZ Jan 20, 2021

KtorZ Jan 20, 2021 •

edited

KtorZ Jan 20, 2021

rvl Jan 22, 2021

rvl commented Jan 25, 2021

iohk-bors bot commented Jan 25, 2021 •

edited by rvl

Anviking commented Jan 25, 2021

iohk-bors bot commented Jan 25, 2021

Anviking commented Jan 25, 2021

iohk-bors bot commented Jan 25, 2021 •

edited by Anviking

Anviking commented Jan 25, 2021

iohk-bors bot commented Jan 25, 2021 •

edited by KtorZ

KtorZ commented Jan 25, 2021

iohk-bors bot commented Jan 25, 2021

		@@ -81,3 +83,11 @@ instance ToText TokenName where

		instance FromText TokenName where
		fromText = pure . UnsafeTokenName

	, forge = mempty -- fixme: ADP-604
	, mint = mempty -- fixme: ADP-604

Multi-asset API extensions #2447

Multi-asset API extensions #2447

Conversation

rvl commented Jan 14, 2021 • edited

Issue Number

Overview

Comments

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 Jan 18, 2021 • edited

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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

KtorZ Jan 20, 2021 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

rvl commented Jan 25, 2021

iohk-bors bot commented Jan 25, 2021 • edited by rvl

Anviking commented Jan 25, 2021

iohk-bors bot commented Jan 25, 2021

Anviking commented Jan 25, 2021

iohk-bors bot commented Jan 25, 2021 • edited by Anviking

Anviking commented Jan 25, 2021

iohk-bors bot commented Jan 25, 2021 • edited by KtorZ

KtorZ commented Jan 25, 2021

iohk-bors bot commented Jan 25, 2021

rvl commented Jan 14, 2021 •

edited

jonathanknowles Jan 18, 2021 •

edited

KtorZ Jan 20, 2021 •

edited

iohk-bors bot commented Jan 25, 2021 •

edited by rvl

iohk-bors bot commented Jan 25, 2021 •

edited by Anviking

iohk-bors bot commented Jan 25, 2021 •

edited by KtorZ