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

Revert "Don't assume AssetName is UTF8" #3206

Merged
merged 1 commit into from
Sep 16, 2021
Merged

Conversation

disassembler
Copy link
Contributor

This reverts commit 2144866.

Reverting as this was only changed in display, but transaction building still assumes latin1. It makes more sense to do this as one piece when devs have time to work on this to limit churn with community tools.

@nc6
Copy link
Contributor

nc6 commented Sep 16, 2021

Looks reasonable to me. @sjoerdvisscher does this interfere with the reasons you needed this merged?

@disassembler
Copy link
Contributor Author

disassembler commented Sep 16, 2021

This was opened as a result of community feedback. Two complaints:

  1. The structure is a lot uglier:
    utxo query on 1.29.0
{
    "f38aee216f08e383a2035acf333425e461eaf326a9f0524ecf6ae3922ab7fc1b#1": {
        "address": "addr_test1vp29cyrdsxhgze2pq47vnd3ev39sxw6rljaf6d46yvfwstq6cr3kr",
        "value": {
            "51f056b530aba5969bf996d1be51c7aa3581f6e0d7b9e0911a5c0c1d": {
                "TheDrifter13": 1
            },
            "b23f63d9d061a4686b782036bb5fb9c712750028b526ef1e242cf5b7": {
                "GOLDtest": 5
            },
            "lovelace": 135064845,
            "c702bcf36fd62f00cc949db80aca5545c1843d481be90e9acb8edb64": {
                "Gold1": 1
            }
        }
    }
}

utxo query after this change

{
    "f38aee216f08e383a2035acf333425e461eaf326a9f0524ecf6ae3922ab7fc1b#1":
    {
        "address": "addr_test1vp29cyrdsxhgze2pq47vnd3ev39sxw6rljaf6d46yvfwstq6cr3kr",
        "value":
        {
            "51f056b530aba5969bf996d1be51c7aa3581f6e0d7b9e0911a5c0c1d":
            [
                [
                    "546865447269667465723133",
                    1
                ]
            ],
            "b23f63d9d061a4686b782036bb5fb9c712750028b526ef1e242cf5b7":
            [
                [
                    "474f4c4474657374",
                    5
                ]
            ],
            "lovelace": 135064845,
            "c702bcf36fd62f00cc949db80aca5545c1843d481be90e9acb8edb64":
            [
                [
                    "476f6c6431",
                    1
                ]
            ]
        }
    }
}
  1. The transaction build tools expect the asset name in a latin1 format instead of hex

@sjoerdvisscher
Copy link
Contributor

To be clear: I did not anticipate that this change would change the structure, and we don't need that. We only need support for non-UTF8 asset names.

I have asked @j-mueller to weigh in regarding how much reverting this will be a problem.

@j-mueller
Copy link

@disassembler : Without this change we won't be able to send certain transactions from the PAB to the wallet backend. So I would very much prefer not to revert it. The spec is pretty clear on this: AssetName can be any bytestring.

@nc6
Copy link
Contributor

nc6 commented Sep 16, 2021

@j-mueller Would the PAB be able to continue developing against the existing version, if we back this out for this release and promise to have it (properly!) working again by the time the PAB is ready? @disassembler I guess this would mean we'd need to push a new release for PAB compatibility (but that wouldn't need a new HF)

@Jimbo4350
Copy link
Contributor

I've asked @cblp to work on this, discuss the changes needed with him.

@AndrewWestberg
Copy link

If this change is made, to make the token names as HEX due to a requirement of the PAB, I would also kindly suggest that cardano-cli support hex names at the same time.

@j-mueller
Copy link

@nc6 : Yes we will be able to keep working on the wallet backend integration, which is our main task currently. But after the HF, when our change is put back in, we'll also need a new release of the wallet backend, because that's where we send the transactions that use this encoding.

And just to be clear what the consequences are: Any apps that use hashes for AssetNames won't be able to send transactions to the network via wallet backend as long as the latin1 encoding is used. This includes everything that uses the thread tokens feature of our state machine library.

@cblp
Copy link
Contributor

cblp commented Sep 16, 2021

If I understand correctly, no-assuming-utf-8 works correctly, and reverting it will fail utxo queries. So, I object against assuming UTF8 and 3206. It's safer to show hex strings.

Copy link
Contributor

@JaredCorduan JaredCorduan left a comment

Choose a reason for hiding this comment

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

I'm approving so that we can keep things moving, and knowing that @cblp will start on the proper fix tomorrow with high priority.

@disassembler
Copy link
Contributor Author

bors r+

@iohk-bors
Copy link
Contributor

iohk-bors bot commented Sep 16, 2021

Build succeeded:

@iohk-bors iohk-bors bot merged commit 8132211 into master Sep 16, 2021
@iohk-bors iohk-bors bot deleted the revert-assetname-utf8 branch September 16, 2021 18:15
@gitmachtl
Copy link
Contributor

gitmachtl commented Sep 17, 2021

Do we have an idea on how the switch to hex will look like for the normal query utxo output and also for the cli parameters --tx-out and --mint? I guess it will be the cleartext readable end for many tokens? We can still try to display it if its UTF-8.

Is the idea to keep the <policyID>.<assetName> structure with the '.' in the middle as separator for all the outputs/inputs, or will it be a combined hexstring with <policyID><assetName> like the metadata-registry subject?

@gitmachtl
Copy link
Contributor

gitmachtl commented Sep 17, 2021

or maybe with an additional parameter for the cli query utxo command '--decode' to get out a readable format and defaulting to pure hex output?

it should be just the <policyID><assetName> combined hexstring for the normal query utxo output too, also for the tx-out and mint parameters. like the "subject" parameter for the metadata-registry, same format for all.

@gitmachtl
Copy link
Contributor

resolved via #3211

@gitmachtl
Copy link
Contributor

I'm approving so that we can keep things moving, and knowing that @cblp will start on the proper fix tomorrow with high priority.

please check #3211 should be good for merging...

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.

None yet

9 participants