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

enable tendermint no_std and error handle by flex-error #890

Closed
wants to merge 60 commits into from
Closed

enable tendermint no_std and error handle by flex-error #890

wants to merge 60 commits into from

Conversation

DaviRain-Su
Copy link

#824 ,Support to Tendermint no_std features.
For error handle use anyhow, flex-eror and use no-std-compat to handle no_std features primitives.

flex-error crates: https://github.com/informalsystems/flex-error/
An experimental PR for ibc-rs is available at informalsystems/ibc-rs informalsystems/hermes#988.

This is a follow-up to support tendermint no_std features #889.

  • Referenced an issue explaining the need for the change
  • Updated all relevant documentation in docs
  • Updated all code comments where relevant
  • Wrote tests
  • Updated CHANGELOG.md

Copy link
Contributor

@soareschen soareschen left a comment

Choose a reason for hiding this comment

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

I went through the changes a little and given some suggestions on how you can better define the errors.

proto/Cargo.toml Outdated
@@ -20,7 +20,8 @@ all-features = true
prost = "0.7"
prost-types = "0.7"
bytes = "1.0"
anomaly = "0.2"
anyhow = {version = "1.0", default-features = false, git = "https://github.com/DaviRain-Su/anyhow.git", branch = "open-stderror"}
Copy link
Contributor

Choose a reason for hiding this comment

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

The version of anyhow here gives a compile error:

error[E0433]: failed to resolve: use of undeclared crate or module `anyhow`
    |
272 | pub type BoxError = alloc::Box<dyn anyhow::StdError + Send + Sync>;
    |                                    ^^^^^^ use of undeclared crate or module `anyhow`

error: aborting due to previous error

Copy link
Author

Choose a reason for hiding this comment

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

I have reset this change.

proto/src/lib.rs Outdated
pub use alloc::boxed::Box;

#[cfg(feature = "std")]
pub use std::boxed::Box;
Copy link
Contributor

Choose a reason for hiding this comment

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

I think we can probably don't use feature flags here and use alloc crate directly. When compiled with std it should automatically link with the one provided by std.

Copy link
Author

Choose a reason for hiding this comment

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

I have reset this change.

#[error("protocol error")]
Protocol,
Parse
[DisplayError<Error>]
Copy link
Contributor

Choose a reason for hiding this comment

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

We only need to wrap upstream errors if they are coming from code not belong to the us. If you just want to attach additional details in string, you can instead define suberrors such as Parse { reason: String } and omit the error source.

Better yet, we should split out the different suberrors such as:

ParseUrl
  { url: String }
  [ DisplayError<url::ParseError> ]
  | e | { format_args!("error parsing url {}", e.url) }
InvalidHashLength
  { length: usize }
  | e | { format_args!("hash has invalid length {}", e.length) }

|_| { format_args!("invalid hash: expected hash size to be 32 bytes") },

NoTimestamp
[DisplayError<Error>]
Copy link
Contributor

Choose a reason for hiding this comment

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

There is no need for any error source here, since there is no additional information needed.

Copy link
Author

Choose a reason for hiding this comment

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

I have reset this change.

@@ -65,7 +68,7 @@ impl TryFrom<RawVote> for Vote {

fn try_from(value: RawVote) -> Result<Self, Self::Error> {
if value.timestamp.is_none() {
return Err(NoTimestamp.into());
return Err(error::no_timestamp_error(anyhow::anyhow!("no timestamp error")));
Copy link
Contributor

Choose a reason for hiding this comment

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

This can be simplified into just

return Err(error::no_timestamp_error());

Behind the scene flex-error automatically calls anyhow to capture the stack trace already.

Copy link
Author

Choose a reason for hiding this comment

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

I have reset this change.

@DaviRain-Su
Copy link
Author

@soareschen Have modified all the problems you mentioned.

.timestamp
.ok_or(error::no_timestamp_error())?
.try_into()
.map_err(|e: std::convert::Infallible| {
Copy link
Contributor

Choose a reason for hiding this comment

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

This is still not fixed. The Infallible variant can also be removed.

.ok_or(error::no_timestamp_error())?
.try_into()
.map_err(|e | {
error::in_fallible_error(e)
Copy link
Contributor

Choose a reason for hiding this comment

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

See the documentation for Infallible here: https://doc.rust-lang.org/std/convert/enum.Infallible.html

Basically Infallible is an empty enum, which means there is no way you can ever construct a valid value for Infallible. So if we have a function that is given an Infallible value, we know that can never happen, so that part of the code will never actually get executed. As a result, we can discard an infallible value e: Infallible using match e {}.

Strictly speaking, we should never need to use Infallible in this case. Because a TryFrom with Infallible error is just equivalent to Try. So the type Timestamp in this case should just implement Try instead of TryFrom:

--- a/tendermint/src/time.rs
+++ b/tendermint/src/time.rs
- impl TryFrom<Timestamp> for Time {
-     type Error = Infallible;
- 
-     fn try_from(value: Timestamp) -> Result<Self, Self::Error> {
-         // prost_types::Timestamp has a SystemTime converter but
-         // tendermint_proto::Timestamp can be JSON-encoded
-         let prost_value = prost_types::Timestamp {
-             seconds: value.seconds,
-             nanos: value.nanos,
-         };
- 
-         Ok(SystemTime::from(prost_value).into())
-     }
- }
+ impl From<Timestamp> for Time {
+     fn from(value: Timestamp) -> Self {
+         // prost_types::Timestamp has a SystemTime converter but
+         // tendermint_proto::Timestamp can be JSON-encoded
+         let prost_value = prost_types::Timestamp {
+             seconds: value.seconds,
+             nanos: value.nanos,
+         };
+ 
+         SystemTime::from(prost_value).into()
+     }
+ }

Then here we can just rewrite it as

timestamp: value
    .timestamp
    .ok_or(error::no_timestamp_error())?
    .into()

See the commit informalsystems/hermes@6b397f1 for similar work done at informalsystems/hermes#988.

Copy link
Author

Choose a reason for hiding this comment

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

I understand thank you.

@xla xla dismissed their stale review June 11, 2021 08:36

Feedback addressed

@DaviRain-Su
Copy link
Author

I made some fixes.

@soareschen
Copy link
Contributor

@DaviRain-Su I notice that right now in master, the tendermint, tendermint-rpc, and tendermint-light-client-js crates are already compilable in wasm using the cargo build-wasm-tendermint and build-wasm-light-client commands. Does that means that the crates already work in WebAssembly and Substrate despite not yet supporting no_std?

@thanethomson do you have any context on how the Wasm builds of Tendermint works?

@DaviRain-Su
Copy link
Author

@DaviRain-Su I notice that right now in master, the tendermint, tendermint-rpc, and tendermint-light-client-js crates are already compilable in wasm using the cargo build-wasm-tendermint and build-wasm-light-client commands. Does that means that the crates already work in WebAssembly and Substrate despite not yet supporting no_std?

@thanethomson do you have any context on how the Wasm builds of Tendermint works?

I think it might not be supported, I found that some of the dependency crates don't have default-feature = false turned on.

@DaviRain-Su
Copy link
Author

@soareschen When I use cargo check -p tendermint --target wasm32-unknown-unknown to compile tendermint it gives the following error.
error: target is not supported, for more information see: https://docs.rs/getrandom/#unsupported-targets --> /Users/davirain/.cargo/registry/src/rsproxy.cn-8f6827c7555bfaf8/getrandom-0.2.2/src/lib.rs:213:9 | 213 | / compile_error!("target is not supported, for more information see: \ 214 | | https://docs.rs/getrandom/#unsupported-targets"); | |_________________________________________________________________________^

@soareschen
Copy link
Contributor

@DaviRain-Su did you tried building with the current branch or on master? I tried checking and building tendermint with the wasm32-unknown-unknown target on master and it works.

@DaviRain-Su
Copy link
Author

@DaviRain-Su did you tried building with the current branch or on master? I tried checking and building tendermint with the wasm32-unknown-unknown target on master and it works.

Yes, the current branch and the master branch are the same, and also encountered the same problem as in compiling ibc-proto.

@soareschen
Copy link
Contributor

Closing this as the related changes have been merged through other PRs or addressed in other issues.

@soareschen soareschen closed this Sep 17, 2021
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.

5 participants