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.

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

refactor(chain)!: Update KeychainTxOutIndex methods to use owned K and ScriptBuf #1506

Merged
merged 2 commits into from
Jul 22, 2024

Conversation

rustaceanrob
Copy link
Contributor

@rustaceanrob rustaceanrob commented Jul 8, 2024

Description

Make all method signatures of KeychainTxOutIndex take owned K and use ScriptBuf instead of its borrowed counterpart &Script. Fixes #1482

Notes to the reviewers

Steve also added a CI fix as well

Changelog notice

  • Make all method signatures of KeychainTxOutIndex take owned K
  • Update KeychainTxOutIndex methods to use ScriptBuf

Checklists

All Submissions:

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

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

@notmandatory notmandatory added the api A breaking API change label Jul 9, 2024
@notmandatory notmandatory added this to the 1.0.0-alpha milestone Jul 9, 2024
@notmandatory
Copy link
Member

notmandatory commented Jul 9, 2024

I pushed commit to also update KeychainTxOutIndex to use owned ScriptBufs instead of &Scripts.

@notmandatory
Copy link
Member

notmandatory commented Jul 9, 2024

⛏️ @rustaceanrob one small thing, can you change your commit description to be a little more descriptive, something like:

refactor(chain)!: update KeychainTxOutIndex methods to use owned K

@rustaceanrob rustaceanrob changed the title refactor(chain)!: standardize API ownership of K Standardize API ownership in KeychainTxOutIndex Jul 10, 2024
@notmandatory
Copy link
Member

I added a commit to limit tests to 2 threads, this is what I've always had to do locally to fix an issue with bitcoind based tests making calls to quickly.

@storopoli
Copy link
Contributor

I added a commit to limit tests to 2 threads, this is what I've always had to do locally to fix an issue with bitcoind based tests making calls to quickly.

Oh the nasty -- --test-threads=2 strikes back.
Yeah I had tons of issues with that in the past while prepping the Nix CI PR.

Copy link
Member

@notmandatory notmandatory left a comment

Choose a reason for hiding this comment

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

ACK ade5999

We should hold off on merging this until after PR for #1103 is merged so we don't disrupt that work. But may also require a re-base for this PR.

Copy link
Contributor

@ValuedMammal ValuedMammal left a comment

Choose a reason for hiding this comment

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

ACK ade5999

@notmandatory
Copy link
Member

This needs to be rebased after #1514 is merged.

@notmandatory notmandatory force-pushed the owned-k branch 2 times, most recently from e239dcf to 515f252 Compare July 22, 2024 00:36
@notmandatory
Copy link
Member

I pushed a test rebase, will do final rebase after #1514 is merged.

@notmandatory notmandatory merged commit 5478bb1 into bitcoindevkit:master Jul 22, 2024
12 checks passed
@notmandatory notmandatory changed the title Standardize API ownership in KeychainTxOutIndex refactor(chain)!: Update KeychainTxOutIndex methods to use owned K and ScriptBuf Jul 22, 2024
@notmandatory notmandatory changed the title refactor(chain)!: Update KeychainTxOutIndex methods to use owned K and ScriptBuf refactor(chain)!: Update KeychainTxOutIndex methods to use owned K and ScriptBuf Jul 22, 2024
@notmandatory notmandatory mentioned this pull request Jul 22, 2024
30 tasks
@rustaceanrob rustaceanrob deleted the owned-k branch July 22, 2024 17:16
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.

KeychainTxOutIndex API inconsistent with respect to references (should be owned)
4 participants