search

CosmWasm / serde-json-wasm

serde_json for Wasm programs (small, deterministic, no floats)
Apache License 2.0
30 stars 15 forks source link

Map serialization #41

Closed hussein-aitlahcen closed 1 year ago

hussein-aitlahcen commented 2 years ago

Hi,

We are currently integrating CosmWasm at Composable. We would like to be able to provide custom functionalities throught the usage of Custom messages. One of the protocol require passing a map from the chain to the contract and vice versa. I found out that the library forbid the serialization of map and was a bit curious about this choice: why a BTreeMap wouldn't be serializable as it is fully deterministic?

Thanks

CyberHoward commented 2 years ago

Same issue here. serde-cw-value solved parsing from a Binary to a generic Value. Thanks for that! Support for serializing cw-value types to JSON would be an awesome feature to make this complete.

webmaster128 commented 2 years ago

There has been a history of discouraging maps in Cosmos. This is primarily because the default map type in Go is randomized, leading to non-deterministic iteration order. Since CosmWasm integrates heavily with Cosmos SDK, we followed that path. Also when forking serde-json-core in October 2019, it did not yet have the map support which came shortly after.

That being said, as far as I can tell it is perfectly safe to use both BTreeMap and HashMap in CosmWasm. BTreeMap is sorted by design. HashMap is deterministic when compiled to the Wasm target but behaves differently between Wasm and the unit test environment on the dev machine, which is a minor caveat.

Another issue to consider is that the JSON standard allows having the same key in an object multiple times, which is something that cannot be deserialized into a map.

That being said, I'd be happy to review a map serde implementation for this library. We can still discourage using it but I don't see any objective blockers.

iboss-ptk commented 2 years ago

@webmaster128 This would be very helpful. I have the same issue dealing with serializing Any proto json format due to #[serde(flatten)] depends on map serialization.

https://github.com/osmosis-labs/osmosis-rust/issues/43

webmaster128 commented 2 years ago

Okay, cool. Anyone wants to start a PR? Looking into https://github.com/rust-embedded-community/serde-json-core/pull/23 is probably a good starting point.

webmaster128 commented 2 years ago

Could you all have a look at https://github.com/CosmWasm/serde-json-wasm/pull/45 and help this get done? I think PR into PR or good comments are a great way to contribute to the existing work.

webmaster128 commented 2 years ago

The work in #45 is likely to be merged this week. I'd appreciate any additional review and testing from folks that plan to use the feature.

ethanfrey commented 1 year ago

Motivation is here: https://github.com/CosmWasm/serde-json-wasm/issues/20

I don't care about maps so much besides as a way to get flatten support. Let's test if this works once maps work or if we need to do more work on top.

webmaster128 commented 1 year ago

Motivation is here: #20

Ah, nice. So we can add a test with flatten into the PR.

ethanfrey commented 1 year ago

Motivation is here: #20

Ah, nice. So we can add a test with flatten into the PR.

And close two issues with one PR 😄

I think there is some follow up stuff on better untagged/flatten support here: https://github.com/CosmWasm/serde-json-wasm/issues/43#issuecomment-1274307334 but getting even one case working is a big step forward