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

Upgrade JSON library #314

Merged
merged 8 commits into from
May 7, 2020
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension


Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 2 additions & 0 deletions CHANGELOG.md
Original file line number Diff line number Diff line change
Expand Up @@ -116,6 +116,8 @@
- Add `staking` feature flag to expose new `StakingMsg` types under `CosmosMsg`
and new `StakingRequest` types under `QueryRequest`.
- Add support for mocking-out staking queries via `MockQuerier.with_staking`
- `from_slice`/`from_binary` now require result type to be `DeserializeOwned`,
i.e. the result must not contain references such as `&str`.

**cosmwasm-vm**

Expand Down
6 changes: 3 additions & 3 deletions Cargo.lock

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

6 changes: 3 additions & 3 deletions contracts/hackatom/Cargo.lock

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

5 changes: 3 additions & 2 deletions contracts/hackatom/tests/integration.rs
Original file line number Diff line number Diff line change
Expand Up @@ -339,8 +339,9 @@ mod singlepass_tests {
// Gas consumtion is relatively small
// Note: the exact gas usage depends on the Rust version used to compile WASM,
// which we only fix when using cosmwasm-opt, not integration tests.
assert!(gas_used > 26000, "{}", gas_used);
assert!(gas_used < 30000, "{}", gas_used);
let expected = 42000; // +/- 20%
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Wow, this increased gas usage a lot! (+/- 28k to +/- 42k)

Maybe it makes sense to have a hot-path when there is no escaped characters inside the string to just do the old copy. We can merge this, but it would be nice to update the json lib before 0.8 (we can do that in a week or so, but it should be possible to do around the same cost when there are no escapes)

assert!(gas_used > expected * 80 / 100, "Gas used: {}", gas_used);
assert!(gas_used < expected * 120 / 100, "Gas used: {}", gas_used);

// Used between 100 and 102 MiB of memory
assert!(deps.get_memory_size() > 100 * 1024 * 1024);
Expand Down
6 changes: 3 additions & 3 deletions contracts/queue/Cargo.lock

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

4 changes: 2 additions & 2 deletions contracts/reflect/Cargo.lock

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

2 changes: 1 addition & 1 deletion packages/std/Cargo.toml
Original file line number Diff line number Diff line change
Expand Up @@ -29,7 +29,7 @@ backtraces = ["snafu/backtraces"]

[dependencies]
base64 = "0.11.0"
serde-json-wasm = "0.1.2"
serde-json-wasm = { version = "0.2.1" }
schemars = "0.7"
serde = { version = "1.0.103", default-features = false, features = ["derive", "alloc"] }
snafu = { version = "0.6.6" }
Expand Down
2 changes: 1 addition & 1 deletion packages/std/src/coins.rs
Original file line number Diff line number Diff line change
Expand Up @@ -139,7 +139,7 @@ impl<'de> de::Visitor<'de> for Uint128Visitor {
formatter.write_str("string-encoded integer")
}

fn visit_borrowed_str<E>(self, v: &'de str) -> Result<Self::Value, E>
fn visit_str<E>(self, v: &str) -> Result<Self::Value, E>
where
E: de::Error,
{
Expand Down
2 changes: 1 addition & 1 deletion packages/std/src/encoding.rs
Original file line number Diff line number Diff line change
Expand Up @@ -77,7 +77,7 @@ impl<'de> de::Visitor<'de> for Base64Visitor {
formatter.write_str("valid base64 encoded string")
}

fn visit_borrowed_str<E>(self, v: &'de str) -> Result<Self::Value, E>
fn visit_str<E>(self, v: &str) -> Result<Self::Value, E>
where
E: de::Error,
{
Expand Down
93 changes: 84 additions & 9 deletions packages/std/src/serde.rs
Original file line number Diff line number Diff line change
Expand Up @@ -2,23 +2,17 @@
// The reason is two fold:
// 1. To easily ensure that all calling libraries use the same version (minimize code size)
// 2. To allow us to switch out to eg. serde-json-core more easily
use serde::{Deserialize, Serialize};
use serde::{de::DeserializeOwned, Serialize};
use std::any::type_name;

use crate::encoding::Binary;
use crate::errors::{parse_err, serialize_err, StdResult};

pub fn from_slice<'a, T>(value: &'a [u8]) -> StdResult<T>
where
T: Deserialize<'a>,
{
pub fn from_slice<T: DeserializeOwned>(value: &[u8]) -> StdResult<T> {
serde_json_wasm::from_slice(value).map_err(|e| parse_err(type_name::<T>(), e))
}

pub fn from_binary<'a, T>(value: &'a Binary) -> StdResult<T>
where
T: Deserialize<'a>,
{
pub fn from_binary<T: DeserializeOwned>(value: &Binary) -> StdResult<T> {
from_slice(value.as_slice())
}

Expand All @@ -35,3 +29,84 @@ where
{
to_vec(data).map(Binary)
}

#[cfg(test)]
mod test {
use super::*;
use serde::Deserialize;

#[derive(Serialize, Deserialize, Debug, PartialEq)]
#[serde(rename_all = "snake_case")]
enum SomeMsg {
Refund {},
ReleaseAll {
image: String,
amount: u32,
time: u64,
karma: i32,
},
Cowsay {
text: String,
},
}

#[test]
fn to_vec_works() {
let msg = SomeMsg::Refund {};
let serialized = to_vec(&msg).unwrap();
assert_eq!(serialized, br#"{"refund":{}}"#);

let msg = SomeMsg::ReleaseAll {
image: "foo".to_string(),
amount: 42,
time: 9007199254740999, // Number.MAX_SAFE_INTEGER + 7
karma: -17,
};
let serialized = String::from_utf8(to_vec(&msg).unwrap()).unwrap();
assert_eq!(
serialized,
r#"{"release_all":{"image":"foo","amount":42,"time":9007199254740999,"karma":-17}}"#
);
}

#[test]
fn from_slice_works() {
let deserialized: SomeMsg = from_slice(br#"{"refund":{}}"#).unwrap();
assert_eq!(deserialized, SomeMsg::Refund {});

let deserialized: SomeMsg = from_slice(
br#"{"release_all":{"image":"foo","amount":42,"time":18446744073709551615,"karma":-17}}"#,
)
.unwrap();
assert_eq!(
deserialized,
SomeMsg::ReleaseAll {
image: "foo".to_string(),
amount: 42,
time: 18446744073709551615,
karma: -17
}
);
}

#[test]
fn to_vec_works_for_special_chars() {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nice. These two

let msg = SomeMsg::Cowsay {
text: "foo\"bar\\\"bla".to_string(),
};
let serialized = String::from_utf8(to_vec(&msg).unwrap()).unwrap();
assert_eq!(serialized, r#"{"cowsay":{"text":"foo\"bar\\\"bla"}}"#);
}

#[test]
fn from_slice_works_for_special_chars() {
let deserialized: SomeMsg =
from_slice(br#"{"cowsay":{"text":"foo\"bar\\\"bla"}}"#).unwrap();
assert_eq!(
deserialized,
SomeMsg::Cowsay {
text: "foo\"bar\\\"bla".to_string(),
}
);
}
}
4 changes: 2 additions & 2 deletions packages/vm/README.md
Original file line number Diff line number Diff line change
Expand Up @@ -21,8 +21,8 @@ To rebuild the test contract, go to the repo root and do
docker run --rm -v "$(pwd)":/code \
--mount type=volume,source="$(basename "$(pwd)")_cache",target=/code/target \
--mount type=volume,source=registry_cache,target=/usr/local/cargo/registry \
confio/cosmwasm-opt:0.7.3 ./contracts/hackatom
cp contracts/hackatom/contract.wasm packages/vm/testdata/contract_0.8.wasm
confio/cosmwasm-opt:0.7.3 ./contracts/hackatom \
&& cp contracts/hackatom/contract.wasm packages/vm/testdata/contract_0.8.wasm
```

You can do the same for `reflect` and `queue` when there are breaking changes.
Expand Down
6 changes: 3 additions & 3 deletions packages/vm/src/instance.rs
Original file line number Diff line number Diff line change
Expand Up @@ -393,7 +393,7 @@ mod singlepass_test {

let init_used = orig_gas - instance.get_gas();
println!("init used: {}", init_used);
assert_eq!(init_used, 45568);
assert_eq!(init_used, 65606);
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

These are all quite huge gas increases for the safety check. I'm sure this can be optimized. But like I said, in a point release of serde-json-wasm, maybe in cosmwasm 0.8.1. This is an optimization that doesn't break any APIs

}

#[test]
Expand All @@ -417,7 +417,7 @@ mod singlepass_test {

let handle_used = gas_before_handle - instance.get_gas();
println!("handle used: {}", handle_used);
assert_eq!(handle_used, 63432);
assert_eq!(handle_used, 94480);
}

#[test]
Expand Down Expand Up @@ -452,6 +452,6 @@ mod singlepass_test {

let query_used = gas_before_query - instance.get_gas();
println!("query used: {}", query_used);
assert_eq!(query_used, 23066);
assert_eq!(query_used, 32676);
}
}
Binary file modified packages/vm/testdata/contract_0.8.wasm
Binary file not shown.