RGB-WG / rgb-core

RGB Core Library: consensus validation for private & scalable client-validated smart contracts on Bitcoin & Lightning
https://spec.rgb.tech
Apache License 2.0
207 stars 52 forks source link

fix consignment yaml conversion #215

Closed zoedberg closed 6 months ago

zoedberg commented 7 months ago

This PR fixes the error we receive when trying to convert a consignment in yaml representation (serializing nested enums in YAML is not supported yet) by manually implementing the serialization and deserialization of XChain.

codecov[bot] commented 7 months ago

Codecov Report

Attention: Patch coverage is 0% with 16 lines in your changes are missing coverage. Please review.

Project coverage is 15.4%. Comparing base (3c72d5f) to head (885f7de). Report is 7 commits behind head on master.

Files Patch % Lines
src/contract/xchain.rs 0.0% 16 Missing :warning:
Additional details and impacted files ```diff @@ Coverage Diff @@ ## master #215 +/- ## ======================================== - Coverage 15.5% 15.4% -0.0% ======================================== Files 33 33 Lines 4162 4173 +11 ======================================== Hits 644 644 - Misses 3518 3529 +11 ``` | [Flag](https://app.codecov.io/gh/RGB-WG/rgb-core/pull/215/flags?src=pr&el=flags&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=RGB-WG) | Coverage Δ | | |---|---|---| | [rust](https://app.codecov.io/gh/RGB-WG/rgb-core/pull/215/flags?src=pr&el=flag&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=RGB-WG) | `15.4% <0.0%> (-<0.1%)` | :arrow_down: | Flags with carried forward coverage won't be shown. [Click here](https://docs.codecov.io/docs/carryforward-flags?utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=RGB-WG#carryforward-flags-in-the-pull-request-comment) to find out more.

:umbrella: View full report in Codecov by Sentry.
:loudspeaker: Have feedback on the report? Share it here.

zoedberg commented 7 months ago

Do you think this is the right way for enum serialization?

I don't think there is a "right way" to serialize nested enums, there may be better solutions but this is the only solution I've found so far. Probably in the future the team working on serde_yaml will come with a solution to this issue, but for now I think we can be happy to have found a way to fix the conversion

dr-orlovsky commented 7 months ago

@zoedberg the problem that this specific fix makes all of the serialization to be YAML-based, breaking all serde compatibility. If someone will try to do JSON, TOML or bincode deserialization, he will fail forever with some wierd explanations coming from serde_yaml.

zoedberg commented 7 months ago

@zoedberg the problem that this specific fix makes all of the serialization to be YAML-based, breaking all serde compatibility. If someone will try to do JSON, TOML or bincode deserialization, he will fail forever with some wierd explanations coming from serde_yaml.

All serializations are already broken and to support JSON and TOML we should do a bunch more work, since the problem with those serializations is there is no support for non-string keys in maps. I recall we already discussed in the past the possibility to just drop support for JSON and TOML, so I would just follow that path (I can make a PR on rgb-wallet after this to remove support for broken serialization formats). I think this fix is very important because it allow us adding consignment validation tests (as already discussed in person). Anyway, if someone in the future will try to fix the conversion for other serde formats I guess he/she will notice the manual implementation of the XChain and find a way to fix it (assuming that in the meantime serde_yaml team hasn't already delivered a fix).

dr-orlovsky commented 7 months ago

What I think is a correct approach: #216

May though require some changes downstream in the code which was using XOutpoint.

dr-orlovsky commented 7 months ago

I think this fix is very important

Yes, but this PR introduces more bugs, so I can't see it as a fix. There are several better approaches. Approach nACK

All serde binary serializations were working before this PR - and this PR breaks them.

zoedberg commented 7 months ago

All serde binary serializations were working before this PR - and this PR breaks them.

Not sure which serializations you are referring to. rgb-wallet supports yaml, toml, json, debug, contractum and only debug works at the moment

dr-orlovsky commented 7 months ago

I am referring to all clients that use or will use RGB Core in a future.

Serde implementation means the type must be serializable in binary form - with CBOR, BSON, binvode etc.

dr-orlovsky commented 6 months ago

Closing in favor of #216