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

taprpc: marshal with metareveal of issuance proof #1050

Merged
merged 4 commits into from
Jul 25, 2024

Conversation

jharveyb
Copy link
Contributor

Fixes decoding of issuance proofs from other nodes.

An example issuance proof:

544150460000000001fd05f55441505000040000000002247ce35d0fbc1696b847eda343f8c1b1d4db92e6eea3053b57af0010b051288e270000000104500000a020f8d9835cebed16eeba76a2981f7af3c2443f2fe50e3aaf27090000000000000034f26bf623a504bcd927ccc612e94ad47fa03622925333f539e1ee79792f4ff0482a8666c0ff3f198054f6be06cd020000000001017ce35d0fbc1696b847eda343f8c1b1d4db92e6eea3053b57af0010b051288e2701000000000000000002e80300000000000022512007a3dae7694327bdb68dcb57050e2a551f893899f98600aedddcf380d9cfe15c7eae0902000000002251204a0172137c0698eadddf5f1e813734c5e276b8c11937ac57b56ac5d6b6c69e230140b6bdf2525a344d37913ff60ebabad2e444ac96c7e5efc605d96dff827c78f90a4e3cce421300ff0495e31c2bba7cce0d1affdfa82cfb9406614a4419311b2d480000000008fd01a30dcba74018d4a282f2d0003e334a22e0a23257230f1fae1b8995e4a54b19ce62a291231fad5f92d5ce2afb5769665bddcb05858d1fee45328c590e4719f35e3e7fef77daef84312be1c667c12deaf299050c2cbbb24e064ab707993752057cfb9123c8e5f87d82da46bd7d72c9b1116c1f8f0ede1107284ccca1531aa92f8eda799b4d3380a5714b7955da464a34371306eb3a2fcc6da169a3ecef691e6ddfc9da241fca4b93e183aa500b06b187e024551e33bd7d47e4228e742d6c9c22812720e155b7f2ab15d53017fe61ae29cc88d179c2a90129e7bc54453397b63fa51db3bc48d15b41edb21df62dd39e8eb2a018a23dfe61bb7894fb2382da5ceabe3d5db9a02fca99d5cf9616866cd56387b6f9a83f000f4e5dc04389c47b19d23bb3f697e537a7dcd888ee0119fe34f41fee155caa38cc811ebe98a2c783223c696f1ac7f417ad004cce6f40df015e58f62e10d9145efba0e292af9fab8ff458ee39f0f96eb21cb5fa1bc69acb7d70ae3fd69fc2385cacd1be37f65cb2e311ad8464362190965591608f17983946f172cd351b2be4ab07234b70d4b59432e68b8a1ed5331d0afd015900010002517ce35d0fbc1696b847eda343f8c1b1d4db92e6eea3053b57af0010b051288e270000000107626565666275781b206639244dae9875be0db3babd9c326e9ffe319f9a913c56b765c8e80b629900000000000401000605fe000f42400bad01ab01650000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000034201400ff3ae2863b13e8b58e15e16256078363da61910ad5d08500bbeb5ca77569ab0259ea3c0f0c5b37d48ef09b520e8f36517bf688589d023804044bb0d0c7d66160e020000102102d2f9533145725d3badecc65382413f67cc30df4788c16c0fce7f5847338b579e11210385a91cb56b237bc4091618e459e0f0380a90d2e08ca17312c2cafe18773f73560cc7000400000000022102d35d233cf4113dac5b19f8d62a302abc0ef0bedd4103333613299a31ff23098a039c014900010002203eb529745a38ceec8f62825af9bc4dbf2c0ab665896098f6ca76d95219a4e59c04220000ffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffff024f000102024a0001e17f500479dbdb0aa6a5d8d3c8b7634500e8eb8db189b0595723a4df15ff0d2200000000000f4240ffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffff7f0d30012e000400000001022102e3ab40dff5a40c2e404bd4c1eca2044c1d34b414d3f6d5e8a1adc4761c64d5800503040101112700010102227b22646563696d616c5f646973706c6179223a332c2268656c6c6f223a747275657d1604002bbb1c17517ce35d0fbc1696b847eda343f8c1b1d4db92e6eea3053b57af0010b051288e270000000107626565666275781b206639244dae9875be0db3babd9c326e9ffe319f9a913c56b765c8e80b62990000000000192103be790997164a0bc4e64a87b1e42c6c24da6c601999dd6a628fc301d1b09e26821f582140941ded071dad944f06f9bbcaa08661f6b5b5e3fd8e1c4b5f71ee84ff

Decode to binary by piping to xxd -r -p. If we try to decode with tapcli proofs decode, we get an error about a metadata lookup failure for this asset.

This fixes that by reading the metadata from the metareveal of the proof if present.

@guggero guggero force-pushed the proof_decode_read_metareveal branch 2 times, most recently from ab1d49f to e7bc705 Compare July 25, 2024 11:54
guggero added 3 commits July 25, 2024 14:04
If the meta data isn't JSON when we expect it to be, we return the
correct error.
When attempting to fetch the decimal display in a non strict way, we
directly return fn.None() if the meta data isn't valid JSON.
@guggero guggero force-pushed the proof_decode_read_metareveal branch from e7bc705 to 21c1148 Compare July 25, 2024 12:05
@guggero
Copy link
Member

guggero commented Jul 25, 2024

Was able to reproduce and continued on the fix. Should be all good now.
Included a version bump to be able to ship this as a hotfix/patch v0.4.1-alpha.

@guggero guggero marked this pull request as ready for review July 25, 2024 12:05
rpcserver.go Outdated
// getDecimalDisplayNonStrict attempts to decode a decimal display value from
// metadata. If no custom decimal display value is decoded, the default value of
// 0 is returned without error.
func getDecimalDisplayNonStrict(meta *proof.MetaReveal) (uint32, error) {
Copy link
Contributor

Choose a reason for hiding this comment

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

I think this should return an fn.Option instead of defaulting to 0. Later at the callsite, taprpc.MarshalAsset takes an option anyway.

Copy link
Member

Choose a reason for hiding this comment

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

That's added in a later commit. Perhaps you were looking at an outdated version?

Copy link
Contributor

@ffranr ffranr Jul 25, 2024

Choose a reason for hiding this comment

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

I don't think I was looking at an outdated version of the commit set. I was going through each commit in order. I think later commits change the changes of earlier commits.

Copy link
Contributor

@ffranr ffranr left a comment

Choose a reason for hiding this comment

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

My comment had already been addressed in a later commit.

@gijswijs gijswijs self-requested a review July 25, 2024 13:11
@gijswijs
Copy link
Contributor

lgtm 🎉

@guggero guggero merged commit 0dfdb12 into main Jul 25, 2024
16 checks passed
@guggero guggero deleted the proof_decode_read_metareveal branch July 25, 2024 14:23
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
Status: ✅ Done
Development

Successfully merging this pull request may close these issues.

4 participants