adds support for host functions to ics23 #90
Conversation
* adds support for host functions * cargo fmt * export HostFunctionsProvider
There was a problem hiding this comment.
Thanks for your PR. I agree with the general direction, but request some design changes.
Rather than keep the HostFunctionsManager only in test code (as dev-dependency), I would rather hide it under a feature flag. And ideally have that flag on by default.
For substrate, you can then use no-default-features to avoid this dependency and use the host functions. But it would be a lot less intrusive for other projects (like ibc-rs) that also run in the Hermes relayer and want such functions as well. They should not have to implement this themselves.
| use sha2::{Digest, Sha512, Sha512Trunc256}; | ||
| use sha3::Sha3_512; | ||
|
|
||
| pub struct HostFunctionsManager; |
There was a problem hiding this comment.
In general, it makes sense to move this.
However, substrate is not the only production user and this code will be needed outside of tests.
There was a problem hiding this comment.
Its available in std now
| anyhow = { version = "1.0.40", default-features = false } | ||
| sp-std = {version = "3.0.0", default-features = false } | ||
| sp-std = { git = "https://github.com/paritytech/substrate", branch = "polkadot-v0.9.22", default-features = false } |
There was a problem hiding this comment.
I really don't like tagging to branches here.
Can we just update to a newer release if that is what you need?
There was a problem hiding this comment.
unfortunately, this won't work. Substrate doesn't publish new releases to crates.io so parachain teams need to track git branches for new releases.
There was a problem hiding this comment.
why do they do that? do you know?
There was a problem hiding this comment.
from the versioning (currently we're at 0.9.24) I'd say they don't want to publish until they hit v1.0.0, which would be considered stable
There was a problem hiding this comment.
@ethanfrey will you be able to tag a version & publish on crates.io with the dependency specified as git/branch?
* adds support for host functions * cargo fmt * export HostFunctionsProvider * make HostFunctionsManager available in std
|
@hdevalence @adizere @romanc I am pretty sure you all use the std variant here. This change should be compatible, just allow more flexibility at the cost of requiring specifying a hash implementation when calling some functions. I will likely merge this shortly, but I would really like your feedback before tagging anything, we can still adjust the API to work for all |
| @@ -1,5 +1,16 @@ | |||
| # Changelog | |||
|
|
|||
| # 0.8.0 | |||
@ethanfrey I think you tagged me by mistake. I am not part of your project. Just letting you know because it sounded like you wanted feedback from someone else. |
This encapsulates all crypto hash functions behind a trait, so downstream crates can delegate these functions to host functions