Skip to content

refactor(chain)!: Create module indexer#1493

Merged
LLFourn merged 1 commit intobitcoindevkit:masterfrom
ValuedMammal:refactor/keychain-balance
Jul 6, 2024
Merged

refactor(chain)!: Create module indexer#1493
LLFourn merged 1 commit intobitcoindevkit:masterfrom
ValuedMammal:refactor/keychain-balance

Conversation

@ValuedMammal
Copy link
Copy Markdown
Collaborator

@ValuedMammal ValuedMammal commented Jun 27, 2024 •
edited
Loading

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
Copy Markdown
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
Copy Markdown
Collaborator Author

ValuedMammal commented Jun 28, 2024

@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
Copy Markdown

storopoli commented Jun 29, 2024

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
Closed
3 tasks
notmandatory
notmandatory reviewed Jul 2, 2024
View reviewed changes
Copy link
Copy Markdown
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
Copy Markdown
Collaborator Author

ValuedMammal commented Jul 3, 2024

Rebased onto a112b4d

notmandatory
notmandatory approved these changes Jul 3, 2024
View reviewed changes
Copy link
Copy Markdown
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
LLFourn reviewed Jul 4, 2024
View reviewed changes
@LLFourn
Copy link
Copy Markdown
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.

@ValuedMammal
ref(chain)!: create module indexer
c3fc1dd 
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
Copy Markdown
Collaborator Author

ValuedMammal commented Jul 5, 2024

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

notmandatory
notmandatory approved these changes Jul 5, 2024
View reviewed changes
Copy link
Copy Markdown
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
Copy Markdown
Member

notmandatory commented Jul 5, 2024

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

LLFourn
LLFourn approved these changes Jul 6, 2024
View reviewed changes
Copy link
Copy Markdown
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
Closed
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

Reviewers

@LLFourn LLFourn LLFourn approved these changes

@notmandatory notmandatory notmandatory approved these changes

Assignees

@ValuedMammal ValuedMammal

Labels

api A breaking API change

Projects

BDK Wallet
Archived in project

Milestone

1.0.0-alpha

Development

Successfully merging this pull request may close these issues.

[chain] keychain module doesn't need to exist

4 participants

@ValuedMammal @LLFourn @storopoli @notmandatory