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

Update serde-json-wasm #1297

Merged
merged 2 commits into from
May 5, 2022
Merged

Update serde-json-wasm #1297

merged 2 commits into from
May 5, 2022

Conversation

hashedone
Copy link
Contributor

No description provided.

@webmaster128
Copy link
Member

You can use those scripts to update and test all dev contracts:
Bildschirmfoto 2022-05-05 um 17 10 57

Are contracts using serde-json-wasm directly? Do they need to upgrade their projects?

Could you add a CHANGELOG entry please?

@hashedone
Copy link
Contributor Author

Yea, I was on it, but didn't aware about scripts. Just forgot to change it to draft. Thanks for hint.

@hashedone hashedone marked this pull request as draft May 5, 2022 15:41
@ueco-jb ueco-jb changed the title Update serde-json-update Update serde-json-wasm May 5, 2022
@hashedone
Copy link
Contributor Author

Now it is ready.

@hashedone hashedone marked this pull request as ready for review May 5, 2022 16:20
@ueco-jb
Copy link
Contributor

ueco-jb commented May 5, 2022

Could you add a CHANGELOG entry please?

Do we want to include in changelog minor dependency update? It's not been the case so far as I can see.

@webmaster128
Copy link
Member

Do we want to include in changelog minor dependency update? It's not been the case so far as I can see.

I'm not sure if it affects our users. Is anyone installing that dependecy directly and needs to update contracts? Or is this only used transitive?

Seems like users are unaffected as long as they use our wrappers:

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

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

pub fn to_vec<T>(data: &T) -> StdResult<Vec<u8>>
where
    T: Serialize + ?Sized,
{
    serde_json_wasm::to_vec(data).map_err(|e| StdError::serialize_err(type_name::<T>(), e))
}

pub fn to_binary<T>(data: &T) -> StdResult<Binary>
where
    T: Serialize + ?Sized,
{
    to_vec(data).map(Binary)
}

Copy link
Member

@webmaster128 webmaster128 left a comment

Choose a reason for hiding this comment

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

I still think this deserves a ## Fixed entry since it fixes serde bugs in our to_vec and to_binary.

@webmaster128
Copy link
Member

webmaster128 commented May 5, 2022

Will add a CHANGELOG entry on main. Thank you!

@webmaster128 webmaster128 merged commit 59cf396 into main May 5, 2022
@webmaster128 webmaster128 deleted the serde-json-update branch May 5, 2022 19:22
@ethanfrey
Copy link
Member

btw, I just released a minor patch (v0.4.1) which includes the fix so u128/i128 work properly (merged like 6 weeks ago, but never in a tagged release).

This should be pulled in automatically, but maybe we can set it to 0.4.1 (also in cosmwasm-template) to ensure people don't have issues there. (Serialising produced unparsable json)

@webmaster128
Copy link
Member

webmaster128 commented May 6, 2022

Yeah saw this too. I got an update 0.3.x -> 0.4.1 automatically in a consumer project. It's probably hard to install 0.4.0 instead of 0.4.1 now. Let's update but with low prio.

@hashedone
Copy link
Contributor Author

@webmaster128 it is not hard, but you need to do it intentionally ("=0.4.1" or playing directly with Cargo.lock)

@webmaster128
Copy link
Member

Yeah, that's what I meant. Hard to do by accident.

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 this pull request may close these issues.

4 participants