Implement Multi-Asset JSON instances #2092

Jimbo4350 · 2020-11-13T16:57:26Z

Multi-Asset JSON is of the following format:

{
  "fooPolicyId": { "fooAssetName": 42 },
  "barPolicyId": { "barAssetName": 84 },
  "lovelace": 3
}

dcoutts

I can't help thinking there's a simpler approach: do the parse/render on a nested Map representation, and then have two functions for the nested Maps <-> single Map conversions.

The reason the code is complicated is because we're mixing in the flattening/unflattening into the rendering/parsing.

cardano-api/src/Cardano/Api/Value.hs

intricate · 2020-11-17T16:58:13Z

Just a note that this needs to be rebased on master.

dcoutts

I see we've made your life harder here with the change to get rid of the ada policy id and have a distinct ada asset id.

If it were not for that, the following scheme would work:

Scheme 1 (doesn't quite work now with ada asset id)

The starting structure matches the Aeson structure 1:1. There are no clashes any more, so no merges needed. We can also do it all with "bulk" operations rather than folds that insert one by one.

Indeed we could do most of it using the existing ToJSON class instances for Map:

(ToJSON v, ToJSONKey k) => ToJSON (Map k v)

All we need is for PolicyId and AssetId to be in ToJSONKey and Quantity to be in ToJSON.

It'd just be

instance ToJSON Value where
  toJSON = toJSON . unflatten

Scheme 2

So then the question is given ada is represented specially now, how do still get as much as possible for free?

I think the answer is to apply the same principle as before: use an intermedite representation that matches the external JSON representation 1:1

Before we merged the change to the multi-asset Value representation, that intermediate representation was

Map PolicyId (Map AssetName Quantity)

but now that we abolished the ada policy id, it'll have to look a little different, and not a simple type alias for nested maps.

How about this as the intermediate representation:

newtype AssetsBundle = AssetsBundle [AssetIdBundle]
data AssetIdBundle = AssetIdBundle PolicyId (Map AssetName Quantity)
                    | AdaAsset Quantity

and then we make these instances of ToJSON/FromJSON and adapt the flatten/unflatten that converts to/from the multi-asset Value type.

instance ToJSON AssetsBundle where
  toJSON (AssetsBundle xss) = Aeson.object (map toPair xss)
    where
      toPair (AdaAsset q) = ("lovelace", toJSON q)
      toPair (AssetIdBundle pid xs) = (renderPolicyId pid, toJSON xs)

cardano-api/src/Cardano/Api/Typed.hs

cardano-api/src/Cardano/Api/Value.hs

dcoutts

Yes much nicer.

One can tell we've got something structurally better from the fact that we now have just

instance ToJSON Value where
  toJSON = toJSON . unflatten

instance FromJSON Value where
  parseJSON jv = flatten <$> parseJSON jv

Name bikeshedding: yes I suggested the asset bundle names. That doesn't mean I'm happy with my own suggestion 😁. If we are forced to export it for testing purposes, then a name is more important. Perhaps NestedValue, to indicate it's equivalent to Value but has a nested representation. And then for the inner one, what? NestedValueAssets? 😬

cardano-api/src/Cardano/Api/Value.hs

dcoutts · 2020-11-20T16:27:31Z

cardano-api/src/Cardano/Api/Value.hs

@@ -115,6 +131,98 @@ instance Semigroup Value where
 instance Monoid Value where
  mempty = Value Map.empty

+instance ToJSON Value where
+  toJSON = toJSON . unflatten


cardano-api/src/Cardano/Api/Value.hs

dcoutts · 2020-11-23T10:32:25Z

Sorry, I think we both did the last bit of name changing at the same time.

dcoutts

Ok, lets squash into a single patch and merge.

dcoutts · 2020-11-23T15:30:39Z

bors merge

2092: Implement Multi-Asset JSON instances r=dcoutts a=Jimbo4350 Multi-Asset JSON is of the following format: ```json { "fooPolicyId": { "fooAssetName": 42 }, "barPolicyId": { "barAssetName": 84 }, "lovelace": 3 } ``` Co-authored-by: Jordan Millar <[email protected]>

iohk-bors · 2020-11-23T17:50:42Z

Timed out.

dcoutts · 2020-11-23T18:52:50Z

https://hydra.iohk.io/build/4907903/nixlog/1


             ┏━━ test/Test/Cardano/Api/Typed/Value.hs ━━━
          35 ┃ prop_roundtrip_Value_unflatten_flatten :: Property
          36 ┃ prop_roundtrip_Value_unflatten_flatten =
          37 ┃     property $ do
          38 ┃       v <- forAll genValueNestedRep
             ┃       │ ValueNestedRep [ ValueNestedBundleAda 0 ]
          39 ┃       let v' = valueToNestedRep (valueFromNestedRep v)
          40 ┃       v `equiv` v'
          41 ┃   where
          42 ┃     equiv (ValueNestedRep a) (ValueNestedRep b) = sort a === sort b
             ┃     ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
             ┃     │ ━━━ Failed (- lhs) (+ rhs) ━━━
             ┃     │ - [ ValueNestedBundleAda 0 ]
             ┃     │ + []

Ok, this is a (minor) problem. The generator is producing nested rep values with zeros for the ada case. That's not a canonical representation.

Either we should canonicalise the nested rep, or we should generate only canonical reps.

I think the better solution is to canonicalise only the original rep and then we just use

canonicalise v === valueToNestedRep (valueFromNestedRep v)

The canonicalise would eliminate zero quantities, merge (sum) duplicates, and put the entries in order.

dcoutts · 2020-11-24T10:53:07Z

cardano-api/src/Cardano/Api/Value.hs

@@ -211,7 +212,7 @@ data ValueNestedBundle = ValueNestedBundle PolicyId (Map AssetName Quantity)
 valueToNestedRep :: Value -> ValueNestedRep
 valueToNestedRep v =
    -- unflatten all the non-ada assets, and add ada separately
-    ValueNestedRep $
+    ValueNestedRep . sort $


Ah! This is why in the other branch I have swapped the order of the AssetId constructors, so that this sort would not be necessary.

844dde7

Perhaps we should include that swap here in this PR, then we don't need the sort.

dcoutts · 2020-11-24T11:00:55Z

cardano-api/test/Test/Cardano/Api/Typed/Value.hs

+  removeZeroAda :: ValueNestedBundle -> Bool
+  removeZeroAda (ValueNestedBundleAda 0) = False
+  removeZeroAda (ValueNestedBundle _ aNameQmap) =
+    0 `notElem` Map.elems aNameQmap
+  removeZeroAda _ = True


This is a filter predicate not a filter itself, so call it something like filter (not . isZeroAda)

But also, filtering at this level cannot work. We need to filter out the zero quantities in the nested bundles, and omit the ada if it itself is zero.

If the test did not catch this, it shows we don't have well-tuned generators. I think we should adjust the Value and ValueNestedRep so 1 out of 10 times it samples from the full range, and 9 out of 10 times from a very small (e.g. 5) range of policy ids (A..E), asset names (A..E) and quantities (-2..2). That will mean we frequently get asset names with the same policy id, and quantities that sum to 0.

dcoutts · 2020-11-24T11:05:59Z

cardano-api/test/Test/Cardano/Api/Typed/Value.hs

+        flattenedMinted = map (uncurry ValueNestedBundle) $ Map.toList summedMinted
+        flattenedAda = ValueNestedBundleAda summedAda
+
+    in sort $ flattenedAda : flattenedMinted


We'll be able to get rid of this sort for the same reasons above.

dcoutts · 2020-11-24T11:07:42Z

cardano-api/test/Test/Cardano/Api/Typed/Value.hs

+  ValueNestedRep . filter removeZeroAda $ mergeDuplicates bundles
+ where
+  mergeDuplicates :: [ValueNestedBundle] -> [ValueNestedBundle]
+  mergeDuplicates l =
+    let allAssets :: [(PolicyId, Map AssetName Quantity)]
+        allAssets = [(pId, qMap) | (ValueNestedBundle pId qMap) <- l]
+
+        allAda :: [Quantity]
+        allAda = [ada | ValueNestedBundleAda ada <- l]
+
+        summedMinted :: Map PolicyId (Map AssetName Quantity)
+        summedMinted = Map.fromListWith (Map.unionWith (<>)) allAssets
+
+        summedAda :: Quantity
+        summedAda = sum allAda
+
+        flattenedMinted = map (uncurry ValueNestedBundle) $ Map.toList summedMinted
+        flattenedAda = ValueNestedBundleAda summedAda


I think we can probably write this more concisely, but lets solve the sort & generator issues first.

dcoutts

I've rebased on master and adjusted the second patch slightly with a (hopefully) clearer and simpler version of the canonicalise function. This required adjusting the order of the ValueNestedBundle constructors to match the AssetId sort order.

intricate · 2020-11-25T19:06:28Z

cardano-api/test/Test/Cardano/Api/Typed/Gen.hs

@@ -149,6 +153,74 @@ genMultiSigScriptsMary =

    ]

+genAssetName :: Gen AssetName
+genAssetName = AssetName <$> Gen.utf8 (Range.constant 1 15) Gen.alphaNum


Suggested change

genAssetName = AssetName <$> Gen.utf8 (Range.constant 1 15) Gen.alphaNum

genAssetName = AssetName <$> Gen.utf8 (Range.constant 1 32) Gen.alphaNum

dcoutts · 2020-11-25T20:58:03Z

bors merge

2092: Implement Multi-Asset JSON instances r=dcoutts a=Jimbo4350 Multi-Asset JSON is of the following format: ```json { "fooPolicyId": { "fooAssetName": 42 }, "barPolicyId": { "barAssetName": 84 }, "lovelace": 3 } ``` 2126: Updated help text for query ledger/protocol-state r=intricate a=kevinhammond Clarified that these commands are not intended for general use, and pointed to the underlying data structures for more information about content Co-authored-by: Jordan Millar <[email protected]> Co-authored-by: Kevin Hammond <[email protected]>

iohk-bors · 2020-11-25T21:59:53Z

Build failed (retrying...):

ci/hydra-build:required

2092: Implement Multi-Asset JSON instances r=dcoutts a=Jimbo4350 Multi-Asset JSON is of the following format: ```json { "fooPolicyId": { "fooAssetName": 42 }, "barPolicyId": { "barAssetName": 84 }, "lovelace": 3 } ``` Co-authored-by: Jordan Millar <[email protected]>

iohk-bors · 2020-11-25T22:19:15Z

Canceled.

dcoutts · 2020-11-25T22:19:30Z

bors merge

2092: Implement Multi-Asset JSON instances r=dcoutts a=Jimbo4350 Multi-Asset JSON is of the following format: ```json { "fooPolicyId": { "fooAssetName": 42 }, "barPolicyId": { "barAssetName": 84 }, "lovelace": 3 } ``` 2126: Updated help text for query ledger/protocol-state r=intricate a=kevinhammond Clarified that these commands are not intended for general use, and pointed to the underlying data structures for more information about content Co-authored-by: Jordan Millar <[email protected]> Co-authored-by: Kevin Hammond <[email protected]>

iohk-bors · 2020-11-26T00:20:09Z

Build failed (retrying...):

ci/hydra-build:required

iohk-bors · 2020-11-26T01:21:14Z

Canceled.

Add round-trip tests. Improve the generators for Value and related types: sample the PolicyId and AssetName from a smaller range of values so that for things like Value we get more "interesting" values with duplicate policy ids or asset names. Add a canonicalise function for ValueNestedRep which should be the equivalent of converting from ValueNestedRep to Value and back.

dcoutts · 2020-11-26T04:14:42Z

bors merge

iohk-bors · 2020-11-26T06:31:03Z

Build succeeded:

dcoutts reviewed Nov 13, 2020

View reviewed changes

Jimbo4350 marked this pull request as ready for review November 17, 2020 16:10

Jimbo4350 requested review from erikd, intricate, mrBliss and newhoggy as code owners November 17, 2020 16:10

Jimbo4350 force-pushed the jordan/ma-json-syntax branch 2 times, most recently from c3883f5 to 5ff3a8f Compare November 18, 2020 08:41

Jimbo4350 changed the title ~~Implement Multi-Asset JSON syntax~~ Implement Multi-Asset JSON instances Nov 18, 2020

dcoutts requested changes Nov 18, 2020

View reviewed changes

Jimbo4350 force-pushed the jordan/ma-json-syntax branch 4 times, most recently from 42d3872 to 3ee99fa Compare November 20, 2020 16:24

dcoutts reviewed Nov 20, 2020

View reviewed changes

Jimbo4350 force-pushed the jordan/ma-json-syntax branch 2 times, most recently from f9e9545 to 1c07404 Compare November 23, 2020 10:22

dcoutts approved these changes Nov 23, 2020

View reviewed changes

Jimbo4350 force-pushed the jordan/ma-json-syntax branch from cda4ba0 to 9a6d588 Compare November 23, 2020 15:07

dcoutts requested changes Nov 24, 2020

View reviewed changes

Jimbo4350 force-pushed the jordan/ma-json-syntax branch 4 times, most recently from 3c891f7 to 743e4e2 Compare November 24, 2020 17:06

dcoutts force-pushed the jordan/ma-json-syntax branch from 743e4e2 to c1d3beb Compare November 25, 2020 17:37

dcoutts approved these changes Nov 25, 2020

View reviewed changes

intricate reviewed Nov 25, 2020

View reviewed changes

intricate approved these changes Nov 25, 2020

View reviewed changes

dcoutts force-pushed the jordan/ma-json-syntax branch from c1d3beb to e7334bd Compare November 25, 2020 20:57

dcoutts force-pushed the jordan/ma-json-syntax branch from e7334bd to 142d139 Compare November 25, 2020 22:19

dcoutts force-pushed the jordan/ma-json-syntax branch from 142d139 to 8390bf7 Compare November 26, 2020 01:21

dcoutts force-pushed the jordan/ma-json-syntax branch from 8390bf7 to 89f8374 Compare November 26, 2020 04:13

iohk-bors bot merged commit 16d8e8e into master Nov 26, 2020

iohk-bors bot deleted the jordan/ma-json-syntax branch November 26, 2020 06:31

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Implement Multi-Asset JSON instances #2092

Implement Multi-Asset JSON instances #2092

Jimbo4350 commented Nov 13, 2020 •

edited

Loading

dcoutts left a comment

intricate commented Nov 17, 2020

dcoutts left a comment

dcoutts left a comment

dcoutts Nov 20, 2020

dcoutts commented Nov 23, 2020

dcoutts left a comment

dcoutts commented Nov 23, 2020

iohk-bors bot commented Nov 23, 2020

dcoutts commented Nov 23, 2020

dcoutts Nov 24, 2020

dcoutts Nov 24, 2020

dcoutts Nov 24, 2020

dcoutts Nov 24, 2020

dcoutts left a comment

intricate Nov 25, 2020

dcoutts commented Nov 25, 2020

iohk-bors bot commented Nov 25, 2020

iohk-bors bot commented Nov 25, 2020

dcoutts commented Nov 25, 2020

iohk-bors bot commented Nov 26, 2020

iohk-bors bot commented Nov 26, 2020

dcoutts commented Nov 26, 2020

iohk-bors bot commented Nov 26, 2020

	genAssetName = AssetName <$> Gen.utf8 (Range.constant 1 15) Gen.alphaNum
	genAssetName = AssetName <$> Gen.utf8 (Range.constant 1 32) Gen.alphaNum

Implement Multi-Asset JSON instances #2092

Implement Multi-Asset JSON instances #2092

Conversation

Jimbo4350 commented Nov 13, 2020 • edited Loading

dcoutts left a comment

Choose a reason for hiding this comment

intricate commented Nov 17, 2020

dcoutts left a comment

Choose a reason for hiding this comment

Scheme 1 (doesn't quite work now with ada asset id)

Scheme 2

dcoutts left a comment

Choose a reason for hiding this comment

dcoutts Nov 20, 2020

Choose a reason for hiding this comment

dcoutts commented Nov 23, 2020

dcoutts left a comment

Choose a reason for hiding this comment

dcoutts commented Nov 23, 2020

iohk-bors bot commented Nov 23, 2020

dcoutts commented Nov 23, 2020

dcoutts Nov 24, 2020

Choose a reason for hiding this comment

dcoutts Nov 24, 2020

Choose a reason for hiding this comment

dcoutts Nov 24, 2020

Choose a reason for hiding this comment

dcoutts Nov 24, 2020

Choose a reason for hiding this comment

dcoutts left a comment

Choose a reason for hiding this comment

intricate Nov 25, 2020

Choose a reason for hiding this comment

dcoutts commented Nov 25, 2020

iohk-bors bot commented Nov 25, 2020

iohk-bors bot commented Nov 25, 2020

dcoutts commented Nov 25, 2020

iohk-bors bot commented Nov 26, 2020

iohk-bors bot commented Nov 26, 2020

dcoutts commented Nov 26, 2020

iohk-bors bot commented Nov 26, 2020

Jimbo4350 commented Nov 13, 2020 •

edited

Loading