cosmos / ibc-rs

Rust implementation of the Inter-Blockchain Communication (IBC) protocol.
Apache License 2.0
179 stars 73 forks source link

prefer `From`/`TryFrom` to `Into`/`TryInto` #1254

Open rnbguy opened 2 weeks ago

rnbguy commented 2 weeks ago

Feature Summary

Currently, we have some Into/TryInto trait bounds which can be rewritten in From/TryFrom.

https://github.com/cosmos/ibc-rs/blob/1ca03717ab074da605ace9ad75247dfa8fb03f86/ibc-clients/cw-context/src/context/client_ctx.rs#L21-L22

From is preferred over Into, as A: From<B> implies B: Into<A>, but not the other way around.

Proposal

- B: Into<A>
+ A: From<B>

- D: TryInto<C>
+ C: TryFrom<D>

Note that, Into in super traits may require Sized.

- trait Foo: Into<Bar> { }
+ trait Foo: Sized where Bar: From<Self> { }
seanchen1991 commented 2 weeks ago

In the code you linked, are you thinking of strengthening the constraints, i.e., re-writing the constraints that are currently written with Into to instead use From?

- <C::ClientState as TryFrom<Any>>::Error: Into<ClientError>,
+ <C::ClientState as TryFrom<Any>>::Error: From<ClientError>,
- <C::ConsensusState as TryFrom<Any>>::Error: From<ClientError>,
+ <C::ConsensusState as TryFrom<Any>>::Error: From<ClientError>,

If that's not what you mean, I'm not actually sure how to re-write these lines to use From, as we can't replace them with

ClientError: From<<C::ClientState as TryFrom<Any>>::Error>,
ClientError: From<<C::ConsensusState as TryFrom<Any>>::Error>,
rnbguy commented 2 weeks ago

are you thinking of strengthening the constraints, i.e., re-writing the constraints that are currently written with Into to instead use From?

I want to relax them. Rewriting the (Try)Into bounds with (Try)From.

ClientError: From<<C::ClientState as TryFrom>::Error>,

I believe this is alright - at least on latest Rust. (Check my comment at rollkit-ibc and the commit.) I guess, I have to check with the MSRV.