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.

Sign up for GitHub

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

Jump to bottom

The Wallet.is_mine() function should return K #1042

Open
Tracked by #74
, #76 ,  #80
...
notmandatory opened this issue Jul 20, 2023 · 10 comments · May be fixed by #1361
Open
Tracked by #74
, #76 ,  #80
...

The Wallet.is_mine() function should return K #1042

notmandatory opened this issue Jul 20, 2023 · 10 comments · May be fixed by #1361
Assignees
@vladimirfomene
Labels
api A breaking API change module-wallet
Milestone
1.0.0-beta

Comments

@notmandatory
Copy link
Member

notmandatory commented Jul 20, 2023 •
edited

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 notmandatory added enhancement New feature or request module-wallet labels Jul 20, 2023
@LLFourn
Copy link
Contributor

LLFourn commented Jul 20, 2023

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.

@notmandatory notmandatory added this to the 1.0.0-alpha.3 milestone Aug 16, 2023
@notmandatory
Copy link
Member Author

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

@nondiremanuel
Copy link

@notmandatory great, thanks for letting me know!

@nondiremanuel nondiremanuel mentioned this issue Aug 22, 2023
Closed
30 tasks
@nondiremanuel nondiremanuel mentioned this issue Sep 10, 2023
Closed
41 tasks
@nondiremanuel nondiremanuel mentioned this issue Sep 23, 2023
Closed
40 tasks
@realeinherjar
Copy link
Contributor

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>.

@notmandatory
Copy link
Member Author

notmandatory commented Sep 27, 2023 •
edited

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>

@LLFourn
Copy link
Contributor

LLFourn commented Sep 27, 2023

which_keychain_derived(&self, &Script)

@nondiremanuel nondiremanuel mentioned this issue Oct 6, 2023
Closed
39 tasks
@vladimirfomene
Copy link
Contributor

Let me take this up, @notmandatory.

@vladimirfomene vladimirfomene self-assigned this Oct 17, 2023
@nondiremanuel nondiremanuel mentioned this issue Oct 22, 2023
Closed
33 tasks
@nondiremanuel nondiremanuel mentioned this issue Nov 5, 2023
Closed
38 tasks
@vladimirfomene vladimirfomene mentioned this issue Nov 9, 2023
Closed
8 tasks
@notmandatory notmandatory modified the milestones: 1.0.0-alpha.3, 1.0.0-alpha.4 Nov 13, 2023
@nondiremanuel nondiremanuel mentioned this issue Nov 19, 2023
Closed
54 tasks
@nondiremanuel nondiremanuel mentioned this issue Dec 2, 2023
Closed
55 tasks
@nondiremanuel nondiremanuel modified the milestones: 1.0.0-alpha.4, 1.0.0 Jan 6, 2024
@LLFourn
Copy link
Contributor

LLFourn commented Jan 30, 2024

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

@evanlinjin
Copy link
Member

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 evanlinjin added the good first issue Good for newcomers label Jan 31, 2024
15IITian added a commit to 15IITian/bdk that referenced this issue Feb 25, 2024
@15IITian
docs: update docs to point to derivation_of_spk
8e72fa7 
updated docs to point `derivation_of_spk` function to find whether a `KeychainKind` exits  for the
given `script`

fix bitcoindevkit#1042
15IITian added a commit to 15IITian/bdk that referenced this issue Feb 25, 2024
@15IITian
docs: update docs to point to derivation_of_spk
b5304e3 
updated docs to point `derivation_of_spk` function to find whether a `KeychainKind` exits  for the
given `script`

fix bitcoindevkit#1042
@15IITian 15IITian linked a pull request Feb 25, 2024 that will close this issue
Open
2 tasks
@notmandatory notmandatory added documentation Improvements or additions to documentation api A breaking API change and removed enhancement New feature or request documentation Improvements or additions to documentation good first issue Good for newcomers labels Mar 16, 2024
@notmandatory
Copy link
Member Author

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

@nondiremanuel nondiremanuel modified the milestones: 1.0.0-alpha, 1.0.0-beta Mar 21, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees

@vladimirfomene vladimirfomene

Labels
api A breaking API change module-wallet
Projects
BDK
Status: Todo
Milestone
1.0.0-beta
6 participants
@LLFourn @notmandatory @vladimirfomene @evanlinjin @nondiremanuel @realeinherjar