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

Map serialization #41

Closed
hussein-aitlahcen opened this issue Jun 10, 2022 · 9 comments · Fixed by #45
Closed

Map serialization #41

hussein-aitlahcen opened this issue Jun 10, 2022 · 9 comments · Fixed by #45

Comments

@hussein-aitlahcen
Copy link

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
Copy link

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
Copy link
Member

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
Copy link

@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.

osmosis-labs/osmosis-rust#43

@webmaster128
Copy link
Member

webmaster128 commented Oct 2, 2022

Okay, cool. Anyone wants to start a PR? Looking into rust-embedded-community#23 is probably a good starting point.

@webmaster128
Copy link
Member

Could you all have a look at #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
Copy link
Member

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
Copy link
Member

Motivation is here: #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
Copy link
Member

Motivation is here: #20

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

@ethanfrey
Copy link
Member

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: #43 (comment) but getting even one case working is a big step forward

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging a pull request may close this issue.

5 participants