Main dids data hash #11440

ytx1991 · 2022-05-04T21:52:30Z

The issue is because the data hash is expected to be a hex, but it is a string. Since JSON doesn't support hex number, use string instead

arvidn

The issue is because the data hash is expected to be a hex, but it is a string. Since JSON doesn't support hex number, use string instead

hex is a string representation of binary data. The code you're pointing at is encoding binary data as hex in order to ship it as a json string. Can you elaborate on what the problem is?

Are you experiencing some error that you believe is caused by this message encoding?

arvidn · 2022-05-05T10:51:26Z

tests/wallet/did_wallet/test_nft_wallet.py

@@ -110,7 +110,7 @@ async def test_nft_wallet_trade_chia_and_cat(self, three_wallet_nodes, trusted):
        metadata = Program.to(
            [
                ("u", ["https://www.chia.net/img/branding/chia-logo.svg"]),
-                ("h", 0xD4584AD463139FA8C0D9F68F4B59F185),
+                ("h", "0xD4584AD463139FA8C0D9F68F4B59F185"),


I don't think it's a good idea to encode data as an ascii string.

arvidn · 2022-05-05T10:52:20Z

chia/wallet/nft_wallet/nft_puzzles.py

@@ -142,7 +142,7 @@ def get_nft_info_from_puzzle(puzzle: Program, nft_coin: Coin) -> NFTInfo:
        uncurried_nft.owner_did.as_python(),
        uint64(uncurried_nft.trade_price_percentage.as_int()),
        data_uris,
-        uncurried_nft.data_hash.as_python().hex(),
+        str(uncurried_nft.data_hash.as_python(), "utf-8"),


if the data is not valid utf-8, this would throw. There's no test covering this case. (but I think this change is poorly justified to begin with)

paninaro · 2022-05-05T15:53:48Z

The issue is because the data hash is expected to be a hex, but it is a string. Since JSON doesn't support hex number, use string instead

hex is a string representation of binary data. The code you're pointing at is encoding binary data as hex in order to ship it as a json string. Can you elaborate on what the problem is?

Are you experiencing some error that you believe is caused by this message encoding?

The issue I'm seeing is in the JSON representation of the hash, so I would expect the fix to be at the RPC level. I previously used the nft_mint_nft RPC to create an NFT with the data hash specified as the string "c520c99955118784734af730590cf74e9c8961b9036b772052597fdb5206ee40" in the JSON payload. Expecting the hash to be provided as a string in the JSON seems correct to me, so I would expect the RPC to convert that string to a byte array (interpreted as hex bytes) if that's what the CLVM is expecting.

Similarly, when calling the nft_get_nfts RPC, the corresponding data hash was returned as the string "63353230633939393535313138373834373334616637333035393063663734653963383936316239303336623737323035323539376664623532303665653430". It may be that this code is already doing the right thing, and that the conversion only needs to occur in the nft_mint_nft RPC, but I haven't done a full accounting.

paninaro · 2022-05-05T15:57:10Z

Maybe it's as simple as making this change:

ytx1991 · 2022-05-05T16:20:34Z

There are two problems.

Like Jeff explained, this is a string in the JSON payload, but I agree it can be covert to bytes at RPC level.
If the fields in the metadata can be any type then people who want to uncurry it needs to know this info, since different types have different uncurry ways.

From my POV, it's better to use a general type for all fields in stead of arbitrary types. If we want to support multiples type in the metadata, I suggest to add a type info field to indicate the way of uncurry and casting.

github-actions · 2022-05-06T02:38:49Z

This pull request has conflicts, please resolve those before we can evaluate the pull request.

lgtm-com · 2022-05-06T21:41:53Z

This pull request introduces 1 alert when merging b3197d5 into ecf7cd8 - view on LGTM.com

new alerts:

1 for Unused import

github-actions · 2022-05-07T01:15:44Z

Conflicts have been resolved. A maintainer will review the pull request shortly.

arvidn · 2022-05-07T09:49:55Z

chia/wallet/nft_wallet/nft_wallet.py

+        try:
+            uncurried_nft = UncurriedNFT.uncurry(puzzle)
+        except Exception:
+            # The parent is not an NFT which means we need to scrub all of its children from our DB


I don't see a test covering this case

After a deep look, I think this case is impossible. Since we already checked and successfully uncurried NFT before this point. I will just remove this exception section

arvidn · 2022-05-07T09:53:30Z

tests/wallet/nft_wallet/test_nft_wallet.py

@@ -212,7 +212,7 @@ async def test_nft_wallet_rpc_creation_and_list(two_wallet_nodes: Any, trusted:
        {
            "wallet_id": nft_wallet_0_id,
            "artist_address": ph,
-            "hash": 0xD4584AD463139FA8C0D9F68F4B59F184,
+            "hash": "0xD4584AD463139FA8C0D9F68F4B59F184",


I don't understand your rationale for changing this from a (compact and efficient) binary representation to a (slow and inflated) ascii string representation.

Even if there's a really good reason to represent this hash in ascii-form, surely you wouldn't need the 0x prefix.

I may be misunderstanding something actually, I was thinking that this was part of the smart contract, but this is the API to our RPC client, right? So this isn't really in a place where we have the ambition to be efficient, since we already use JSON at this level. And presumably the client class will turn this hash into a string when encoding it as JSON anyway.

However, I think there's a consistency argument to be made. The "artist_address" is a puzzle hash, fundamentally the same type as this "hash". They should probably use the same representation

That's correct, the tests are bypassing the normal JSON serialization/deserialization that would be applied in RpcServer, so the onus is on the test writer to ensure that the requests/responses are JSON compatible. In this case, the hash(es) being supplied as raw bytes is incorrect, as that isn't directly possible with JSON. Ideally we should come up with a test fixture (or something) that can wrap an RPC API object instance and enforce JSON compatibility.

I agree that we should be consistent, using strings to pass around hashes (puzzle hashes, data hashes, etc.)

arvidn · 2022-05-07T10:01:14Z

please make sure to have unit tests covering the new cases you're adding, and ideally covering other cases that lack tests as well. If the code needs to be refactored in order to be tested, I think that's fine.

trepca · 2022-05-09T11:19:34Z

chia/wallet/wallet_state_manager.py

-            return await self.handle_nft(coin_spend, iter(nft_curried_args))
+        try:
+            uncurried_nft = UncurriedNFT.uncurry(Program.from_bytes(bytes(coin_spend.puzzle_reveal)))
+        except Exception:


lets be a bit more specific here, as can easily lead to very hard to debug bugs

Fix data hash issue

9da3cbb

ytx1991 requested a review from paninaro May 4, 2022 21:52

ytx1991 changed the base branch from main to main_dids May 4, 2022 21:53

arvidn requested changes May 5, 2022

View reviewed changes

github-actions bot added the merge_conflict Branch has conflicts that prevent merge to main label May 6, 2022

Merge from main_dids

b3197d5

Fix tests

4c2f41a

ytx1991 requested a review from arvidn May 6, 2022 23:40

github-actions bot removed the merge_conflict Branch has conflicts that prevent merge to main label May 7, 2022

arvidn reviewed May 7, 2022

View reviewed changes

trepca suggested changes May 9, 2022

View reviewed changes

altendky mentioned this pull request May 9, 2022

Cleanup UncurriedNFT #11379

Closed

1 task

Remove impossible case

9f7cc25

trepca merged commit 3b417e7 into main_dids May 9, 2022

ytx1991 deleted the main_dids_data_hash branch May 10, 2022 04:55

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Main dids data hash #11440

Main dids data hash #11440

ytx1991 commented May 4, 2022

arvidn left a comment

arvidn May 5, 2022

arvidn May 5, 2022

paninaro commented May 5, 2022

paninaro commented May 5, 2022

ytx1991 commented May 5, 2022

github-actions bot commented May 6, 2022

lgtm-com bot commented May 6, 2022

github-actions bot commented May 7, 2022

arvidn May 7, 2022

ytx1991 May 9, 2022

arvidn May 7, 2022

arvidn May 7, 2022

paninaro May 8, 2022

arvidn commented May 7, 2022

trepca May 9, 2022

Main dids data hash #11440

Main dids data hash #11440

Conversation

ytx1991 commented May 4, 2022

arvidn left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

paninaro commented May 5, 2022

paninaro commented May 5, 2022

ytx1991 commented May 5, 2022

github-actions bot commented May 6, 2022

lgtm-com bot commented May 6, 2022

github-actions bot commented May 7, 2022

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

arvidn commented May 7, 2022

Choose a reason for hiding this comment