Skip to content

Conversation

@yukibtc
Copy link
Contributor

@yukibtc yukibtc commented Sep 29, 2025

Description

Replace ScriptBuf with &Script in SpkTxOutIndex::index_of_spk and KeychainTxOutIndex::index_of_spk methods, to avoid the need of cloning the ScriptBuf for a SPK index lookup.

Changelog notice

Breaking changes:

  • Change SpkTxOutIndex::index_of_spk and KeychainTxOutIndex::index_of_spk args from ScriptBuf to &Script.

Checklists

All Submissions:

New Features:

  • I've added tests for the new feature
  • I've added docs for the new feature

Bugfixes:

  • This pull request breaks the existing API
  • I've added tests to reproduce the issue which are now passing
  • I'm linking the issue being fixed by this PR

@ValuedMammal ValuedMammal added this to the Chain 0.24.0 milestone Sep 29, 2025
@ValuedMammal ValuedMammal moved this to Needs Review in BDK Chain Sep 29, 2025
@ValuedMammal ValuedMammal added the api A breaking API change label Sep 29, 2025
@ValuedMammal
Copy link
Collaborator

Thank you @yukibtc.

@evanlinjin
Copy link
Member

Concept ACK.

For ergonomics, what do you think about having spk: impl AsRef<Script>? This way the caller can use ScriptBuf, &Script, Box<Script>, etc.

For methods that take ownership of the script, maybe we should change to spk: impl Into<ScriptBuf> - however, this will increase the scope of the PR - should be okay since we are relaxing bounds. Let me know what you think.

…x methods

Replace `ScriptBuf` with `AsRef<Script>` in `SpkTxOutIndex::index_of_spk` and `KeychainTxOutIndex::index_of_spk` methods, to avoid the need of cloning the `ScriptBuf` for a SPK index lookup.

Signed-off-by: Yuki Kishimoto <[email protected]>
@yukibtc
Copy link
Contributor Author

yukibtc commented Sep 30, 2025

For ergonomics, what do you think about having spk: impl AsRef<Script>? This way the caller can use ScriptBuf, &Script, Box<Script>, etc.

Agree, I've updated to take AsRef<Script> instead.

For methods that take ownership of the script, maybe we should change to spk: impl Into - however, this will increase the scope of the PR - should be okay since we are relaxing bounds. Let me know what you think.

Yeah, maybe is better to do this in a dedicated PR

@yukibtc
Copy link
Contributor Author

yukibtc commented Sep 30, 2025

Seems that CI failed for a electrsd timeout

Copy link
Contributor

@aagbotemi aagbotemi left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Code tested and result look okay.

Copy link
Contributor

@oleonardolima oleonardolima left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

cACK c095145

Copy link
Member

@evanlinjin evanlinjin left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

ACK c095145

@evanlinjin evanlinjin merged commit 00b5f02 into bitcoindevkit:master Oct 2, 2025
31 of 34 checks passed
@github-project-automation github-project-automation bot moved this from Needs Review to Done in BDK Chain Oct 2, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

api A breaking API change

Projects

Status: Done

Development

Successfully merging this pull request may close these issues.

5 participants