Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Serialize AssetName to JSON as hex string #3211

Merged
merged 4 commits into from
Nov 2, 2021
Merged

Serialize AssetName to JSON as hex string #3211

merged 4 commits into from
Nov 2, 2021

Conversation

cblp
Copy link
Contributor

@cblp cblp commented Sep 17, 2021

Fix (second attempt) of incorrect UTF-8 decoding of non-textual (non-UTF-8) asset names.

I also changed tx-view to test JSON repr.

BEFORE:

{
  "51f056b530aba5969bf996d1be51c7aa3581f6e0d7b9e0911a5c0c1d": {
    "TheDrifter13": 1,
    "GOLDtest": 5,
    "��\"3DUfw��������": 42 // UTF-8 fail, actually
  }
}

AFTER:

{
  "51f056b530aba5969bf996d1be51c7aa3581f6e0d7b9e0911a5c0c1d": {
    "546865447269667465723133": 1,
    "474f4c4474657374": 5,
    "00112233445566778899AABBCCDDEEFF": 42
  }
}

This is how it already looks like in UI (Cardano Explorer):

asset1...
Ticker:
Name:
Description:
Policy ID: 51f056b530aba5969bf996d1be51c7aa3581f6e0d7b9e0911a5c0c1d
Asset Name: 474f4c4474657374

Future Daedalus:

Name: GoodCoin
Asset name: 676f6f64636f696e (ASCII: goodcoin)

@gitmachtl
Copy link
Contributor

gitmachtl commented Sep 17, 2021

Just as a note: It should also be than <policyID>.<assetName> hexstring for the normal query utxo output too, also for the tx-out and mint parameters for CLI. like the "subject" parameter for the metadata-registry, same format for all.

@cblp
Copy link
Contributor Author

cblp commented Sep 17, 2021

Removed textual AssetName everywhere, hex only

@gitmachtl
Copy link
Contributor

gitmachtl commented Sep 18, 2021

Removed textual AssetName everywhere, hex only

minted and sent some tokens with bytearrays, looking good

image

Copy link
Contributor

@Jimbo4350 Jimbo4350 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nice, a few comments

cardano-api/src/Cardano/Api/Value.hs Outdated Show resolved Hide resolved
cardano-api/src/Cardano/Api/Value.hs Outdated Show resolved Hide resolved
cardano-api/src/Cardano/Api/Value.hs Outdated Show resolved Hide resolved
cardano-api/src/Cardano/Api/Value.hs Outdated Show resolved Hide resolved
cardano-api/src/Cardano/Api/Value.hs Outdated Show resolved Hide resolved
cardano-cli/src/Cardano/CLI/Mary/ValueParser.hs Outdated Show resolved Hide resolved
@gitmachtl
Copy link
Contributor

@cblp any updates here from your side? would be nice to see this in 1.31.0 😄

@cblp cblp force-pushed the cblp/asset-hex branch 4 times, most recently from caf2e3f to a8660e2 Compare September 23, 2021 14:09
@gitmachtl
Copy link
Contributor

how does the output of "Friendly output multi-asset with ASCII version where possible" look like? i would leave it up to the software behind the CLI to decide to show it as readable or as hex/binary. makes it more complicated to post-process if the output is not hex all the time.

@cblp
Copy link
Contributor Author

cblp commented Sep 23, 2021

how does the output of "Friendly output multi-asset with ASCII version where possible" look like?

Here is an example:

mint:
  policy 52dc3d43b6d2465e96109ce75ab61abe5e9c1d8a3c9ce6ff8a3af528:
    asset 736b79 (sky): 142

i would leave it up to the software behind the CLI to decide to show it as readable or as hex/binary. makes it more complicated to post-process if the output is not hex all the time.

cardano-cli transaction view is an end-user software that uses Cardano API.

API version of cardano-cli is to be built in #3213

@gitmachtl
Copy link
Contributor

ah, so the normal utxo query is not affected by this, only in the transaction view output?

@cblp
Copy link
Contributor Author

cblp commented Sep 23, 2021

@gitmachtl utxo query is affected only in part of switching from text to hex.

@gitmachtl
Copy link
Contributor

@gitmachtl utxo query is affected only in part of switching from text to hex.

thats perfect, so hex output in the normal utxo query output, also in the JSON version of the output. and friendly names in the transaction view output.

@cblp
Copy link
Contributor Author

cblp commented Sep 23, 2021

@dcoutts should I fix docs (couttscoins etc.) in this PR?

@gitmachtl
Copy link
Contributor

ping 😄

Copy link
Contributor

@Jimbo4350 Jimbo4350 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

A few comments

ttoken = proxyToAsType (Proxy :: Proxy a)
tname = (tyConName . typeRepTyCon . typeRep) (Proxy :: Proxy a)

instance SerialiseAsRawBytes a => ToJSONKey (UsingRawBytesHex a)
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why did you make this explicit?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The default ToJSONKey implementation is the Value-based encoding; we need the Text-based here.

cardano-api/src/Cardano/Api/Value.hs Outdated Show resolved Hide resolved
cardano-api/src/Cardano/Api/Value.hs Outdated Show resolved Hide resolved
cardano-cli/src/Cardano/CLI/Mary/ValueParser.hs Outdated Show resolved Hide resolved
cardano-api/test/Test/Cardano/Api/Typed/Value.hs Outdated Show resolved Hide resolved
Aeson.FromJSONKeyTextParser $
either fail pure . deserialiseFromRawBytesBase16 . Text.encodeUtf8

deserialiseFromRawBytesBase16 ::
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We now have two implementations of essentially the same thing deserialiseFromRawBytesHex and deserialiseFromRawBytesBase16. Not to mention we Base16 decode the input ByteString twice in order to get an error message in deserialiseFromRawBytesBase16. The proper solution here is to update deserialiseFromRawBytesHex to return an Either. This refactor would be better as a separate commit.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Too many places expect Maybe from deserialiseFromRawBytesHex. Should it be a separate PR?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yeah, please raise a follow up PR to correct this. Leave a TODO in the code explaining this.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Here it is #3304

cardano-api/src/Cardano/Api/Utils.hs Show resolved Hide resolved
cardano-api/src/Cardano/Api/Value.hs Outdated Show resolved Hide resolved
@cblp cblp force-pushed the cblp/asset-hex branch 3 times, most recently from 5576208 to d24e226 Compare October 13, 2021 14:45
Copy link
Contributor

@Jimbo4350 Jimbo4350 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nice 👍

@gitmachtl
Copy link
Contributor

Cool! 👍

@gitmachtl
Copy link
Contributor

Looking forward to see this PR within the next release.

@gitmachtl
Copy link
Contributor

@JaredCorduan @newhoggy maybe someone of you is having the time to review it also?

@gitmachtl
Copy link
Contributor

can we expect to see this in the next cardano-cli release?

@gitmachtl
Copy link
Contributor

gitmachtl commented Nov 1, 2021

@nc6 do we have a chance to get this into the 1.31.0 release?

@cblp
Copy link
Contributor Author

cblp commented Nov 2, 2021

bors merge

@iohk-bors
Copy link
Contributor

iohk-bors bot commented Nov 2, 2021

Build succeeded:

@iohk-bors iohk-bors bot merged commit b6c7928 into master Nov 2, 2021
@iohk-bors iohk-bors bot deleted the cblp/asset-hex branch November 2, 2021 08:37
@gitmachtl
Copy link
Contributor

wohoo! :-)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants