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

Fix: remove formatting in serde custom error handler #53

Closed
wants to merge 2 commits into from

Conversation

chris-ricketts
Copy link

Please see #42

This has solved a lot of float inclusion issues for me, it'd be great to get it merged upstream.

Copy link
Member

@ethanfrey ethanfrey left a comment

Choose a reason for hiding this comment

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

This deserves a review, but I've been busy. Maybe @hashedone can take a look.

If this helps resolve #43 that would be awesome.

However, I wonder a bit about the information being thrown away. The messages like found 'send' but expected one of 'increment', 'decrement' which appear when calling with missing variant of Query/ExecuteMsg are extremely valuable for debugging.

If that info is lost with these changes, that would be a hard tradeoff.

Could you add a test of the output when eg. deserailizing {"send": {"amount": "12345"}} into

enum ExecuteMsg {
  Increment{},
  Reset{ value: u32 }
}

Check that before and after your change.

@hashedone
Copy link

hashedone commented Mar 13, 2023

I get how this solves your problem, but there is a particular thing I don't like about it - it needs to include the error information. I am wondering how one uses the Custom variant, so it generates f64 output. It may be a case that it is not possible in a reasonable way to prevent it, but before I decide to merge it like this, I want to see an actual minimal example of what this causes. I reviewed your description here, but I only found your debugging process without the failing contract example. Note that if the problem is, for example, when you use the Value type (a known issue - as you know reading my article you linked), we have another crate to solve without losing error information: https://github.com/CosmWasm/serde-cw-value.

Summarizing - I am very cautious about merging this PR and not having an actual failing example so I can verify this fix is needed.

@hashedone hashedone self-requested a review March 13, 2023 21:46
@hashedone hashedone added the question Further information is requested label Mar 13, 2023
@chris-ricketts
Copy link
Author

Understood on both points.

The Custom variant is more important when it comes to contract interfaces than internal state serialisation.

One example of where it fails for internal state serialization is if you have a type that includes std::num::NonZeroU128 (useful if you are following the 'parse, don't validate' principle).

I'm now using this for internal state in my projects (https://github.com/v26-solutions/kv-storage) and just using cosmwasm-std (serde-json-wasm) at the edges.

Closing out this PR in favor of the above.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
question Further information is requested
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants