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

Second attempt to cosmwasm-core #2136

Open
webmaster128 opened this issue May 8, 2024 · 2 comments
Open

Second attempt to cosmwasm-core #2136

webmaster128 opened this issue May 8, 2024 · 2 comments
Milestone

Comments

@webmaster128
Copy link
Member

webmaster128 commented May 8, 2024

Right now on main, the existence cosmwasm-core creates all sorts of indirections an undesired dependencies that we want to avoid. So let's revert the current approach.

A new attempt to pull out cosmwasm-core that we may or may not do should have the following properties

  • does not depend on cosmwasm-crypto (not at runtime, not as dev-dependency)
  • does not pull in the StdError type

One way to do that is to ensure the error types for the relevant symbols are not StdError.

  • Avoid renaming symbols whenever possible (StdError/CoreError causes confusion)
  • from_base64 can get it's own InvalidBase64Error
  • HexBinary can get its own InvalidDataSizeError
  • For the integer and decimal types create a dedicated error type for the impl TryFrom<&str>, all the other errors are already specialized
  • ...

Those new errors can then be converted to StdError in cosmwasm-std.

@webmaster128 webmaster128 added this to the 3.0.0 milestone May 21, 2024
This was referenced May 21, 2024
@chipshort
Copy link
Collaborator

I have another proposal that is somewhat tangential to this, so leaving this here for feedback:
Right now, one annoyance with upgrading to CosmWasm 2.0 is the fact that having two versions of cosmwasm-std in the dependency tree, even if the old one is just used internally in a crate you depend on, leads to confusing linker errors like these:

= note: rust-lld: error: duplicate symbol: interface_version_8
>>> defined in /Users/christoph/Projects/test-multiple-versions/target/wasm32-unknown-unknown/release/deps/libcosmwasm_std-32a3224afcc6a9f2.rlib(cosmwasm_std-32a3224afcc6a9f2.cosmwasm_std.4d9a7148e0633f34-cgu.0.rcgu.o)
>>> defined in /Users/christoph/Projects/test-multiple-versions/target/wasm32-unknown-unknown/release/deps/libcosmwasm_std-e35aa42e4bf30cbe.rlib(cosmwasm_std-e35aa42e4bf30cbe.cosmwasm_std.c38b44a8d467ec7e-cgu.0.rcgu.o)

This happens because both versions of cw-std provide the same exports in exports.rs, which causes a conflict.
We could avoid this by doing the following:

  • moving the exports into a separate crate (let's call it cosmwasm-exports for now)
  • version that crate separately and only do minor / patch releases there
  • depend on it in cw-std and make sure it gets included in the compilation (might need re-exporting)

This should not cause too much confusion for users, since they don't usually interact directly with these exports, but cargo should then be able to resolve the cosmwasm-exports crate to one version, avoiding that problem for future major versions.

Limitations:

  • Cargo workspaces seem to be resolved differently, so I don't know if this also works there.
    When testing with CW2.0, I ran into this problem in a workspace because of our k256 dependency on an exact version (which changed between the major releases)
  • if you depend on a crate that exposes types from cosmwasm-std (e.g. Deps) in its public API, you will have a hard time in any case, since those are different types, even when they didn't change in the major cosmwasm release. It's possible to work around, but it's not pretty.

Opinions? Is this too much of a niche problem?

@webmaster128
Copy link
Member Author

Opinions? Is this too much of a niche problem?

I would absolutely address this. However, I think an easier way is to put the extern "C" fns behind a macro such that they are only expanded once (let's say in the contract's lib.rs) instead of implicitly by being a dependency.

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

No branches or pull requests

2 participants