f32/f64/usize in cosmwasm(and other no f32/f64 envs) reproduction and fixes #803

dzmitry-lahoda · 2023-08-01T15:25:58Z

Closes: #800

Description

Requires #790

replaced f64 logic with equivalent u64
forbidden use of f64 (and usize)
usize was replaced by u64, assuming that wasm64 is nearest highest target for mem size. i am ok with safety of thing, so u32/u16 could be better per case (mostly length on names and some history things - sure never u64, but u64 is correct map to usize), so no ideal but working.
replaced weird serde issues (both serde and serde_wasm compile into wasm f64 even if do just strings, had to rip them as could)
added test to test changes (with these there is no validation error)
side change added schema for ChainId (i use in CW)

PR author checklist:

Added changelog entry, using unclog.
Added tests.
Linked to GitHub issue.
Updated code comments and documentation (e.g., docs/).
Tagged one reviewer who will be the one responsible for shepherding this PR.

Reviewer checklist:

Reviewed Files changed in the GitHub PR explorer.
Manually tested (in case integration/unit/mock tests are absent).

dzmitry-lahoda · 2023-08-01T17:54:18Z

feel free to merge with #790 and run insuction in shell.nix header. try to rollback any change and see effects. serde works exactly only this way i found, other compile with floats

dzmitry-lahoda · 2023-08-01T18:20:18Z

for usize correctness parity uses static assets

substrate/primitives/runtime-interface/src/impls.rs:45:1

so can asset some usize if needed too

dzmitry-lahoda · 2023-08-01T20:16:01Z

pr has issue

"serde-json-wasm 0.5.1 (git+https://github.com/dzmitry-lahoda-forks/serde-json-wasm?rev=8a7e522c0e4de36a6dfb535766f26a9941017d81)",

need to use no-std serde-json-wasm

dzmitry-lahoda · 2023-08-01T20:17:05Z

CosmWasm/serde-json-wasm#58

CosmWasm/serde-json-wasm#56

we trying to get no_std merged for 1 year+ already so

…y fork pr not merged for no std

Some explanation is here cosmos/ibc-rs#803 So wasmd validation in cosmos will not find reference to code (which never executes) it fails like floats anymore and will success upload. Any attempt to strip float code failed (likely it anticipates input which may contain floats in some case, which is not case to handle at all). Also had hard times with std because of centauri repo, added CI checks to ensure we always good. May be fixes #3952 too. Required for merge: - [x] `pr-workflow-check / draft-release-check` is ✅ success - Other rules GitHub shows you, or can be read in [configuration](../terraform/github.com/branches.tf) Makes review faster: - [x] PR title is my best effort to provide summary of changes and has clear text to be part of release notes - [x] I marked PR by `misc` label if it should not be in release notes - [x] Linked Zenhub/Github/Slack/etc reference if one exists - [x] I was clear on what type of deployment required to release my changes (node, runtime, contract, indexer, on chain operation, frontend, infrastructure) if any in PR title or description - [x] Added reviewer into `Reviewers` - [x] I tagged(`@`) or used other form of notification of one person who I think can handle best review of this PR - [x] I have proved that PR has no general regressions of relevant features and processes required to release into production - [x] Any dependency updates made, was done according guides from relevant dependency - Clicking all checkboxes - Adding detailed description of changes when it feels appropriate (for example when PR is big)

Farhad-Shabani

Hi @dzmitry-lahoda. This is awesome and much appreciated! 👌🏻
Let’s get it over the finish line. Would you plz apply the following changes:

Merge changes from the main to the PR's branch
Set serde-json-wasm package to use v1.0.0
Add a CI job that does cw_check by running the shell.nix
Add unclog

Then, I will run an integration test against gaia chains in basecoin-rs to make sure using serde-json-wasm we still keep compatibility with both Hermes and basecoin_app (Though, from what I see most probably should be ok)

Let me know if for any of above, I can assist

Farhad-Shabani · 2023-10-02T21:13:22Z

Hey @dzmitry-lahoda, Any chance you could wrap up this PR?
I also can assist with this if you select "Allow edits from maintainers".

Farhad-Shabani · 2023-10-02T21:23:51Z

crates/ibc/src/applications/transfer/amount.rs

@@ -15,7 +15,7 @@ use crate::alloc::{borrow::ToOwned, string::String};
 #[derive(Copy, Clone, Debug, Eq, PartialEq, PartialOrd, Ord, Display, From, Into)]
 pub struct Amount(
    #[cfg_attr(feature = "schema", schemars(with = "String"))]
-    #[serde(serialize_with = "crate::serializers::serde_string::serialize")]
+    #[serde(serialize_with = "serialize")]


Taking another peek, the "serialize" function in "amount.rs" seems redundant. Why not use "crate::serializers::serde_string::serialize"?

afaik for some reason it triggered float to be compiled into wasm (seems like just patter matching, but validation tools do not care).

Would be OK if the ci/cw-check passes with the crate::serializers::serde_string::serialize or still might be the case?

would be OK

i used only reproducible check to fix issues

dzmitry-lahoda · 2023-10-02T21:23:56Z

hello, some things

I cannot allow edit because i have -forks org I do all forks (to avoid mix of my projects), and there is bug https://github.com/orgs/community/discussions/5634 . I cannot set the value. So I am ok with copy paste or else.
https://github.com/CosmWasm/serde-json-wasm was released version 1.0. they made it no_std afaid and fixed some issue - so this one should be used.
and final one, latest version of CosmWasm wasmd emulated floats (software implementation I guess). But I am not sure it works in no_std or without using their wasmd wasmer host or without using special assembly rewrite hook. So float less wasm still a thing, so for some people may be less prio.

but in general things I tried still holds, for next 1 month i am on ibc for solidity, so will not catch up this minimum this period, i guess company will continue more efforts to no_std versions of ibc in 2-3 month (after evm).

Farhad-Shabani · 2023-10-02T21:42:16Z

Thanks a bunch for the tips and updates. 🙏 No worries, I’ll fork yours and open up a new PR.
Just, as for the 3rd point, given another issue we've got here, still we should move away from floats.

Looking forward to hearing about the progress on the no_std versions of IBC.

Farhad-Shabani · 2023-10-02T21:50:23Z

Superseded by #894

fixed f64/f32 validation in wasm

c580d56

dzmitry-lahoda mentioned this pull request Aug 1, 2023

fixed f32/f64 issue in cosmwasm contracts validation ComposableFi/composable#4003

Merged

9 tasks

what to do with serde json? main one is float, fork from cw is std, m…

7a6837c

…y fork pr not merged for no std

Farhad-Shabani mentioned this pull request Sep 5, 2023

serde_json uses floats that may be disallowed in WASM #850

Closed

Farhad-Shabani requested changes Sep 6, 2023

View reviewed changes

dzmitry-lahoda marked this pull request as draft September 10, 2023 13:51

Farhad-Shabani reviewed Oct 2, 2023

View reviewed changes

Farhad-Shabani mentioned this pull request Oct 2, 2023

f32/f64/usize in cosmwasm(and other no f32/f64 envs) reproduction and fixes #894

Merged

7 tasks

Farhad-Shabani closed this Oct 2, 2023

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

f32/f64/usize in cosmwasm(and other no f32/f64 envs) reproduction and fixes #803

f32/f64/usize in cosmwasm(and other no f32/f64 envs) reproduction and fixes #803

dzmitry-lahoda commented Aug 1, 2023 •

edited

Loading

dzmitry-lahoda commented Aug 1, 2023

dzmitry-lahoda commented Aug 1, 2023

dzmitry-lahoda commented Aug 1, 2023

dzmitry-lahoda commented Aug 1, 2023

Farhad-Shabani left a comment

Farhad-Shabani commented Oct 2, 2023

Farhad-Shabani Oct 2, 2023

dzmitry-lahoda Oct 2, 2023

Farhad-Shabani Oct 2, 2023

dzmitry-lahoda Oct 2, 2023

dzmitry-lahoda Oct 2, 2023

dzmitry-lahoda commented Oct 2, 2023

Farhad-Shabani commented Oct 2, 2023

Farhad-Shabani commented Oct 2, 2023

f32/f64/usize in cosmwasm(and other no f32/f64 envs) reproduction and fixes #803

f32/f64/usize in cosmwasm(and other no f32/f64 envs) reproduction and fixes #803

Conversation

dzmitry-lahoda commented Aug 1, 2023 • edited Loading

Description

PR author checklist:

Reviewer checklist:

dzmitry-lahoda commented Aug 1, 2023

dzmitry-lahoda commented Aug 1, 2023

dzmitry-lahoda commented Aug 1, 2023

dzmitry-lahoda commented Aug 1, 2023

Farhad-Shabani left a comment

Choose a reason for hiding this comment

Farhad-Shabani commented Oct 2, 2023

Farhad-Shabani Oct 2, 2023

Choose a reason for hiding this comment

dzmitry-lahoda Oct 2, 2023

Choose a reason for hiding this comment

Farhad-Shabani Oct 2, 2023

Choose a reason for hiding this comment

dzmitry-lahoda Oct 2, 2023

Choose a reason for hiding this comment

dzmitry-lahoda Oct 2, 2023

Choose a reason for hiding this comment

dzmitry-lahoda commented Oct 2, 2023

Farhad-Shabani commented Oct 2, 2023

Farhad-Shabani commented Oct 2, 2023

dzmitry-lahoda commented Aug 1, 2023 •

edited

Loading