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

no_std support #26

Closed

Conversation

dzmitry-lahoda
Copy link

@dzmitry-lahoda dzmitry-lahoda commented Dec 25, 2022

@dzmitry-lahoda
Copy link
Author

cosmwasm-std should somehow provide trait Storage in no_std (may be other)

@dzmitry-lahoda dzmitry-lahoda changed the title #draft #wip no_std support #draft no_std support Jan 3, 2023
@dzmitry-lahoda dzmitry-lahoda marked this pull request as ready for review January 3, 2023 15:23
@chipshort
Copy link
Collaborator

Thank you for working on this. What exactly is your goal with having no_std support?
Also, why have all those conditional imports in that cw_std module? I think it makes more sense to use alloc and core directly, even in an std environment.

@dzmitry-lahoda
Copy link
Author

i want to execute party dotsama xcm in cw contract. i was not able to compule wasm until no_std used inside cw(and storage). will recheck later again.

@dzmitry-lahoda
Copy link
Author

@chipshort i am trying to run CW contracts codebase(serde_json_wasm, sw-plus, cw-std, cw-storage-plus) in Substrate runtime. Currently there is no way to compile any CW stuff to no_std environment. Basically CW really no_std, but import STD stuff. That leads to conflict with no_std libs importing their own alloc stuff.

I want to some compile CW codes (for precompiles on top of Substrate modules), but cannot because of

error[E0152]: duplicate lang item in crate `std` (which `serde_json_wasm` depends on): `panic_impl`.
  |
  = note: the lang item is first defined in crate `sp_io` (which `frame_support` depends on)
  = note: first definition in `sp_io` loaded from /home/dz/github.com/ComposableFi/composable/code/target/wasm32-unknown-unknown/debug/deps/libsp_io-3c7e30a042e25ab0.rmeta
  = note: second definition in `std` loaded from /nix/store/r0jvsra41lh1kvpxszjss9zcdci86zh1-rust-std-1.67.0-nightly-2022-11-17-wasm32-unknown-unknown/lib/rustlib/wasm32-unknown-unknown/lib/libstd-65032bf6a898adaa.rlib

in whole stack.

@dzmitry-lahoda
Copy link
Author

likely direct usage of alloc will work, but need to make it to be used in. ok with that. but i am not sure if that will be risk no to work in some case.

cw-std
serde-wasm-json
cw-plus
cw-storage-plus

and check on CI it does not violates.

this will allow to run CW on more chains.

@ethanfrey will you accept such patches to ALL packages? you mentioned it is not prio for you to expand CW to other chain previosly. did anything changed?

@ethanfrey
Copy link
Member

@dzmitry-lahoda can you explain your usecase more?

https://github.com/ComposableFi/ has written a substrate pallet that runs cosmwasm contracts (the contracts are imported as wasm, and can use the full std... the pallet is no-std)

This is the most obvious integration and is already done. It is live on https://www.picasso.xyz

But I am guessing you want something else? You want to embed contracts directly in a pallet? Why? As pallets don't allow permissionless uploading, which is a major purpose of cosmwasm.

@ethanfrey
Copy link
Member

In general, I do not want to take on larger patches that increase the maintainability of all packages and users for one niche use case (or someone's personal experiment). If you can convince me this is an important use-case, I would consider.

@dzmitry-lahoda
Copy link
Author

technical use case is to create contract which can dispatch scale encoded transaction to pallets, but i cannot compile auch contract if i mix parity and cw crates.

other use case, share some code, like ibc or manually coded adapter to imitate contract to real cw contracts. i cannot share code too. have to maintain forks.

solution which parity uses is no std feature flags. same can be for other no std like near or solana 9r even offchain wasm cloud stuff. i guess.

the thing is that cw does no std, wasm specifically, not in mainline with official rust wasm support. because there is cargo wasm which compiles wasm without std and cannot compile cw contracts which are not std.

so cw is really not std, but does not want to collaborate with other more correct no std implementations.

@dzmitry-lahoda
Copy link
Author

so use case, hm, running cw somewhere else except cosmos...

@dzmitry-lahoda
Copy link
Author

cw is no_std. which is official rust and cargo way of doing not_std. parity follows that rule, cw not, and hence not compatible with cargo wasm tooling and no_std crates.

@dzmitry-lahoda
Copy link
Author

dzmitry-lahoda commented Jun 1, 2023

btw, i will not have any influence on any contracts as long as they use default features. all compilation will be as before. as i see cw people use default features.

so it concerns only 2 packages , cw-std and cw-storage-plus.

@dzmitry-lahoda
Copy link
Author

near/borsh#108

@dzmitry-lahoda
Copy link
Author

@dzmitry-lahoda
Copy link
Author

aptos no_std https://github.com/search?q=org%3Aaptos-labs+no_std&type=code

no std does not have hash maps with rand, so it will fail to compile bad contracts more explicitly.

@ethanfrey
Copy link
Member

the thing is that cw does no std, wasm specifically, not in mainline with official rust wasm support. because there is cargo wasm which compiles wasm without std and cannot compile cw contracts which are not std.

so cw is really not std, but does not want to collaborate with other more correct no std implementations.

This is not true. We have std (and all memory management there), just not sys calls (like random, time, etc) which would need host functions.

When we started writing this in 2019, no_std was lacking on many crates and forcing no_std would have seriously limited external libraries people could import. I see the story is better now.

@ethanfrey
Copy link
Member

The only use case that really convinced me there is that a CosmWasm contract would want to import Polkadot/NEAR code in order to interact with those chains, decode/encode tx, validate headers, etc.

I don't assume CosmWasm contracts would be embedded in other Rust code directly, but only as a WASM blob, which could run in various runtimes. So the main purpose (which should be tested along with this) is importing no_std-only libraries in a contract.

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 looks solid to me in general. A few points:

  1. This cannot be merged with a reference to a cosmwasm-std fork. We need to get that merged first.
  2. CI tests must be updated to run std and no_std versions of everything. (build, test, clippy)
  3. It would be nice to make a simple test demoing a use case now working well... Only on #[cfg(not(feature = "std")))] Like decoding a borsch-encoded message (embed it as raw hex) and then storing into storage-plus (JSON encoded).

I am not the maintainer of cosmwasm repo for the first point, but please link any open PR on that repo and I can ask for it to be reviewed. Points 2 and 3 are needed and I would be happy to give feedback on this PR when those are handled.

Cargo.toml Outdated

[lib]
# See https://bheisler.github.io/criterion.rs/book/faq.html#cargo-bench-gives-unrecognized-option-errors-for-valid-command-line-options
bench = false

[dependencies]
cosmwasm-std = { version = "1.1.6", default-features = false }
cosmwasm-std = { git = "https://github.com/ComposableFi/cosmwasm.git", rev = "21351cc1ced863b9af7c8a69f923036bc919b3b1", default-features = false }
Copy link
Member

Choose a reason for hiding this comment

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

Okay, before merging this, we would want to get their version merged in somehow, at least their no_std support.

I don't remember seeing a PR on that

schemars = "0.8.3"
serde = { version = "1.0", default-features = false, features = ["derive"] }
serde = { version = "1.0", default-features = false, features = ["derive", "alloc"] }
Copy link
Member

Choose a reason for hiding this comment

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

Import alloc only when no std would be better, right?

src/cw_std.rs Outdated
@@ -0,0 +1,62 @@
// no_std support
Copy link
Member

Choose a reason for hiding this comment

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

Seems like a solid way to implement this

Copy link
Collaborator

Choose a reason for hiding this comment

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

But do we really need this file? I'm pretty sure we can just use the alloc and core crates directly, even in std environment. std just re-exports them anyway.

Copy link
Member

Choose a reason for hiding this comment

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

We want to conditionally use them.
I wouldn't use core libs in std. It gets all kinds of confusion when you start passing core::Vec to a lib that expects std::Vec

If we want to support both, such a set of conditional imports seems a proper way to do it.

Copy link
Collaborator

Choose a reason for hiding this comment

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

You mean "confusion" as in: The crate user will get confused? Just asking because alloc::vec::Vec and std::vec::Vec are just two names for the same thing, so passing them around works perfectly fine.

But if you prefer importing it from std, then yes, the conditional imports file is the best solution.

src/deque.rs Outdated
@@ -291,7 +291,7 @@ where
#[cfg(test)]
mod tests {
use crate::deque::Deque;

use crate::cw_std::{vec::Vec, string::String};
Copy link
Member

Choose a reason for hiding this comment

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

How about just replacing this and the above line with use super::*

src/helpers.rs Outdated
@@ -123,6 +123,7 @@ pub(crate) fn query_raw<Q: CustomQuery>(
#[cfg(test)]
mod test {
use super::*;
use crate::cw_std::prelude::*;
Copy link
Member

Choose a reason for hiding this comment

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

This should be unnecessary due to it being defined in super, but the compiler knows better than I

@ethanfrey
Copy link
Member

I assume you looked at CosmWasm/cosmwasm#1483 (which is where your git rev comes from, right?)

This will make it easier to maintain functionality of std. std won't necessarily need to be restricted with no_std and it would make it much more easier to use a crate that doesn't support no_std (like serde_json_wasm, thiserror, etc.).

Their no_std doesn't support serde_json_wasm and we make extensive use of it for all serialization/deserialization on cw-strorage-plus. Until that is fixed or there is an alternative I don't see how this could work (... maybe another fork of serde-json-core that rips out floats but remains no_std??)

@dzmitry-lahoda
Copy link
Author

What no_std does is it improve error messages when you refer to no_std in smart contracts which do not really have js/rand/io, etc, tooling.

cw_std wrapper allows this lib easy solve when anybody wants really non core/alloc/std crates to be used for no_std (so they really can patch one file and maintain instead of code scattered.

indeed it is true that kind of core/alloc refer same std stuff, but when you compile no_std, there is no std, and it cannot compile. so each lib which is references should have no_std flag to allow whole tree to compile. so it is not only this lib, but each lib in tree.

when it appears that no_std and std libs included in same tree, it just tells that there are several panics, or some rand/js unknown things, or several memory hooks - it cannot pick one. so it solves decision for rust.

@dzmitry-lahoda
Copy link
Author

dzmitry-lahoda commented Jun 2, 2023

for use cases. there are two

  1. writing CW adapter to native contract to the chain as part of host. in this case one imitates CW contract via host API on top not really CW contracts. in this case I even do not compile any wasm file. that is just usual code for that chain. just provides different ABI. UPDATE: this pr is no exactly needed, but base pr to cosmwasm-std is. But this still may be not case if this crate has some staff to have some reflection on raw key value data. E.g. we have now issue to write good ui for data, but in theory can read something from underlying contract and have some frame to show. I did not checked that.
  2. another use case, I want to write CW contract which uses some no_std libs provided by some people. For Substrate it Substrtate Dispatch API and Substrate XCM. Dispatch allows to do 1, but inside contract - give universal access to native ABI of host chain. And XCM is sure clear - allows for XCM messages to execute things on Cosmos over IBC. Both are (and where when I checked) - no_std, I failed to compile (until this patch). There are other use cases as I see some ZK libs for RISK-V are no_std too. So that will ensure ZK stuff compiles too in CW.

@dzmitry-lahoda dzmitry-lahoda changed the title #draft no_std support no_std support Jun 8, 2023
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.

3 participants