bdk icon indicating copy to clipboard operation
bdk copied to clipboard

Updated is_mine documentation

Open matthiasdebernardini opened this issue 1 year ago • 4 comments

Description

To clarify the purpose of a method and to indicate how to get more information from the wallet

Notes to the reviewers

We can also change the how is_mine works by calling derivation_of_spk directly, so its more clear how the internals work. This way rather than having the docs explicitly call out the method, they can jump to source.

Checklists

All Submissions:

  • [ x] I've signed all my commits
  • [ x] I followed the contribution guidelines
  • [ x] I ran cargo fmt and cargo clippy before committing

matthiasdebernardini avatar Feb 05 '24 16:02 matthiasdebernardini

Sure I support reimplementing in terms of derivation_of_spk. Also there might be a better name for it.

LLFourn avatar Feb 05 '24 23:02 LLFourn

    /// Finds how the wallet derived the script pubkey `spk`.
    ///
    /// Will only return `Some(_)` if the wallet has given out the spk.
    pub fn derivation_of_spk(&self, spk: &Script) -> Option<(KeychainKind, u32)> {
        self.indexed_graph.index.index_of_spk(spk)
    }

Also there might be a better name for it

@LLFourn rename this? Could be find_spk_derivation or find_spk?

Also with regards to;

Will only return Some(_) if the wallet has given out the spk

I think this means to say that it will return Some(_) if the spk is findable/derivable, to me, "has given out" means its tracking which spks its been giving out which sounds inaccurate.

matthiasdebernardini avatar Feb 06 '24 01:02 matthiasdebernardini

    /// Finds how the wallet derived the script pubkey `spk`.
    ///
    /// Will only return `Some(_)` if the wallet has given out the spk.
    pub fn derivation_of_spk(&self, spk: &Script) -> Option<(KeychainKind, u32)> {
        self.indexed_graph.index.index_of_spk(spk)
    }

Also there might be a better name for it

@LLFourn rename this? Could be find_spk_derivation or find_spk?

I don't like find because it implies it's not just doing a lookup. find in rust is used in iterators to go through each element and return the first match.

Also with regards to;

Will only return Some(_) if the wallet has given out the spk

I think this means to say that it will return Some(_) if the spk is findable/derivable, to me, "has given out" means its tracking which spks its been giving out which sounds inaccurate.

I think it was accurate. That's one of the main functions of the KeychainTxOutIndex is to track which has given out. However I think this will now return results for things that have not yet been given out because of the "lookahead" feature so I do think the docs needs to be updated to say that if it matches a script pubkey that is has derived internally.

LLFourn avatar Feb 06 '24 02:02 LLFourn

Since this is just a docs and internal implementation change that doesn't change the API I'd like to move it to a post 1.0 milestone.

notmandatory avatar Mar 20 '24 02:03 notmandatory

I closed this but would like to revisit this post 1.0 release.

matthiasdebernardini avatar May 24 '24 16:05 matthiasdebernardini