hmm not sure I get that..
something like this?

pub trait DescriptorTemplate {
    /// Build the complete descriptor
    fn build(self, network: Network) -> Result<DescriptorTemplateOut, DescriptorError>;
}

Bip44(prvkey, KeychainKind::External).build(Network::Bitcoin)

pub struct Bip44<K: DerivableKey<Legacy>>(pub K, pub KeychainKind);

impl<K: DerivableKey<Legacy>> DescriptorTemplate for Bip44<K> {
    fn build(self, network: Network) -> Result<DescriptorTemplateOut, DescriptorError> {
        P2Pkh(legacy::make_bipxx_private(44, self.0, self.1, network)?).build(network)
    }
}

Yes, exactly, something like that.

This will prevent you from having conflicts, i.e. specifying network x in the template constructor, but then trying to use it on a wallet for network y

i see what u mean.
we end up with a couple of unused network inputs
https://gist.github.com/jbesraa/1e807a730e45746371b2d1dbf65d47fd#file-bdk-desc-template-diff-rs-L26-L49
look for _network

Yeah I think that's fine, some templates will use the network and some won't

@rajarshimaitra what do u think about the solution mentioned in prev message(gist)?

edit:
https://gist.github.com/jbesraa/1e807a730e45746371b2d1dbf65d47fd#file-bdk-desc-template-diff-rs

I looked at the diff.. But couldn't apply it.. Probably its on top of a commit not in the tree anymore.. Anyway if the issue is only having to pass unused args I think thats a better next step.. So probably worth to put another commit with the changes..

But something feels off to me. IIUC the network information should be needed for for all kind of keys.. So ideally we should only have one source of it and disperse it evenly through all the macros..

Like how would this macro know which network to derive the key for??

pub struct P2Pkh<K: IntoDescriptorKey<Legacy>>(pub K);

impl<K: IntoDescriptorKey<Legacy>> DescriptorTemplate for P2Pkh<K> {
    fn build(self) -> Result<DescriptorTemplateOut, DescriptorError> {
        descriptor!(pkh(self.0))
    }
}

P2PKH mainnet and P2PKH testnet should have different coin types too..

So I think somewhere deeper a fix needs to be made so that we can disperse the network information evenly?? @afilini let me know if I am missing something here.

Like how would this macro know which network to derive the key for??

This is all handled by the descriptor!() macro, so you don't have to worry about it at this level.

If you look at the DescriptorTemplateOut type you'll see that it contains a ValidNetworks set. This is the intersection of all the valid network sets for all the keys. For example, if I have a key that's valid everywhere (let's say, a bip39 mnemonic) and a xprv the final valid network is the intersection of and { mainnet }, which is just mainnet.

If you then try to load that descriptor in a testnet wallet, you'll get an error.

P2PKH mainnet and P2PKH testnet should have different coin types too..

I think you are confusing the address encoding, which is different, with the script, which is the same. The base58 encoding uses different prefixes for mainnet/testnet, so you get a 1Something address in mainnet and a m/nsomething on testnet. But this encoding only happens when you want to get a address string for a given script (which is done inside the wallet that knows the network). Here you are just defining the script, and that's the same on both chains

If you look at the DescriptorTemplateOut type you'll see that it contains a ValidNetworks set.

Ok I see.. So the underlying macros already knows how to transmit back valid network set..

I think you are confusing the address encoding, which is different, with the script, which is the same

Yes I was confused here. The P2PKH template will already have all the valid network set it needs..

I guess then the only work remaining here is to make BipXXPublic also set the coin type flag with network?? And will only need the same modification in the make_bipxx_public macro function.. And then a test to seal the behavior for all the bips..

I don't think you can do it for the BipXXPublic structs because the coin type is part of the hardened derivation steps, so it has to be applied first. The -Public structs are meant to be used when you only know the pubkey.

We will have to update the documentation though, for example for bip44 it says:

This assumes that the key used has already been derived with m/44'/0'/0'

This should be updated and contain a note that on testnet the path should be X and on mainnet Y.

Hi, please rebase to pickup changes in #596. Thanks!

@jbesraa can you please simplify the commit history by doing git rebase master from your local branch?? The history has become confusing.. And don't know why git log is showing me different history than github..

In general anytime you wanna rebase your PR just do git rebase master and fix any conflict that occurs in rebase.. And not commit master into your branch.. That makes review much simpler..

+1 for the rebase, plus i would just squash everything into a single commit for the same reason Raj mentioned

I'm also starting the CI so we can see if something is wrong there

This needs rebase to pick up the CI changes :)

@jbesraa I'd love to see https://github.com/bitcoindevkit/bdk/pull/585/files#r857772454 fixed before merging :)

Ah damn, I can't merge this because the commits aren't signed. Could you please fix this @jbesraa ?

@jbesraa if you need any help setting up signing with github let me know or check out this link: https://docs.github.com/en/authentication/managing-commit-signature-verification/signing-commits

re-ACK db9d43e