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

feat: add no-std to rollup-interface #1077

Merged
merged 37 commits into from
Oct 25, 2023

Conversation

vlopes11
Copy link
Contributor

This commit introduces a no-std build for rollup-interface. This is required to create WASM modules for module implementations, having one use-case the web wallet.

We follow a similar approach to borsh and create a maybestd module so dependencies can rely on that for fallback implementations of common stdlib components that are available either on alloc or commonly used crates.

Most of the requirements exists on alloc, but we do have a couple of exceptions. The first is HashMap, that we fallback for hashbrown when std isn't available. We shouldn't expect issues from risc0 integration as risc0 code will be compiled with std.

The second is Mutex; for that, we fallback to spin, another common choice on the Rust ecosystem.

Finally, this commit introduces a couple of changes on the root Cargo.toml to not use default-features for crates that offers no-std support.

This commit introduces a `no-std` build for `rollup-interface`. This is
required to create WASM modules for module implementations, having one
use-case the web wallet.

We follow a similar approach to `borsh` and create a `maybestd` module
so dependencies can rely on that for fallback implementations of common
stdlib components that are available either on `alloc` or commonly used
crates.

Most of the requirements exists on `alloc`, but we do have a couple of
exceptions. The first is `HashMap`, that we fallback for `hashbrown`
when `std` isn't available. We shouldn't expect issues from risc0
integration as `risc0` code will be compiled with `std`.

The second is `Mutex`; for that, we fallback to `spin`, another common
choice on the Rust ecosystem.

Finally, this commit introduces a couple of changes on the root
`Cargo.toml` to not use default-features for crates that offers `no-std`
support.
@codecov
Copy link

codecov bot commented Oct 19, 2023

Codecov Report

Merging #1077 (c3dcf91) into nightly (e03ef9c) will increase coverage by 0.0%.
The diff coverage is 85.0%.

❗ Current head c3dcf91 differs from pull request most recent head fce8c53. Consider uploading reports for the commit fce8c53 to get more accurate results

Files Coverage Δ
rollup-interface/src/node/rpc/mod.rs 89.2% <ø> (ø)
rollup-interface/src/node/services/da.rs 0.0% <ø> (ø)
rollup-interface/src/state_machine/mocks/da.rs 75.7% <100.0%> (-5.4%) ⬇️
...ollup-interface/src/state_machine/mocks/service.rs 100.0% <100.0%> (ø)
...face/src/state_machine/mocks/validity_condition.rs 60.8% <ø> (ø)
rollup-interface/src/state_machine/mocks/zk_vm.rs 98.4% <100.0%> (ø)
rollup-interface/src/state_machine/stf.rs 85.7% <ø> (ø)
rollup-interface/src/state_machine/stf/fuzzing.rs 59.8% <ø> (ø)
rollup-interface/src/state_machine/zk/mod.rs 100.0% <ø> (ø)
rollup-interface/src/state_machine/da.rs 84.7% <33.3%> (ø)
... and 1 more

... and 1 file with indirect coverage changes

@vlopes11 vlopes11 enabled auto-merge October 19, 2023 22:29
@vlopes11 vlopes11 mentioned this pull request Oct 20, 2023
14 tasks
Copy link
Member

@neysofu neysofu left a comment

Choose a reason for hiding this comment

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

Looks great! I have some reservations mostly around three items:

  1. sov_db and sov_state_db. Why is no_std needed there?
  2. We need an effective way to test that our dependencies are no_std as well. Running cargo check in CI for sov-rollup-interface with a target that has no std support would be a good start. I see that there's a few std dependencies that slipped in, it'll be way too easy for that to happen again even if we fix it in this PR without adding a CI check.
  3. I would suggest we rely more on alloc for the types that it exposes, without maybestd. Namely, Arc, Vec, String, Box, etc..

full-node/db/sov-db/Cargo.toml Outdated Show resolved Hide resolved
rollup-interface/src/state_machine/mod.rs Show resolved Hide resolved
rollup-interface/src/state_machine/mocks/da.rs Outdated Show resolved Hide resolved
rollup-interface/src/state_machine/mocks/da.rs Outdated Show resolved Hide resolved
rollup-interface/src/state_machine/da.rs Show resolved Hide resolved
rollup-interface/src/lib.rs Outdated Show resolved Hide resolved
rollup-interface/src/lib.rs Outdated Show resolved Hide resolved
@vlopes11 vlopes11 requested review from bkolad and neysofu October 24, 2023 19:58
@vlopes11
Copy link
Contributor Author

@bkolad @neysofu review re-requested; sov-schema-db and sov-db removed in favor of a more minimal approach

Copy link
Member

@neysofu neysofu left a comment

Choose a reason for hiding this comment

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

sov-rollup-interface is not no_std-compatible yet because of its dependency tree, unfortunately. We'll also need some checks in CI for it. @vlopes11 how do you want to handle this? It doesn't feel right to merge no_std support without actual no_std support, but if that's what it takes to reduce merge conflicts I'm okay with it, assuming we'll have the bandwidth to work on these fixes ASAP.

full-node/db/sov-schema-db/src/db.rs Outdated Show resolved Hide resolved
rollup-interface/src/state_machine/da.rs Show resolved Hide resolved
@vlopes11 vlopes11 requested a review from neysofu October 25, 2023 10:29
Copy link
Member

@neysofu neysofu left a comment

Choose a reason for hiding this comment

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

As per our discussion on Slack, let's address the dependency tree problem (and CI) before merging

.github/workflows/rust.yml Outdated Show resolved Hide resolved
@vlopes11 vlopes11 added this pull request to the merge queue Oct 25, 2023
Merged via the queue into nightly with commit eb04e6f Oct 25, 2023
14 checks passed
@vlopes11 vlopes11 deleted the vlopes11/feature/no-std-rollup-interface branch October 25, 2023 17:53
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