cardano-foundation / cardano-wallet

HTTP server & command-line for managing UTxOs and HD wallets in Cardano.
Apache License 2.0
755 stars 213 forks source link

🐞 Bug: JSON roundtrip property broken for `TxMetadataWithSchema`. #4647

Open jonathanknowles opened 1 week ago

jonathanknowles commented 1 week ago

Version

fae66a56f33ab7497c19830d3fcf4803173691f4

Description

The JSON encoding roundtrip property is not always satisfied for the TxMetadataWithSchema type.

Here's a PR that demonstrates the failure:

Why is this a bug?

There is a long-standing convention that types used in the HTTP API should satisfy the following JSON round trip property:

decode . encode == id

jonathanknowles commented 1 week ago

This appears to be specific to the TxMetadataNoSchema option, defined within the Cardano.Api.TxMetadata module of the cardano-api package.

Given the following import:

> import Cardano.Api.TxMetadata

And the following metadata:

> m =
    TxMetadata
      { unTxMetadata = fromList
        [ ( 0
          , TxMetaMap
            [ ( TxMetaText "k", TxMetaText "v1" )
            , ( TxMetaText "k", TxMetaText "v2" )
            ]
          )
        ]
      }

It is possible to perform a JSON round trip with the DetailedSchema option:

> metadataFromJson TxMetadataJsonDetailedSchema
   (metadataToJson TxMetadataJsonDetailedSchema m)
   == Right m
True

But not possible to perform a JSON round trip with the NoSchema option.

> metadataFromJson TxMetadataJsonNoSchema
   (metadataToJson TxMetadataJsonNoSchema m)
   == Right m
False

This is because the NoSchema option encodes a TxMetaMap as a JSON object, which does not permit duplicate keys:

> metadataToJson TxMetadataJsonNoSchema m
Object (fromList [("0",Object (fromList [("k",String "v2")]))])

Which when rendered as JSON produces the following string:

> BL.putStrLn $ Aeson.encode $ metadataToJson TxMetadataJsonNoSchema m
{"0":{"k":"v2"}}

The preservation of only the latest key-value mapping is consistent with the behaviour of Data.Aeson.object.

Going back to DetailedSchema, this encoding uses JSON arrays of key-value pairs to represent TxMetaMap objects:

> BL.putStrLn $ Aeson.encode $ metadataToJson TxMetadataJsonDetailedSchema m
{ "0":
  { "map":
    [ { "k": {"string":"k"}
      , "v": {"string":"v1"}
      }
    , { "k": {"string":"k"}
      , "v": {"string":"v2"}
      }
    ]
  }
}

Thus making it possible to satisfy the JSON round-trip property.

jonathanknowles commented 1 week ago

From the perspective of the upstream cardano-api library, if the TxMetadataJsonNoSchema option is used, then it seems that not satisfying the round trip property decode . encode == id is by design:

https://github.com/IntersectMBO/cardano-api/blob/2b5e9485df5a771fef4b15e0e1a4ba936c5d20d6/cardano-api/test/cardano-api-test/Test/Cardano/Api/Metadata.hs#L186

Source code extract:

-- This uses the \"no schema\" mapping. Note that with this mapping it is /not/
-- the case that any tx metadata can be converted to JSON and back to give the
-- original value.