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

Reconsider or fix json lib #110

Closed
ethanfrey opened this issue Jan 13, 2020 · 13 comments · Fixed by #314
Closed

Reconsider or fix json lib #110

ethanfrey opened this issue Jan 13, 2020 · 13 comments · Fixed by #314
Assignees
Milestone

Comments

@ethanfrey
Copy link
Member

ethanfrey commented Jan 13, 2020

I made serde-json-wasm for small size, and to avoid all floats (determinism). We now enforce this determinism, so serde-json will fail to compile with the singlepass toolchain.

However, I keep finding minor incompatibilities when pushing serde-json-wasm. It has basic functions and should not be treated like a drop-in.

In particular, adjacent types in serde don't work, and now I find they don't use base64 for encoding binary data, but a format like: [124, 2, 217, 12, ..], which is causing issues with go compatibility CosmWasm/wasmvm#31 (comment)

Time to see if we can just strip out the floats from serde-json and get determinism with full features. Or if we want to ensure all functions work in the stripped down lib

Update: issue #122 (pr #133) fixes the encoding issue by making Base64 explicit. We can still consider this for the other issues, but that pressing issue is being addressed

@ethanfrey ethanfrey changed the title Reco Reconsider or fix json lib Jan 13, 2020
@webmaster128 webmaster128 added this to the 0.8.0 milestone Feb 25, 2020
@webmaster128
Copy link
Member

Another limitation of serde-json-wasm is the lack of a generic valid JSON document type, like serde_json::value::RawValue. This could be useful for things like query results.

@ethanfrey
Copy link
Member Author

It would be an interesting spike to see:

(1) what the size is swapping out the serde-json-wasm lib with standard serde-json?
(2) double check that this does get rejected by the deterministic middleware (I remember floats, but always good to validate)
(3) spend 1/2 day to investigate how hard it would be to remove all float-producing code (metric:acceptable by the deterministic middleware)

@ethanfrey
Copy link
Member Author

@webmaster128 this is not top priority, but curious as to which tasks to schedule for myself.

Would you be interested in this spike, or should I do it later?

@webmaster128
Copy link
Member

webmaster128 commented Apr 2, 2020

I suggest starting by keeping serde-json-wasm in the contract and using serde-jsin on the VM side. This would be a useful first step, even if serde-json is not practical within contracts. And it builds up knowledge about the differences of the two. I can do that, but you can do as well.

@ethanfrey
Copy link
Member Author

That's a good simple step that avoids forking a large lib.

@ethanfrey
Copy link
Member Author

I will do tomorrow

@ethanfrey ethanfrey self-assigned this Apr 3, 2020
@ethanfrey ethanfrey removed their assignment Apr 3, 2020
@ethanfrey ethanfrey removed this from the 0.8.0 milestone Apr 9, 2020
@webmaster128
Copy link
Member

Near was exploring this path already:

@ethanfrey
Copy link
Member Author

Looking into this, and all the serialization and signing discussions with cosmos-SDK, that we should not make any changes now.

Let's take a deeper look into serialization for the 0.9 or 0.10 release.

@webmaster128
Copy link
Member

webmaster128 commented Apr 28, 2020

I agree. There are small obvious bugs I'd fix (like escaping " in strings) but keep it as is mostly.

@webmaster128 webmaster128 added this to the 0.8.0 milestone Apr 29, 2020
@webmaster128 webmaster128 self-assigned this Apr 30, 2020
@webmaster128
Copy link
Member

I think maintaining out own JSON implementation is a dead end. While escaping strings during serialization is relatively easy, the interpretation of input strings is much more complex. So much that can go wrong. Even if we manage to get it done somehow, we can never compete with another well-maintained library in terms of correctness.

In webmaster128/serde_json_wasm#1 I tried patching out the float functionality from serde_json, which seems to work and should be maintainable.

@ethanfrey
Copy link
Member Author

You did this in one day?

🔥

I should have done this before taking the wasm core approach....

I gotta look at your fork

@webmaster128
Copy link
Member

Half the day was wasted trying to implement deserializing ;)

@webmaster128
Copy link
Member

After all the pain with patching serde_json, especially the undirected trial and error work, I'm mostly back to implement string unescaping in json-serde-wasm. The deteaction of the string end is finished. Unescaping cannot be super hard from there. Then we at least have the choice between the two libs.

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.

2 participants