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

Remove bytes (Vec<u8>) in json interface #122

Closed
ethanfrey opened this issue Jan 20, 2020 · 7 comments
Closed

Remove bytes (Vec<u8>) in json interface #122

ethanfrey opened this issue Jan 20, 2020 · 7 comments
Assignees
Milestone

Comments

@ethanfrey
Copy link
Member

ethanfrey commented Jan 20, 2020

BREAKING CHANGE: 0.7

Binary encoding is not really json, although most libs use base64. It is also somewhat inefficient.

I note that the serde-json-wasm library encodes bytes like [15, 182, 8, 72, 202, ...], as an array of ints.

I notices a large gas cost rise with the CanonicalAddr as binary.

Let's just make all "binary" fields as strings in the definitions, and define a proper encoding for them (eg. base64) if we need it. We can either parse inside wasm (not inside another serde lib) when read, or just treat it as opaque bytes.

This also should update the return value of query, which is a Vec<u8> and integer encoded right now.

We can use a type Base64(String) newtype to make this encoding more clear.


Note: I only aim at structs that are serialized to json. We copy eg InitMsg as raw bytes over the wasm boundary - it is json text ascii encoded, we parse it later, not base64-ing here.

@webmaster128
Copy link
Member

The query result is converted into a list of integer as well (due to serialization of QueryResult::Ok), which can easily consume large amounts of memory.

Rust (implicitly via serde): https://github.com/confio/cosmwasm/blob/b51e89715847ee94b3c197da6601a11eb44ebdac/src/exports.rs#L141
AssemblyScript (explicit encoding for now): https://github.com/confio/cosmwasm/blob/b51e89715847ee94b3c197da6601a11eb44ebdac/contracts/assemblyscript-poc/contract/src/index.ts#L10-L24

@ethanfrey
Copy link
Member Author

Yes, good point. Will add query return to the list.

I think adding a type Base64(String) would help make this clearer

@webmaster128
Copy link
Member

webmaster128 commented Jan 30, 2020

If we use separate return types for smart queries and raw queres, we can avoid the binary issue there in a more elegant way: By convention smart query results are JSON, i.e. are strings. So for the query result {"balance": "123"} we don't pass

{
  "ok": "eyJiYWxhbmNlIjogIjEyMyJ9Cg=="
}

but

{
  "ok": "{\"balance\": \"123\"}"
}

by chaning

pub enum QueryResult {
    Ok(Vec<u8>),
    Err(String),
}

to

pub enum QueryResult {
    Ok(String),
    Err(String),
}

@ethanfrey
Copy link
Member Author

I think you meant Ok(Vec<u8>) on the top QueryResult.

The reason I don't do that is that I tried... and it turns out the simple json encoder doesn't escape strings. So {"balance": "123"} gets encoded to {"ok": "{"balance": "123"}"} but that is clearly invalid.

I would look at ripping it out, but that means forking the standard json parser to remove all references to floats (which add invalid ops to wasm, and bloat size incredibly). For now, I would just use Base64 as the interchange format for anything that needs to go into json. We can use something like String or RawJson in the future. But that requires #110

As a first step, I would just like to make sure all binary is encoded decently. The other one is a bigger project, maybe inside 0.7, maybe later....

@webmaster128
Copy link
Member

I think you meant Ok(Vec) on the top QueryResult.

👍 updated

it turns out the simple json encoder doesn't escape strings

Oh, that is broken

But that requires #110

Okay, good to know

@ethanfrey
Copy link
Member Author

I would actually like to use something like RawJson, so it ensures this is valid json then just embeds it.

This is available as a feature flag in recent versions of serde-json. Giving more reason to revisit #110. However, doing major surgery on that library is a daunting task.

@ethanfrey
Copy link
Member Author

Closed by #139

@webmaster128 webmaster128 added this to the 0.7.0 milestone Feb 25, 2020
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

No branches or pull requests

2 participants