input-output-hk / daedalus

The open source cryptocurrency wallet for ada, built to grow with the community
https://daedaluswallet.io/
Apache License 2.0
1.23k stars 296 forks source link

Extra square brackets on all objects in metadata json #2492

Open vicepool opened 3 years ago

vicepool commented 3 years ago

I attach screenshots to compare. Daedalus puts every object in square brackets. Btw, a smaller, monospace font would look better for displaying a structured data.

Daedalus 202103300156

Cardano Explorer 202103300157

AdaStat 202103300158

CardanoScan 202103300159

nikolaglumac commented 3 years ago

Is this different than what it used to be in earlier Daedalus versions?

vicepool commented 3 years ago

I am not sure because I don't have it installed anymore. I am using the last 4.0.3 version.

nikolaglumac commented 3 years ago

Alright - we will check it out. Thanks for raising this!

vicepool commented 3 years ago

Here is another thing to fix. 202103300162

It display empty string as null. There is no null. The original had a "string" type for the value:

    "1": {
        "map": [
            {
                "k": {
                    "string": "message"
                },
                "v": {
                    "list": [
                        {
                            "string": "This was the beginning of the Farm in Downtown Lynden, WA May "
                        },
                        {
                            "string": "11, 2019. My hope is that at least a portion of this Land "
                        },
                        {
                            "string": "remain in open space farm for future generations to enjoy. "
                        },
                        {
                            "string": ""
                        },
                        {
                            "string": "Blake S."
                        },
                        {
                            "string": "VonStar Farms - Lynden, WA."
                        }
                    ]
                }
            }
        ]
    },

I understand the effort you need to put into schema conversion. I don't understand why the JSON-schema was made this hard way. I struggled and won the schema, although I had to study source codes in Haskel since the lack of good documentation and examples.

nikolaglumac commented 3 years ago

@DominikGuzei can you please check this?

DominikGuzei commented 3 years ago

@vicepool thanks for the feedback - the null issue is definitely a bug that is easy to fix đź‘Ť

Regarding the "Objects" (actually called "maps" in the metadata schema) being rendered with square brackets (as JS arrays): That was a design decision I made while implementing the metadata parsing because I figured that since metadata maps can have arbitrary "key" values (even nested lists & maps) we cannot directly translate them to normal JSON objects (because JSON object keys can only be simple values like int and string).

So I would rather say: The explorer might have the bug that it explodes when you provide complex data for map keys (I actually asked @rhyslbw about that but didn't get any answer – would be great to test that as part of this issue) … or it might be smart enough to fall back to square brackets notation for these complex cases (which is what I will try to implement now in Daedalus).

rhyslbw commented 3 years ago

@DominikGuzei As explained at the time, the metadata shown in the cardano-explorer-app is formatted by cardano-db-sync. It's certainly worth testing how that implementation handles the complex keys.

DominikGuzei commented 3 years ago

@rhyslbw @vicepool @nikolaglumac while trying to implement the improvements discussed above I figured that it's technically not possible to mix the "simple" object representation (like in Cardano Explorer) with the requirement of handling arbitrarily complex keys too. The problem is that if we simplify the map rendering to key-based object representation we can no longer display complex keys (because we cannot stringify complex keys to unique values that would make sense).

@rhyslbw I haven't tried to create metadata with complex nested keys yet to test the Explorer but after my work on this task, my strong guess is that it cannot handle complex keys. The only viable option I can think of is to fall back to array-based representation if there is any complex key in an object.

nikolaglumac commented 3 years ago

Whatever we decide to do, let's just make sure all products are in sync in terms of the metadata display. cc @darko-mijic @KtorZ @rvl

nikolaglumac commented 3 years ago

@disassembler I would like to hear your opinion here too!

rvl commented 3 years ago

Why not show metadata in (nested) tables instead of JSON?

nikolaglumac commented 3 years ago

Empty string values display has been fixed in https://github.com/input-output-hk/daedalus/pull/2503 and released in Daedalus 4.0.5 today.

DominikGuzei commented 3 years ago

@rvl we certainly can do this - actually in Daedalus it's all prepared for more advanced display of transaction metadata but we decided to release the simple solution as fast as possible (now we know the downside of the JSON representation)

nikolaglumac commented 3 years ago

Thanks @rvl and @DominikGuzei - we will tackle this in our next sprint. cc @darko-mijic