proof: add decimal display as TLV field to meta reveal

[guggero]

Minimal implementation that fixes #956.
Also fixes #997.

[Roasbeef]

LGTM 💃🏾

[jharveyb]

Looks good overall! Def cleaner than what we had before.

Added comments + I think we want to add this field to the AssetMeta proto as well + pipe it through there? If it's actually going to be a TLV field.

taproot-assets/taprpc/taprootassets.proto

Line 163 in 5236dea

message AssetMeta {

[jharveyb]

I think this condition now needs to change and not require the metaType to be JSON?

[guggero]

Good point! Fixed this and a couple of other occurrences of similar checks.

[jharveyb]

Also, I don't see the utility of adding this field as JSON in the metadata any more?

IIRC the actual user request was 'it shows up in RPC responses', and if its also discoverable from decoding proofs then it seems redundant. AFAICT we never read it after setting it.

[Roasbeef]

Also, I don't see the utility of adding this field as JSON in the metadata any more?

Good point, we can just leave it out there.

[Roasbeef]

I think we want to add this field to the AssetMeta proto as well

Technically it's part of the the metal reveal, which contains the raw asset meta, and now these additional bytes. I guess it's useful though to have it there if people want to use FetchAssetMeta to query the decimal value.

[jharveyb]

I think we want to add this field to the AssetMeta proto as well

Technically it's part of the the metal reveal, which contains the raw asset meta, and now these additional bytes. I guess it's useful though to have it there if people want to use FetchAssetMeta to query the decimal value.

Just realized it's now also input for MetaReveal.MetaHash() - so definitely 'consensus' if we're going to leave that as is.

[Roasbeef]

Discussed offline, and concluded that this approach was a bit too slick to be true:

• Old clients won't know of this meta reveal field.
• They'll parse the meta reveal w/o the new field, then go to check against the meta hash.
• That fails as they didn't include the portion they didn't know about.

[GeorgeTsagk]

So the purpose here was to drop this field to the MetaReveal level in order to support dec display for custom meta data?

[GeorgeTsagk]

could also check against Opaque data type and also with 0 dec display

[guggero]

Discussed offline, and concluded that this approach was a bit too slick to be true:

* Old clients won't know of this meta reveal field.

* They'll parse the meta reveal w/o the new field, then go to check against the meta hash.

* That fails as they didn't include the portion they didn't know about.

Ah, yes, nice catch.

Adding a summary of my comments made offline here for reference:

I see two possibilities for something we could do now:

1. don't add the decimal display to the meta hash (e.g. don't commit to it, just put it there as informational piece of data). not super nice and probably opens the door for shenanigans
2. add a "feature bit" to the meta type that indicates the meta data is nested TLV. then old nodes just read in the meta data as a blob and arrive at the correct hash. and new nodes would see the feature bit and do a second round of TLV decode on the data, being able to read and interpret extra fields that we put in there (the "actual" raw meta data being one of the sub fields). should work but feels veeery hacky...

I guess doing the two-level TLV encoding would qualify as "carveout/placeholder for future fields" as described in the issue #997. it just feels very hacky if we need to describe it in the BIP as "two levels of TLV encoding, depending on a feature bit"... but happy to go ahead and implement it, if we think it's the way to go

[guggero]

Okay, the current approach doesn't work and any quick hacks are too hacky. I'll implement the proper carve out to be able to do this properly in the next major version in an upcoming PR.