Skip to content

Conversation

@tankyleo
Copy link
Contributor

No description provided.

@ldk-reviews-bot
Copy link

ldk-reviews-bot commented Nov 17, 2025

👋 Thanks for assigning @benthecarman as a reviewer!
I'll wait for their review and will help manage the review process.
Once they submit their review, I'll check if a second reviewer would be helpful.

@tankyleo tankyleo force-pushed the esplora-config branch 3 times, most recently from e107189 to 693beec Compare November 17, 2025 17:34
@tankyleo tankyleo mentioned this pull request Nov 17, 2025
#[derive(Debug)]
pub enum ChainSource {
Rpc { rpc_address: SocketAddr, rpc_user: String, rpc_password: String },
Esplora { server_url: SocketAddr },
Copy link
Collaborator

Choose a reason for hiding this comment

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

I don't think socketaddr works with urls which is what most people will be using with this.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Right it does not, initially had String, went with SocketAddr for consistency with RPC, now switched back to String

rpc_password = "bitcoind-testpassword"
[esplora]
server_url = "127.0.0.1:3000"
Copy link
Collaborator

Choose a reason for hiding this comment

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

would be better to make this and all the other hard coded values to something like mempool space or blockstream, will make this easier for people to understand at a quick glance

);
},
ChainSource::Esplora { server_url } => {
builder.set_chain_source_esplora(format!("http://{}", server_url), None);
Copy link
Collaborator

Choose a reason for hiding this comment

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

we should make them include the http vs https

Copy link
Collaborator

@benthecarman benthecarman left a comment

Choose a reason for hiding this comment

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

otherwise lgtm

SocketAddr::from_str(&toml_config.bitcoind.rpc_address).map_err(|e| {
io::Error::new(
let chain_source = match (toml_config.esplora, toml_config.bitcoind) {
(Some(EsploraConfig { server_url }), _) => ChainSource::Esplora { server_url },
Copy link
Collaborator

Choose a reason for hiding this comment

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

imo it'd be better to throw an error if both are set. Setting both options is likely an error and could lead to them using a config they dont expect

dir_path = "/tmp/ldk-server/" # Path for LDK and BDK data persistence


# Must set either the bitcoind or the esplora table; the esplora table is prioritized over the bitcoind table
Copy link
Collaborator

Choose a reason for hiding this comment

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

would need to update this comment too

@tankyleo
Copy link
Contributor Author

otherwise lgtm

sorry pushed some more work and missed your review addressing now

@benthecarman benthecarman merged commit 4bf8685 into lightningdevkit:main Nov 20, 2025
7 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants