osmosis-labs / osmosis-rust

Rust libraries for osmosis
Apache License 2.0
59 stars 52 forks source link

fix: Use StdError constructor functions #82

Closed apollo-sturdy closed 1 year ago

apollo-sturdy commented 1 year ago

After discussing here: https://github.com/CosmWasm/cosmwasm/issues/1760#issuecomment-1620192501 @webmaster128 mentioned that it makes more sense to use the constructor functions for StdError. This way we don't have to add the backtraces field and also don't need a backtraces feature for osmosis-std. I didn't yet remove the backtraces feature though since that would be a breaking change. We can do it in the next major version.

webmaster128 commented 1 year ago

Looks good

and also don't need a backtraces feature for osmosis-std.

Keeping backtraces = ["cosmwasm-std/backtraces", allows you to use the backtraces when needed. So there is value to keep it. The switching happens inside of the constructor functions you are now using.

apollo-sturdy commented 1 year ago

Looks good

and also don't need a backtraces feature for osmosis-std.

Keeping backtraces = ["cosmwasm-std/backtraces", allows you to use the backtraces when needed. So there is value to keep it. The switching happens inside of the constructor functions you are now using.

Since features are shared between dependencies it shouldn't be needed? E.g.:

Dependency tree:

When my-contract enables feature backtraces for its cosmwasm-std dependency, the cosmwasm-std dependency of osmosis-std and osmosis-std-derive will also get the feature, which is why osmosis-std doesn't need to have the feature flag itself.

I think this is how it works. Lmk if incorrect.

webmaster128 commented 1 year ago

This is true for the original resolver. But the described behaviour is probably different if you use resolver version 2. Then the cosmwasm-stds are compiled multiple times with different features. See https://doc.rust-lang.org/edition-guide/rust-2021/default-cargo-resolver.html.

apollo-sturdy commented 1 year ago

This is true for the original resolver. But the described behaviour is probably different if you use resolver version 2. Then the cosmwasm-stds are compiled multiple times with different features. See https://doc.rust-lang.org/edition-guide/rust-2021/default-cargo-resolver.html.

Oh good point. Ok in that case I agree makes sense to keep the feature here as well.