Skip to content

Conversation

ValuedMammal
Copy link
Collaborator

@ValuedMammal ValuedMammal commented Jun 27, 2024

Introduce module indexer and replace old keychain module with balance.rs

fixes #1317
replaces #1175

Changelog notice

Removed keychain module from bdk_chain. The Balance type is now re-exported from the top level

Checklists

All Submissions:

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

@ValuedMammal ValuedMammal self-assigned this Jun 27, 2024
@ValuedMammal ValuedMammal added this to the 1.0.0-alpha milestone Jun 27, 2024
@LLFourn
Copy link
Collaborator

LLFourn commented Jun 28, 2024

I wonder what we think about:

indexer.rs // the trait in here
indexer/
  ├── keychain_txout.rs
  └── spk_txout.rs

@ValuedMammal
Copy link
Collaborator Author

@LLFourn Should indexer be a pub mod or do we just re export everything under it?

@ValuedMammal ValuedMammal force-pushed the refactor/keychain-balance branch 2 times, most recently from eae3ffe to 744cbf2 Compare June 29, 2024 15:02
@ValuedMammal ValuedMammal changed the title [chain] Create module keychain_txout_index [chain] Create module indexer Jun 29, 2024
@storopoli
Copy link
Contributor

I think re-export, despite pedantic, is more future-proof.

@notmandatory notmandatory added the api A breaking API change label Jul 1, 2024
@notmandatory notmandatory mentioned this pull request Jul 1, 2024
3 tasks
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.

Overall looks good, just one question.

@ValuedMammal ValuedMammal force-pushed the refactor/keychain-balance branch 2 times, most recently from c7092c6 to 884ac90 Compare July 3, 2024 14:51
@ValuedMammal
Copy link
Collaborator Author

Rebased onto a112b4d

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 884ac90

@LLFourn
Copy link
Collaborator

LLFourn commented Jul 4, 2024

@LLFourn Should indexer be a pub mod or do we just re export everything under it?

I think it must be pub mod otherwise you will be re-exporting the ChangeSet in the keychain_txout.rs file at the root! I would just make people import by indexer::keychain_txout::KeychainTxOutIndex.

I wouldn't mind dropping the TxOut from these which seems pretty redundant.

and replace keychain module with `balance.rs`
@ValuedMammal ValuedMammal force-pushed the refactor/keychain-balance branch from 884ac90 to c3fc1dd Compare July 5, 2024 16:24
@ValuedMammal
Copy link
Collaborator Author

  • Fixed file indexer.rs
  • Made indexer a public module

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.

reACK c3fc1dd

@notmandatory
Copy link
Member

I'd prefer to merge this as is rather than doing any more renaming at this point.

Copy link
Collaborator

@LLFourn LLFourn left a comment

Choose a reason for hiding this comment

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

ACK c3fc1dd

@LLFourn LLFourn merged commit db8fbd7 into bitcoindevkit:master Jul 6, 2024
@ValuedMammal ValuedMammal deleted the refactor/keychain-balance branch July 12, 2024 22:53
@notmandatory notmandatory mentioned this pull request Jul 20, 2024
30 tasks
@notmandatory notmandatory changed the title [chain] Create module indexer refactor(chain): Create module indexer Jul 20, 2024
@notmandatory notmandatory changed the title refactor(chain): Create module indexer refactor(chain)!: Create module indexer Jul 20, 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
Projects
Archived in project
Development

Successfully merging this pull request may close these issues.

[chain] keychain module doesn't need to exist
4 participants