Skip to content

Create cosmwasm-core crate #2054

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

Merged
merged 42 commits into from
Apr 9, 2024
Merged

Create cosmwasm-core crate #2054

merged 42 commits into from
Apr 9, 2024

Conversation

aumetra
Copy link
Member

@aumetra aumetra commented Mar 13, 2024

Closes #2041

Rough draft for now. Some uglypoints, such as the weird __internal module definitely need bikeshedding.

TODO:

  • Align version between cosmwasm-core and cosmwasm-std
  • Add CI tests for cosmwasm-core, including no_std tests
  • Add cosmwasm-core to table of crates in repo root README.md
  • Move types
    • Binary
    • HexBinary
    • Addr
    • CanonicalAddr
    • Uint*/Int*
  • Make base64/hex encoders/decoders public (see Implement From<&Vec> for Binary and HexBinary #2036 (review))?

@aumetra
Copy link
Member Author

aumetra commented Mar 13, 2024

The build for no_std fails because of the forward_ref crate (see jtempest/forward_ref#2)
Funnily enough the linked PR was originally made because of a precursor of this change.

We can either:

  • get rid of it and manually roll the impls
  • vendor the macros
  • fork and maintain it ourselves

I'm guessing there's not a lot to maintain since the crate just copied internal macros from the STL

@webmaster128
Copy link
Member

  • get rid of it and manually roll the impls

I would prefer that approach as it allows us to ensure the implementation serves our often-not-trivial needs. If you look at packages/std/src/forward_ref.rs we already have a place where those things can go.

@aumetra aumetra force-pushed the aw/core-crate branch 2 times, most recently from 00882e6 to eb021c8 Compare March 14, 2024 09:20
@aumetra
Copy link
Member Author

aumetra commented Mar 14, 2024

For some reason the derivative crate just didn't want to cooperate? (some weird compile errors complaining about modules not being found in core while they are clearly available there)
So I simply replaced them with derive_more which is already used in the core crate to provide the thiserror-like From and Display impls.

And it's one less dependency with the syn v1 requirement.

@aumetra
Copy link
Member Author

aumetra commented Mar 14, 2024

Last bit of bikeshedding: Do we want to remove the error cases of things that aren't handled in std anymore?
Right now I kept them due to semver compatibility. But that is kinda broken anyway since there was no #[non_exhaustive] annotation above the error, meaning a new case is technically semver breaking since it breaks exhaustive matches.

Well, I would just go ahead and keep them since exhaustive matches are a niche usecase but removing cases could have major implications.

@aumetra aumetra marked this pull request as ready for review March 14, 2024 14:26
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.

Nice work!

My main worry here is that looking at a bunch of details we can't easily hide away the extra crate from the contract developer, which means more comlpexity to handle for them. Not sure if there is anything we can do to improve the situation.

@aumetra aumetra force-pushed the aw/core-crate branch 7 times, most recently from 0be49cd to 248ee3f Compare March 15, 2024 10:26
@aumetra
Copy link
Member Author

aumetra commented Mar 15, 2024

Looking at the codecov diff, it seems like the difference mostly stems from the fact that the schemars derive has to be gated behind the std feature in a separate line.
Since most of the JSON Schema doesn't really have tests, the coverage misses add up (+0.10-0.20% per file which had to be modified)

@aumetra
Copy link
Member Author

aumetra commented Mar 15, 2024

For what it's worth, I think snapshot testing for the JSON schema is worthwhile, but IMO outside the scope of this PR.

@aumetra aumetra force-pushed the aw/core-crate branch 4 times, most recently from 50d63e6 to 389e92b Compare March 15, 2024 13:39
@aumetra aumetra requested a review from webmaster128 March 15, 2024 16:16
Copy link
Collaborator

@chipshort chipshort left a comment

Choose a reason for hiding this comment

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

Nice work!

@aumetra
Copy link
Member Author

aumetra commented Apr 4, 2024

God, that rebase was something..

@aumetra
Copy link
Member Author

aumetra commented Apr 5, 2024

Oh good, the commit finally made it. GitHub really crashed for a bit there, huh?

@aumetra aumetra merged commit 4dec802 into main Apr 9, 2024
29 of 30 checks passed
@aumetra aumetra deleted the aw/core-crate branch April 9, 2024 15:46
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.

Create cosmwasm-core
3 participants