Skip to content
This repository has been archived by the owner on Feb 14, 2021. It is now read-only.

WIP: move from serde_json to serde_json_core #59

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

Pzixel
Copy link
Contributor

@Pzixel Pzixel commented Jul 19, 2018

I have created an issue that serde_json clashes in no_std scenarios, but library authors said that they don't support no_std.

std feature should be removed from the crate (to not confuse people that it supports some kind of no_std)

See more details here serde-rs/json#463

So I tried to add required functionallity into the package that should be a no_std replacement for serde_json - serde_json_core.

It doesn't work yet (so I added WIP) but it probably would be helpful to have this PR here. I appreciate any help because now I don't quite understand what's wrong with code. All tests passed for alloc types, but when you actually use them you encounter several errors in macro invocations.

@Pzixel
Copy link
Contributor Author

Pzixel commented Jul 19, 2018

Looks like serde bug: serde-rs/serde#1339

@NikVolf
Copy link
Collaborator

NikVolf commented Jul 23, 2018

There is no need to support json in no-std, because json is generated on compile time only and json serialization/deserialization never makes it to the wasm binary

@Pzixel
Copy link
Contributor Author

Pzixel commented Jul 23, 2018

@NikVolf having dependency on serde_json makes impossible to use crates like bincode, see #54 . It's because you cannot reference serde in std and no_std scenarios. This is limitation of existing cargo, and it's unlikely that it will be changed in the future.

In my case I want to bincode some types in my contract (basically to implement generic object saving in pwasm-abstractions library), but I can't do that currently.

@Pzixel
Copy link
Contributor Author

Pzixel commented Jul 26, 2018

I have reread the derive subcrate source code. It seems that json generation should be moved to build.rs to make serde build-time dependency. It looks like eth_abi macro voliates SRP principle: it generates both source code and JSON. I see that it uses the same AST to do two things, but I actually think that it should be separated into two methods, and one of them moved into build.rs.

@lexfrl
Copy link
Contributor

lexfrl commented Jul 27, 2018

I have reread the derive subcrate source code. It seems that json generation should be moved to build.rs to make serde build-time dependency. It looks like eth_abi macro voliates SRP principle: it generates both source code and JSON. I see that it uses the same AST to do two things, but I actually think that it should be separated into two methods, and one of them moved into build.rs.

The argument behind it is following: ".json ABI is an artefact of compilation". That means we should maintain the following requirement: in case of compilation failure neither .json ABI nor Wasm binary should change.

@Pzixel
Copy link
Contributor Author

Pzixel commented Jul 27, 2018

But it currently makes serde_json runtime dependency. As i have shown it doesn't allow user to use serde in the contract. If you have other ideas i'd like to try them too.

Just try to create a contract, that serialize the tuple of U256 into byte array and save it to bc. You will face the error.

@lexfrl
Copy link
Contributor

lexfrl commented Jul 27, 2018

I think the ideal solution would be to make a generic build tool, as an extension of cargo tool (will take the same arguments as cargo and some additional), which will yield the .json ABI as well. We could call it pargo, for example.

@Pzixel
Copy link
Contributor Author

Pzixel commented Jul 27, 2018

Sounds promising

@Robbepop
Copy link
Contributor

Robbepop commented Oct 15, 2018

I think the ideal solution would be to make a generic build tool, as an extension of cargo tool (will take the same arguments as cargo and some additional), which will yield the .json ABI as well. We could call it pargo, for example.

@fckt Is this idea still a thing? If so where should we collect information about it? Maybe a new issue?

Could a cargo plugin be helpful here?

@lexfrl
Copy link
Contributor

lexfrl commented Oct 16, 2018

@Robbepop yeah, still actual. I'm thinking to make it as a subcommand like cargo pwasm-build I've started implementing it, but had to focus on Parity Signer recently, due to a release matters. This refactoring was the first move into that direction.

@lexfrl lexfrl force-pushed the master branch 2 times, most recently from efb0994 to 4d47596 Compare January 19, 2019 21:16
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants