-
Notifications
You must be signed in to change notification settings - Fork 347
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 for cosmwasm-std
#1483
Conversation
would this help to have follow up changes, for example |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Great work, thanks a lot.
I'm pretty impressed by your work re-implementing cosmwasm-vm. Congratz and respect. As mentioned in #1484 this is a valuable step to ensure the codebases converge instead of diverge. The diff might look big to some people, but I think it is tiny compared to a cosmwasm-std copy. Looking forward to see your team contributing to the main codebase!
I have a few comments inline. Some follow-up items are
- Add no_std CI jobs (at least build and unit test of cosmwasm-std)
- Add CHANGELOG entry
@webmaster128 Hi Simon, thank you for your kind words! I thought that
This will make it easier to maintain functionality of |
Would you guys mind copying over the relevant parts of the mininal package discussion to #1484 such that we have continue the design discussion there? Feels like we are about to close here soon. |
b87282b
to
25faa8c
Compare
Hey @webmaster128, I realized that splitting this will bring too many changes as well as some breaking ones. So it is best to stick with a single
|
This is in line with my thinking in #1484 (comment). Happy to continue like this.
Right, sure. Features are not for disabling something. We'll have to think about semver in this case but a source code breaking 2.0 is upcoming anyways.
The resolver is good. It's better to use v2 for CosmWasm things. We had many issues with v1 before. I just thought "2" is the default resolver in Rust edition2021 projects. |
I also thought about that but it doesn't work without the resolver. I think this is the reason. |
Let's add a |
Agreed, if you are not truly on a |
bbbe184
to
21351cc
Compare
@webmaster128 now that everything seems to be working fine, what do you think about semver? Should this be a part of 2.0? |
Yeah, let's add the |
Signed-off-by: aeryz <[email protected]>
Signed-off-by: aeryz <[email protected]>
Signed-off-by: aeryz <[email protected]>
Signed-off-by: aeryz <[email protected]>
Signed-off-by: aeryz <[email protected]>
Signed-off-by: aeryz <[email protected]>
Signed-off-by: aeryz <[email protected]>
Signed-off-by: aeryz <[email protected]>
Signed-off-by: aeryz <[email protected]>
Signed-off-by: aeryz <[email protected]>
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thank you for this. Just saw the PR. What is the status here?
I ask as there is another PR CosmWasm/cw-storage-plus#26 that would build on such.
@@ -39,22 +39,23 @@ cosmwasm_1_1 = [] | |||
# This feature makes `GovMsg::VoteWeighted` available for the contract to call, but requires | |||
# the host blockchain to run CosmWasm `1.2.0` or higher. | |||
cosmwasm_1_2 = ["cosmwasm_1_1"] | |||
std = ["forward_ref", "schemars", "serde-json-wasm", "thiserror", "cosmwasm-crypto"] |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I see removing stuff from no_std build, but I think both serde-json-wasm
is essential for it to work. Maybe a PR to make that no-std friendly (it was forked from serde-json-core which was no-std and we added stuff).
Otherwise, how would a contract actually parse data? How would cw-storage-plus work?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
thiserror is a bit sad to lose also, but I guess not absolutely needed here?
Not sure how StdError will interact with other types without it.
|
||
#[derive(Error, Debug)] | ||
#[cfg_attr(feature = "std", derive(thiserror::Error))] |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
In no-std, StdError: Error
will not be implemented, right?
I think it would be good to make some alternative implementation for no-std, so we can at least have to_string()
and treat it like an error.
@ethanfrey @webmaster128 I don't work at composable anymore and don't have access to the fork. cc @dzmitry-lahoda for further discussions. |
I replacing this PR with new one. |
I don't have a need for this but happy to include it in cosmwasm-std 2.0 (soonish). I'd like to reduce the negative impact of our regular non-no_std users. Adding a |
@webmaster128 soonish is when? |
@webmaster128 what breaking change are you referring too? |
This summer, June/July. Worst case August. The rest of 2.0 is tracked here and will be burned down after 1.3 is out of the door.
Adding a |
@webmaster128 i am doing other way. only if feature is that better? (if september would be target we need to think about other strategy to handle things). |
no-std hack not worked, also i have patched
so we will be on forks |
We are now working towards a split into cosmwasm-core (no_std compatible) and cosmwasm-std. CosmWasm 2.1 will come with a first but almost empty cosmwasm-core. See #2136 for how we plan address the centralization issue of StdError. Thanks everyone for pushing this discussion. Closing here |
Added
no_std
support tocosmwasm-std
so thatno_std
projects can use it like our cosmwasm-vm. Note that it doesn't change anything when it compiles forstd
and sincestd
is added to the default features, none of existing users ofcosmwasm-std
will be effected.