change Variant serde (de|)serialization

[zoedberg]

This PR fixes Consignment JSON serialization (in detail its types: TypeSystem field).

Without this fix the serialization fails with a "key must be a string" error due to UnionVariants having a NonEmptyOrdMap with the Variant struct as key.

There are other possible solutions, like changing the Union(UnionVariants<Ref>) variant or the UnionVariants struct, but we would like @dr-orlovsky opinion on which solution is more appropriate.

P.S. I would like to open this on top of v2.7.2 since in RGB 0.11.1 we are still using that version, but there is no branch there (so for now I'm opening this on master)

[dr-orlovsky]

Approach NACK

1. This PR ignores the fact that serde is not only for string serialization, but also for binary. Binary serialization must not happen through Display/FromStr and in this way the PR puts in regression to the existing functionality.

2. The issue of key must be a string should be solved by modifying Variant serde implementation to be serde(transparent) (if needed, also for upstream types)

3. Even after 2 this will be a breaking change requiring to move to v3 and release all dependent crates in order to be used in RGB. I doubt this worth the effort; just implement custom consignment serde serialization doing type system as a Base85-ASCII encoding, as was before.

[zoedberg]

1. This PR ignores the fact that serde is not only for string serialization, but also for binary. Binary serialization must not happen through Display/FromStr and in this way the PR puts in regression to the existing functionality.

You are right, I thought we didn't care about serde binary serialization since for that we already have strict encoding, but I agree it's still better to keep that door open (note: it would have worked but it would just have been less efficient).
I fixed the PR in order to keep binary serialization efficient. This doesn't seem a backward incompatible change so please if you now agree with this PR I would ask you to create a v2.7 branch so I can change this PR base branch and then release version v2.7.3

[dr-orlovsky]

You just need to fix serde for VariantName, and leave Variant intact

[dr-orlovsky]

Specifically, you need to go to strict-encoding, remove serde(transparent) on RString type and do manual serde implementation like here (although no need to do binary encoding there, since both the string and binary would be the same for RString)

[codecov]

Codecov Report

Attention: Patch coverage is 77.35849% with 12 lines in your changes missing coverage. Please review.

Project coverage is 50.09%. Comparing base (f7be0a7) to head (ac23647).
Report is 3 commits behind head on master.

Files with missing lines	Patch %	Lines
rust/src/util.rs	77.35%	12 Missing ⚠️

Additional details and impacted files

@@            Coverage Diff             @@
##           master      #48      +/-   ##
==========================================
- Coverage   51.57%   50.09%   -1.49%     
==========================================
  Files          18       18              
  Lines        2220     2134      -86     
==========================================
- Hits         1145     1069      -76     
+ Misses       1075     1065      -10     

Flag	Coverage Δ
rust	50.09% <77.35%> (-1.49%)	⬇️

Flags with carried forward coverage won't be shown. Click here to find out more.

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:

• ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

[zoedberg]

Variant is a struct and is used as a key in a map. Some serialization methods, like JSON, don't support that. How would changing the serialization of VariantName fix this?

[dr-orlovsky]

Ok, I see now, sorry. I was under impression that it is VariantName which is used as a key.

Anyway, pls see my comments. We also need to add roundtrip serde string and binary test in order to make sure that the change actually works,

[zoedberg]

Anyway, pls see my comments. We also need to add roundtrip serde string and binary test in order to make sure that the change actually works,

Comments addressed and test added.

[dr-orlovsky]

Thank you! Two more things and seems we are done

[zoedberg]

Thank you! Two more things and seems we are done

Great, thank you. This should be ready, the only thing that's missing is a branch for v2.7.2 because I need these changes on top of that and not on top of master. Afterwards I will open another PR to put these changes on top of v2.8.1

P.S. I've also bumped the MSRV since half now requires at least Rust 1.81.0 (you can see that from the previous CI run that was failing)

[dr-orlovsky]

Thank you!

I wish I could merge, but the reality that MSRV bump is a breaking change and the problem need to be handled differently (pls see my comment)

I have also created v2.7 branch so you can do a PR there (additionally to this one; please keep this PR against master and just do another one with a backport)

[dr-orlovsky]

ACK ac23647

[dr-orlovsky]

Thank you.

The version was reverted, but not pinned in Cargo.toml; but (a) this is fine and (b) we can do it in some other PR if you'd like.

Merging, since it already takes forever :)

[zoedberg]

Thank you!

I wish I could merge, but the reality that MSRV bump is a breaking change and the problem need to be handled differently (pls see my comment)

Fixed and rebased on top of master

I have also created v2.7 branch so you can do a PR there (additionally to this one; please keep this PR against master and just do another one with a backport)

You created it in strict-types but this PR is on strict-encoding, could you please create the branch here as well? Thanks

[zoedberg]

The version was reverted, but not pinned in Cargo.toml; but (a) this is fine and (b) we can do it in some other PR if you'd like.

Not sure what you mean, I have fixed the unwanted MSRV bump by adding half = "<2.5.0" to the dev-dependencies

[dr-orlovsky]

You created it in strict-types but this PR is on strict-encoding, could you please create the branch here as well? Thanks

Done

Not sure what you mean, I have fixed the unwanted MSRV bump by adding half = "<2.5.0" to the dev-dependencies

Sorry, my eyes are betraying me