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

fix: Use StdError constructor functions #82

Merged
merged 1 commit into from
Jul 19, 2023

Conversation

apollo-sturdy
Copy link
Contributor

After discussing here: CosmWasm/cosmwasm#1760 (comment)
@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
Copy link

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
Copy link
Contributor Author

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:

  • my-contract
    • cosmwasm-std
    • osmosis-std
      • cosmwasm-std
      • osmosis-std-derive
        • cosmwasm-std

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
Copy link

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
Copy link
Contributor Author

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.

@iboss-ptk iboss-ptk merged commit 0fa82db into osmosis-labs:main Jul 19, 2023
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