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 compatibility: handling the 4 incompatible crates #951

Closed
3 of 4 tasks
livelybug opened this issue May 17, 2021 · 9 comments
Closed
3 of 4 tasks

no_std compatibility: handling the 4 incompatible crates #951

livelybug opened this issue May 17, 2021 · 9 comments
Assignees
Labels
E: no-std External: support for no_std compatibility
Milestone

Comments

@livelybug
Copy link

livelybug commented May 17, 2021

@romac In ibc-rs module and dependencies.tendermint module, there are 4 crates that's incompatible with no-std, as below:

If you have any progress of processing the crates above for no-std please update here, thank you.

@romac
Copy link
Member

romac commented May 17, 2021

anomaly

Yes, we need to remove it.

thiserror

Replace with displaydoc and a conditional std::error:Error instance.
See dtolnay/thiserror#64 (comment)

serde-repr

It looks like this crate is not used at all in tendermint, and we can thus just remove it from the dependencies.

toml

This crate is only used to parse a Tendermint configuration file. I would suggest only defining this function when no_std is not set, and thus only enable the toml dependency when the std feature is enabled.

@livelybug Does this make sense to you?

@romac romac changed the title ibc-rs no-std switch: handling the 4 no-std imcompatible crates no-std compatibility: handling the 4 no_std incompatible crates May 17, 2021
@romac romac added the E: no-std External: support for no_std compatibility label May 17, 2021
@romac romac changed the title no-std compatibility: handling the 4 no_std incompatible crates no_std compatibility: handling the 4 incompatible crates May 17, 2021
@romac
Copy link
Member

romac commented May 17, 2021

Note that we often use the .context method of anomaly to provide more context when throwing an error. Each use of .context should therefore be replace with a specific error variant:

Before:

use thiserror::Error;

type Error = anomaly::Error<Kind>;

#[derive(Clone, Debug, Error, PartialEq, Eq)]
enum Kind {
   #[error("foo error")]
    Foo,
}

fn some_func() -> Result<(), Error> {
  if true {
    Err(Kind::Foo.context(format!("bar: {}", n)).into())
  } else {
    Err(Kind::Foo.context("baz").into())
  }
}

After

use displaydoc::Display;

#[derive(Clone, Debug, Display, ...)]
enum Foo {
  /// bar: {0}
  Baz(i32),
  /// baz
  Baz
}

#[cfg(feature = "std")]
impl std::error::Error for Foo {}

#[derive(Clone, Debug, Display, ...)]
enum Error {
   /// Foo error: {0}
    Foo(Foo),
}

#[cfg(feature = "std")]
impl std::error::Error for Error {}

fn some_func(n: i32) -> Result<(), Error> {
  if true {
    Err(Kind::Foo(Foo::Bar(n)))
  } else {
    Err(Kind::Foo(Foo::Baz))
  }
}

@romac romac mentioned this issue May 17, 2021
5 tasks
@soareschen
Copy link
Contributor

I'm not sure if the proposed composition of Foo and Bar errors handle the stack traces well? Actually, with this the backtrace is not captured at all. Or perhaps backtraces are not that useful in practice? (I don't really need it personally, just thought some developers may find the backtraces useful)

@romac
Copy link
Member

romac commented May 17, 2021

That's a good point, thanks! I will investigate to see if we can somehow retain backtraces when std is enabled. Backtraces are indeed useful imho but if we retain them at the relayer and relayer-cli it might be enough for our needs, cf. #950 (comment).

@DaviRain-Su
Copy link
Contributor

anomaly

Yes, we need to remove it.

thiserror

Replace with displaydoc and a conditional std::error:Error instance.
See dtolnay/thiserror#64 (comment)

serde-repr

It looks like this crate is not used at all in tendermint, and we can thus just remove it from the dependencies.

toml

This crate is only used to parse a Tendermint configuration file. I would suggest only defining this function when no_std is not set, and thus only enable the toml dependency when the std feature is enabled.

@livelybug Does this make sense to you?

hi, For anomaly crate, have you some advise? I am try to use anyhow to re-code.

@DaviRain-Su
Copy link
Contributor

anomaly

Yes, we need to remove it.

thiserror

Replace with displaydoc and a conditional std::error:Error instance.
See dtolnay/thiserror#64 (comment)

serde-repr

It looks like this crate is not used at all in tendermint, and we can thus just remove it from the dependencies.

toml

This crate is only used to parse a Tendermint configuration file. I would suggest only defining this function when no_std is not set, and thus only enable the toml dependency when the std feature is enabled.

@livelybug Does this make sense to you?

I have re-code, and meet some error, for anomaly. I see your use eyre move to anomaly, I have been do this. But, in no_std env, eyre and displaydoc (support no_std thiserror replace displaydoc). Have been some error. So I use anyhow and displaydoc to do this, But I have other error. informalsystems/tendermint-rs#884

@romac
Copy link
Member

romac commented May 20, 2021

For anomaly crate, have you some advise? I am try to use anyhow to re-code.

I was thinking we would not use any error handling library at all, just displaydoc as mentioned above.

@soareschen What do you think we should do there? Do you see any value on using anyhow in the ibc crate?

@DaviRain-Su
Copy link
Contributor

I am re-code by anomaly move to eyre and pare with displaydoc to handle with this, I meet some error, then, use anyhow and display have no some problem, but meet time mod have some error in no_std env.

@adizere adizere added this to the 09.2021 milestone Aug 3, 2021
@adizere adizere modified the milestones: 09.2021, 10.2021 Sep 27, 2021
@adizere adizere modified the milestones: 10.2021, 01.2022 Nov 2, 2021
@soareschen
Copy link
Contributor

All the problematic dependencies have been removed. So closing this one.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
E: no-std External: support for no_std compatibility
Projects
None yet
Development

No branches or pull requests

5 participants