-
-
Notifications
You must be signed in to change notification settings - Fork 36
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
Add electrum support #131
Add electrum support #131
Conversation
it works good, thanks |
I tried compile for wasm target and I receive some errors:
First Error : AnyResolver
Regards the blocking http calls, both options in AnyResolver does not working in We can work with a "Dummy" option that would be great for doing some tests without accessing external data, for example: #[derive(From)]
#[non_exhaustive]
pub enum AnyResolver<T>
where
T: ResolveWitness + ResolveHeight + std::fmt::Debug,
{
#[cfg(feature = "electrum")]
#[from]
/// Electrum resolver
Electrum(Box<electrum::Resolver>),
#[cfg(feature = "esplora_blocking")]
#[from]
/// Esplora resolver
Esplora(Box<esplora_blocking::Resolver>),
/// Dummy resolver
Dummy(T),
} Second Error : cli as default-member
If we remove "cli" from default-members, in Cargo.toml, the compilation works. default-members = [
"psbt",
"fs",
"."
] |
@crisdut Notice that you conditionally compile it without either esplora or electrum feature, as you have, the match arms would be empty: Perhaps it would make sense to just mark it as non-exhaustive: |
Yeah, that's why I suggested making Also, this is the reason I was against this approach: #126 (review) I do not remember all the reasons, but when we tried it in |
By running the command that @crisdut is giving (
By adding the
so maybe to build on wasm we need to patch some dependencies differently? Anyway, when implementing the #[cfg(feature = "electrum")]
#[cfg(feature = "esplora_blocking")]
mod any;
#[cfg(feature = "electrum")]
#[cfg(feature = "esplora_blocking")]
pub use any::AnyResolver; |
I think I figured out what was keeping this from compiling to WASM. See #132 |
@@ -66,13 +68,15 @@ name = "rgb_rt" | |||
[dependencies] | |||
amplify = { workspace = true } | |||
baid58 = { workspace = true } | |||
bitcoin = { workspace = true, optional = true } | |||
commit_verify = { workspace = true } | |||
strict_types = { workspace = true } | |||
bp-core = { workspace = true } | |||
bp-std = { workspace = true } | |||
bp-wallet = { workspace = true, features = ["fs"] } |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hi @zoedberg,
Please set default-features=false
, to avoid load bp-esplora blocking client along wasm compilation.
bp-wallet = { workspace = true, features = ["fs"] } | |
bp-wallet = { workspace = true, default-features = false, features = ["fs"] } |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hi @crisdut, sorry for the late answer but I've somehow missed this. Thanks for the tip, unfortunately though I'm still receiving the previously reported errors.
Nice, so it seems #132, which is using my suggested fix, has fixed the wasm compilation (I've tried the branch and I can confirm it). So @dr-orlovsky, can we merge that PR into this and then this PR into master? |
fd639e4
to
ac5c2b9
Compare
These are electrum-specific commits extracted from
v0.11
branch #103, including the commit from #126 PR, which I move here to be able to run CI on the whole thing.We need to make sure this compiles to WASM before merging - and I expect it should due to BP-WG/bp-wallet@9d1cf14, making #127 and #128 outdated.
I will wait for @crisdut and @zoedberg confirmation before merging this and close the other two PRs trying to fix default features of
bp-wallet