bdk icon indicating copy to clipboard operation
bdk copied to clipboard

The Wallet.is_mine() function should return K

Open notmandatory opened this issue 2 years ago • 10 comments

Describe the enhancement

The Wallet.is_mine(&self, script: &[Script]) function currently returns a bool representing "whether or not a script is part of this wallet (either internal or external)". It would be more useful to know which KeychainKind the script was found for, or None if not one of my scripts:

pub fn is_mine(&self, script: &[Script]) -> Option<KeychainKind>

Use case

A request came up on discord about how to figure out which output is the change. Having is_mine() tell you which chain a script is for would make this easy to figure out.

notmandatory avatar Jul 20 '23 02:07 notmandatory

Note is_mine just takes a &Script. You probably want the keychain and dervation index. We have a method for that called derivation_of_spk so this can be fixed by just updating the docs to point to that function if you want it.

LLFourn avatar Jul 20 '23 03:07 LLFourn

@nondiremanuel we didn't discussed this one yesterday but I think it belongs in alpha.3.

notmandatory avatar Aug 16 '23 12:08 notmandatory

@notmandatory great, thanks for letting me know!

nondiremanuel avatar Aug 18 '23 13:08 nondiremanuel

As discussed in W39 Lib Team Call, I think that the methods prefixed with is_* should always return a bool for consistency sake. We should create a new Wallet method that returns Option<KeychainKind>.

realeinherjar avatar Sep 26 '23 12:09 realeinherjar

I agree we should add a new function. What should we call it? does something like this make sense? in_keychain(&self, &Script) -> Option<KeychainKind>

notmandatory avatar Sep 27 '23 00:09 notmandatory

which_keychain_derived(&self, &Script)

LLFourn avatar Sep 27 '23 06:09 LLFourn

Let me take this up, @notmandatory.

vladimirfomene avatar Oct 17 '23 06:10 vladimirfomene

Is there actually anything to do here. What's wrong with derivation_of_spk?

LLFourn avatar Jan 30 '24 23:01 LLFourn

Idiomatically, I think is_mine should return a boolean. I think we can update the docs (as @LLFourn suggested) to use the deriation_of_spk method of Wallet.

evanlinjin avatar Jan 31 '24 16:01 evanlinjin

A likely small change but I think we should push to 2.0 milestone.

notmandatory avatar Mar 20 '24 02:03 notmandatory