Skip to content

Conversation

@evanlinjin
Copy link
Member

@evanlinjin evanlinjin commented May 10, 2024

Description

Adds various improvements to the work of #1203. These were missed out while cherry-picking, or review comments left in #1428 that were forgotten:

  • Change keychains_to_descriptors to keychains_to_descriptor_ids which simplifies the field. This was mentioned here and included in Further work on #1203 #1428, but an older commit was cherry-picked.
  • Rename KeychainTxOutIndex field descriptor_ids_to_keychain_set to descriptor_ids_to_keychains was missed out as an older commit was cherry-picked. This change to naming shows the direct relationship between keychains_to_desriptor_ids and descriptor_ids_to_keychains (one is directly a reverse lookup of the other).
  • Change reveal_to_target_with_id to reveal_to_target_with_descriptor, reasoning mentioned here.

In addition to this, I changed the output signature of reveal_to_target, reveal_next_spk and next_unused_spk methods to return (Option<spk(s)>, changeset), whereas previously it was Option<(spk(s), changeset)>. This makes the API more consistent as the ChangeSet is always returned, and reveal_to_target and unbounded_spk_iter-esc methods all return Option<SpkIterator> (which we can .flatten()).

Notes to the reviewers

Not all changes in this PR are Changelog-worthy. I.e. renaming of internal variables to increase readability, changing code comments, refactoring private methods - are all excluded from the changelog.

Changelog notice

  • Change KeychainTxOutIndex methods to always return a changeset. This makes the API more consistent.

Checklists

All Submissions:

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

Also rename `descriptor_ids_to_keychain_set` to
`descriptor_ids_to_keychains` and update it's documentation.
Change the out signature of methods `reveal_to_target`,
`reveal_next_spk` and `next_unused_spk` to return a tuple of
`(Option<spk(s)>, changeset)` instead of `Option<(spk(s), changeset)>`.
This makes the API more consistent.

Also refactored various helper methods to take in a descriptor instead
of a descriptor id. `.expect` calls now exist outside of these helper
methods, making it more obvious where they are being called.

Docs are updated to reflect the new API.
@evanlinjin evanlinjin force-pushed the issue/1011-touchups branch from 86f3028 to 78e3264 Compare May 15, 2024 12:44
@notmandatory notmandatory added this to the 1.0.0-alpha milestone May 15, 2024
@notmandatory notmandatory added module-blockchain api A breaking API change labels May 15, 2024
@evanlinjin evanlinjin marked this pull request as ready for review May 15, 2024 16:18
@ValuedMammal
Copy link
Collaborator

In terms of super simple names you could have descriptor_ids_to_descriptors be named descriptors and descriptor_ids_to_keychains be keychains. With a method like get, it's still clear what the key-value relationship is.

@ValuedMammal
Copy link
Collaborator

I got tripped up at this comment. I think what's happening is we're removing this keychain from the old descriptor's set

// we should remove old descriptor that is associated with this keychain as the index
// is designed to track one descriptor per keychain (however different keychains can
// share the same descriptor)
let _is_keychain_removed = self
.descriptor_ids_to_keychains
.get_mut(&old_desc_id)
.expect("we must have already inserted this descriptor")
.remove(&keychain);

@ValuedMammal
Copy link
Collaborator

Maybe more of a philosophical point, but I think an argument can be made that methods which previously returned K, such as index_of_spk et al, can return the descriptor id and let the caller match the id with a pre-determined label. If a user is interested in getting the set of keychains marking a descriptor, there can be a method for that called keychains_of_descriptor{_id}

@notmandatory
Copy link
Member

Can you also add changes from #1341?

@evanlinjin
Copy link
Member Author

Can you also add changes from #1341?

Good idea

@ValuedMammal
Copy link
Collaborator

Is it worth revisiting a discussion of returning Option vs empty iterators? I don't see an issue with returning bogus values (given the wrong inputs) if it means reducing the syntactic overhead in function signatures. We could expand this concept by returning ScriptBuf::new when we don't have an spk to reveal, and returning an empty SpkIterator (which we'd have to implement) for reveal_to_target.

@ValuedMammal
Copy link
Collaborator

Reviewed up through 78e3264. For a minute I thought all of #1341 was done but it looks like there's a few more places like spk_at_index, revealed_spks, unused_spks.

documentation nit: There's a missing backtick in the docs for keychain ChangeSet, which is strange because I think this was fixed in an older version.

@evanlinjin
Copy link
Member Author

@LagginTimes can you complete this PR for me?

@evanlinjin
Copy link
Member Author

Replaced by #1451

@evanlinjin evanlinjin closed this Jun 4, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

api A breaking API change module-blockchain

Projects

Archived in project

Development

Successfully merging this pull request may close these issues.

3 participants