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

Introduce bdk_core crate #1155

Closed
Tracked by #86 ...
evanlinjin opened this issue Oct 7, 2023 · 10 comments
Closed
Tracked by #86 ...

Introduce bdk_core crate #1155

evanlinjin opened this issue Oct 7, 2023 · 10 comments
Labels
discussion There's still a discussion ongoing module-blockchain new feature New feature or request

Comments

@evanlinjin
Copy link
Member

Describe the enhancement

This will contain common types shared across bdk crates.

Use case

Better crate structure. Allows chain-source crates to be non-dependent on bdk_chain.

Additional context

@evanlinjin evanlinjin added the new feature New feature or request label Oct 7, 2023
@evanlinjin evanlinjin added this to the 1.0.0-beta.0 milestone Oct 7, 2023
@evanlinjin evanlinjin added this to BDK Oct 7, 2023
@danielabrozzoni
Copy link
Member

Concept ACK, the name bdk_types might be more explicative

@LLFourn
Copy link
Contributor

LLFourn commented Oct 10, 2023

I think this crate will include some logic and not just types. E.g. the CheckPoint I think is the main thing that's caused the desire to have this.

I did suggest this but fwiw I am not entirely convinced it's a good idea. In retrospect I like the idea of just not depending on any bdk crate and just using impl Iterator<Item=(u32, BlockHash)> instead of CheckPoint. I'd see if we cannot exhaust all other ways of doing this before adding this crate.

@evanlinjin evanlinjin added the discussion There's still a discussion ongoing label Oct 11, 2023
@evanlinjin
Copy link
Member Author

Right now the RPC emitter must emit blocks contiguously (cannot skip blocks), because we are emitting (u32, Block) and we construct the chain update from the block header.

If the emitter emits (Checkpoint, Block) instead, we will have the flexibility to skip blocks.

I think the ability to skip blocks is useful for block-by-block chain sources (especially with CBF). Also, if we go along with #1079 and still want to ability to specify a start_height (instead of starting from genesis), we need this as well.

@evanlinjin
Copy link
Member Author

I did suggest this but fwiw I am not entirely convinced it's a good idea.

@LLFourn can you please expand on why it's not a good idea? Thanks

@LLFourn
Copy link
Contributor

LLFourn commented Oct 13, 2023

I don't know that it's not a good idea I just am not convinced it is compared to other possible directions. Happy to leave this call to you if you are totally convinced that we cannot get a good API without it.

@notmandatory
Copy link
Member

On the one hand I hate adding yet another crate to the project, but if these types will be more or less unchanging and will be needed for the CBF client then I support it. I also think it's easier to read and talk about code with specific types like (Checkpoint, Block) instead of dealing with all the places we have a u32 or u64 and having to think about is this a height or a timestamp or maybe some sort of index into transaction or block data, etc. Can we also put a type alias for u64 unix epoch timestamps here?

@danielabrozzoni
Copy link
Member

danielabrozzoni commented Oct 13, 2023

On the one hand I hate adding yet another crate to the project

Important to note, the more crates we add, the slower the release process becomes. Which isn't a big deal but it's something to consider.

Edit: mhh or maybe if they are all in the same workspace it's just a matter of bumping the version in the Cargo.toml, but you can just do cargo publish once? So maybe it's not that bad?

@notmandatory notmandatory moved this to Todo in BDK Oct 14, 2023
@notmandatory
Copy link
Member

notmandatory commented Oct 16, 2023

Edit: mhh or maybe if they are all in the same workspace it's just a matter of bumping the version in the Cargo.toml, but you can just do cargo publish once? So maybe it's not that bad?

I agree it's not a big deal, especially if it's not a crate that changes very much. If changed it's only a couple more steps to bump the version and publish. But keep in mind we'll have to publish each updated crate on it's own so we can do it in the right order, ie. "bdk_types", "bdk_chain", then the rest. At least that's what I've been doing for the alpha releases since I don't think cargo publish for the whole workspace can figure it out.

@notmandatory notmandatory modified the milestones: 1.0.0-beta.0, 1.0.0-alpha.4 Nov 13, 2023
@notmandatory
Copy link
Member

Since this is a functional change I moved it to the alpha.4 release.

@nondiremanuel nondiremanuel modified the milestones: 1.0.0-alpha.4, 1.0.0 Jan 6, 2024
@nondiremanuel nondiremanuel moved this from Todo to Discussion in BDK Jan 25, 2024
@evanlinjin evanlinjin modified the milestones: 1.0.0, 1.0.0-beta, 1.0.0-alpha Feb 5, 2024
@notmandatory notmandatory removed this from the 1.0.0-alpha milestone Mar 18, 2024
@evanlinjin
Copy link
Member Author

Done by #1569

@github-project-automation github-project-automation bot moved this from Discussion to Done in BDK Sep 4, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
discussion There's still a discussion ongoing module-blockchain new feature New feature or request
Projects
Status: Done
Development

No branches or pull requests

5 participants