[esplora] Support proxies in EsploraBlockchain

[afilini]

Description

Add support for http/socks proxies in Esplora

Notes to the reviewers

Opening this as a draft, since I think it's gonna break for wasm32.

This is also technically an API break, which according to the new updated guidelines shouldn't happen. On the other hand, I can't think of a better way to do this. Am I supposed to make a different EsploraBlockchainConfig struct with the new field added and only deprecate the current one?

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
• I've updated CHANGELOG.md
• This pull request breaks the existing API

[notmandatory]

On the question of how to make this sort of API change, since it's an additive change with an Option value I think it can be considered backward compatible. It does still technically break the API since anyone adopting the new version will have to do something. But it doesn't change the name or functionality of any existing struct fields or functions or function arguments. The alternative would be to make a whole new struct and new versions of all the functions that use it, which seems more disruptive when the original struct is eventually removed.

@thomaseizinger, as a user, what do you think about his sort of change? would it disrupt your workflow to have to add a new None value for this field after upgrading to a new version of bdk?

[thomaseizinger]

It is technically breaking yes.

I see two ways of changing the code so this change would be non breaking:

1. Put #[non_exhaustive] on such kind of structs which forces users to use ::default or ::new to make an instance.
2. Make all fields private and use a builder pattern to initialize such configurations:

EsploraBlockchainConfig::default().with_xyz().with_abc()

(1) is less code (no builders and getters) and would result in usage such as:

let config = EsploraBlockchainConfig{concurrency:Some(4),
..EsploraBlockchainConfig::default()};

Personally, I think (1) would be the better option unless you want to support Rust < 1.40.

For this particular PR, it is still a breaking change but we might at least take this opportunity to make future changes non-breaking :)

[RCasatta]

is less code (no builders and getters) and would result in usage such as:

 let config = EsploraBlockchainConfig {
concurrency: Some(4),
..EsploraBlockchainConfig::default()
};

It looks this doesn't work for users outside the bdk crate (while logically flawless)

[thomaseizinger]

is less code (no builders and getters) and would result in usage such as:

 let config = EsploraBlockchainConfig {
concurrency: Some(4),
..EsploraBlockchainConfig::default()
};

It looks this doesn't work for users outside the bdk crate (while logically flawless)

Damn, sorry about that. I swear I did this before with non-exhaustive structs. In guess it will have to be something like this then:

letmut config = ElectrumBlockchainConfig::new("foo".to_owned());
config.retry = 1;

[notmandatory]

What about the option:

3. implement the Default trait for structs that are part of our public API? We can then state in the docs that users who want to maintain forward compatibility with new fields must add ..Default::default() whenever using a struct expression for a struct that implements the Default trait.

If we go this route I'll add an issue to implement Default for all the bdk structs.

And maybe someday a future version of Rust will let us add the #[non_exhaustive] macro and have it work as we'd like for option 1.

[thomaseizinger]

What about the option:

1. implement the `Default` trait for structs that are part of our public API? We can then state in the docs that users who want to maintain forward compatibility with new fields must add `..Default::default()` whenever using a struct expression for a struct that implements the `Default` trait.

If we go this route I'll add an issue to implement Default for all the bdk structs.

And maybe someday a future version of Rust will let us add the #[non_exhaustive] macro and have it work as we'd like for option 1.

For EsploraBlockchainConfig though, there isn't really a sensible default I am afraid unless you want to default the URL to something?

In that case, providing a constructor that only takes the mandatory arguments (in this case url) requires rather little code and allows the use of #[non_exhaustive] as well. The only difference being that users need to make the struct mutable and override the defaults instead of being able to use struct initializer syntax.

[afilini]

We've talked about this yesterday during our team meeting and we reached the conclusion that it's better to just add the field and technically break the API: the reasoning is that we think it's better to force the user to use the explicit initialization syntax so that they are aware of all the existing fields of the struct.

Ideally we would also implement Default, but as you said we can't since we can't assume a default URL.

[notmandatory]

Needs a rebase and then I think this one is ready to review.

[notmandatory]

ACK 3fe2380

[afilini]

Description

Add support for http/socks proxies in Esplora

Notes to the reviewers

Opening this as a draft, since I think it's gonna break for wasm32.

This is also technically an API break, which according to the new updated guidelines shouldn't happen. On the other hand, I can't think of a better way to do this. Am I supposed to make a different EsploraBlockchainConfig struct with the new field added and only deprecate the current one?

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
• I've updated CHANGELOG.md
• This pull request breaks the existing API

[notmandatory]

On the question of how to make this sort of API change, since it's an additive change with an Option value I think it can be considered backward compatible. It does still technically break the API since anyone adopting the new version will have to do something. But it doesn't change the name or functionality of any existing struct fields or functions or function arguments. The alternative would be to make a whole new struct and new versions of all the functions that use it, which seems more disruptive when the original struct is eventually removed.

@thomaseizinger, as a user, what do you think about his sort of change? would it disrupt your workflow to have to add a new None value for this field after upgrading to a new version of bdk?

[thomaseizinger]

It is technically breaking yes.

I see two ways of changing the code so this change would be non breaking:

1. Put #[non_exhaustive] on such kind of structs which forces users to use ::default or ::new to make an instance.
2. Make all fields private and use a builder pattern to initialize such configurations:

EsploraBlockchainConfig::default().with_xyz().with_abc()

(1) is less code (no builders and getters) and would result in usage such as:

let config = EsploraBlockchainConfig{concurrency:Some(4),
..EsploraBlockchainConfig::default()};

Personally, I think (1) would be the better option unless you want to support Rust < 1.40.

For this particular PR, it is still a breaking change but we might at least take this opportunity to make future changes non-breaking :)

[RCasatta]

is less code (no builders and getters) and would result in usage such as:

 let config = EsploraBlockchainConfig {
concurrency: Some(4),
..EsploraBlockchainConfig::default()
};

It looks this doesn't work for users outside the bdk crate (while logically flawless)

[thomaseizinger]

is less code (no builders and getters) and would result in usage such as:

 let config = EsploraBlockchainConfig {
concurrency: Some(4),
..EsploraBlockchainConfig::default()
};

It looks this doesn't work for users outside the bdk crate (while logically flawless)

Damn, sorry about that. I swear I did this before with non-exhaustive structs. In guess it will have to be something like this then:

letmut config = ElectrumBlockchainConfig::new("foo".to_owned());
config.retry = 1;

[notmandatory]

What about the option:

3. implement the Default trait for structs that are part of our public API? We can then state in the docs that users who want to maintain forward compatibility with new fields must add ..Default::default() whenever using a struct expression for a struct that implements the Default trait.

If we go this route I'll add an issue to implement Default for all the bdk structs.

And maybe someday a future version of Rust will let us add the #[non_exhaustive] macro and have it work as we'd like for option 1.

[thomaseizinger]

What about the option:

1. implement the `Default` trait for structs that are part of our public API? We can then state in the docs that users who want to maintain forward compatibility with new fields must add `..Default::default()` whenever using a struct expression for a struct that implements the `Default` trait.

If we go this route I'll add an issue to implement Default for all the bdk structs.

And maybe someday a future version of Rust will let us add the #[non_exhaustive] macro and have it work as we'd like for option 1.

For EsploraBlockchainConfig though, there isn't really a sensible default I am afraid unless you want to default the URL to something?

In that case, providing a constructor that only takes the mandatory arguments (in this case url) requires rather little code and allows the use of #[non_exhaustive] as well. The only difference being that users need to make the struct mutable and override the defaults instead of being able to use struct initializer syntax.

[afilini]

We've talked about this yesterday during our team meeting and we reached the conclusion that it's better to just add the field and technically break the API: the reasoning is that we think it's better to force the user to use the explicit initialization syntax so that they are aware of all the existing fields of the struct.

Ideally we would also implement Default, but as you said we can't since we can't assume a default URL.

[notmandatory]

Needs a rebase and then I think this one is ready to review.

[notmandatory]

ACK 3fe2380

[afilini]

Description

Add support for http/socks proxies in Esplora

Notes to the reviewers

Opening this as a draft, since I think it's gonna break for wasm32.

This is also technically an API break, which according to the new updated guidelines shouldn't happen. On the other hand, I can't think of a better way to do this. Am I supposed to make a different EsploraBlockchainConfig struct with the new field added and only deprecate the current one?

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
• I've updated CHANGELOG.md
• This pull request breaks the existing API

[notmandatory]

On the question of how to make this sort of API change, since it's an additive change with an Option value I think it can be considered backward compatible. It does still technically break the API since anyone adopting the new version will have to do something. But it doesn't change the name or functionality of any existing struct fields or functions or function arguments. The alternative would be to make a whole new struct and new versions of all the functions that use it, which seems more disruptive when the original struct is eventually removed.

@thomaseizinger, as a user, what do you think about his sort of change? would it disrupt your workflow to have to add a new None value for this field after upgrading to a new version of bdk?

[thomaseizinger]

It is technically breaking yes.

I see two ways of changing the code so this change would be non breaking:

1. Put #[non_exhaustive] on such kind of structs which forces users to use ::default or ::new to make an instance.
2. Make all fields private and use a builder pattern to initialize such configurations:

EsploraBlockchainConfig::default().with_xyz().with_abc()

(1) is less code (no builders and getters) and would result in usage such as:

let config = EsploraBlockchainConfig{concurrency:Some(4),
..EsploraBlockchainConfig::default()};

Personally, I think (1) would be the better option unless you want to support Rust < 1.40.

For this particular PR, it is still a breaking change but we might at least take this opportunity to make future changes non-breaking :)

[RCasatta]

is less code (no builders and getters) and would result in usage such as:

 let config = EsploraBlockchainConfig {
concurrency: Some(4),
..EsploraBlockchainConfig::default()
};

It looks this doesn't work for users outside the bdk crate (while logically flawless)

[thomaseizinger]

is less code (no builders and getters) and would result in usage such as:

 let config = EsploraBlockchainConfig {
concurrency: Some(4),
..EsploraBlockchainConfig::default()
};

It looks this doesn't work for users outside the bdk crate (while logically flawless)

Damn, sorry about that. I swear I did this before with non-exhaustive structs. In guess it will have to be something like this then:

letmut config = ElectrumBlockchainConfig::new("foo".to_owned());
config.retry = 1;

[notmandatory]

What about the option:

3. implement the Default trait for structs that are part of our public API? We can then state in the docs that users who want to maintain forward compatibility with new fields must add ..Default::default() whenever using a struct expression for a struct that implements the Default trait.

If we go this route I'll add an issue to implement Default for all the bdk structs.

And maybe someday a future version of Rust will let us add the #[non_exhaustive] macro and have it work as we'd like for option 1.

[thomaseizinger]

What about the option:

1. implement the `Default` trait for structs that are part of our public API? We can then state in the docs that users who want to maintain forward compatibility with new fields must add `..Default::default()` whenever using a struct expression for a struct that implements the `Default` trait.

If we go this route I'll add an issue to implement Default for all the bdk structs.

And maybe someday a future version of Rust will let us add the #[non_exhaustive] macro and have it work as we'd like for option 1.

For EsploraBlockchainConfig though, there isn't really a sensible default I am afraid unless you want to default the URL to something?

In that case, providing a constructor that only takes the mandatory arguments (in this case url) requires rather little code and allows the use of #[non_exhaustive] as well. The only difference being that users need to make the struct mutable and override the defaults instead of being able to use struct initializer syntax.

[afilini]

We've talked about this yesterday during our team meeting and we reached the conclusion that it's better to just add the field and technically break the API: the reasoning is that we think it's better to force the user to use the explicit initialization syntax so that they are aware of all the existing fields of the struct.

Ideally we would also implement Default, but as you said we can't since we can't assume a default URL.

[notmandatory]

Needs a rebase and then I think this one is ready to review.

[notmandatory]

ACK 3fe2380

[afilini]

Description

Add support for http/socks proxies in Esplora

Notes to the reviewers

Opening this as a draft, since I think it's gonna break for wasm32.

This is also technically an API break, which according to the new updated guidelines shouldn't happen. On the other hand, I can't think of a better way to do this. Am I supposed to make a different EsploraBlockchainConfig struct with the new field added and only deprecate the current one?

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
• I've updated CHANGELOG.md
• This pull request breaks the existing API

[notmandatory]

On the question of how to make this sort of API change, since it's an additive change with an Option value I think it can be considered backward compatible. It does still technically break the API since anyone adopting the new version will have to do something. But it doesn't change the name or functionality of any existing struct fields or functions or function arguments. The alternative would be to make a whole new struct and new versions of all the functions that use it, which seems more disruptive when the original struct is eventually removed.

@thomaseizinger, as a user, what do you think about his sort of change? would it disrupt your workflow to have to add a new None value for this field after upgrading to a new version of bdk?

[thomaseizinger]

It is technically breaking yes.

I see two ways of changing the code so this change would be non breaking:

1. Put #[non_exhaustive] on such kind of structs which forces users to use ::default or ::new to make an instance.
2. Make all fields private and use a builder pattern to initialize such configurations:

EsploraBlockchainConfig::default().with_xyz().with_abc()

(1) is less code (no builders and getters) and would result in usage such as:

let config = EsploraBlockchainConfig{concurrency:Some(4),
..EsploraBlockchainConfig::default()};

Personally, I think (1) would be the better option unless you want to support Rust < 1.40.

For this particular PR, it is still a breaking change but we might at least take this opportunity to make future changes non-breaking :)

[RCasatta]

is less code (no builders and getters) and would result in usage such as:

 let config = EsploraBlockchainConfig {
concurrency: Some(4),
..EsploraBlockchainConfig::default()
};

It looks this doesn't work for users outside the bdk crate (while logically flawless)

[thomaseizinger]

is less code (no builders and getters) and would result in usage such as:

 let config = EsploraBlockchainConfig {
concurrency: Some(4),
..EsploraBlockchainConfig::default()
};

It looks this doesn't work for users outside the bdk crate (while logically flawless)

Damn, sorry about that. I swear I did this before with non-exhaustive structs. In guess it will have to be something like this then:

letmut config = ElectrumBlockchainConfig::new("foo".to_owned());
config.retry = 1;

[notmandatory]

What about the option:

3. implement the Default trait for structs that are part of our public API? We can then state in the docs that users who want to maintain forward compatibility with new fields must add ..Default::default() whenever using a struct expression for a struct that implements the Default trait.

If we go this route I'll add an issue to implement Default for all the bdk structs.

And maybe someday a future version of Rust will let us add the #[non_exhaustive] macro and have it work as we'd like for option 1.

[thomaseizinger]

What about the option:

1. implement the `Default` trait for structs that are part of our public API? We can then state in the docs that users who want to maintain forward compatibility with new fields must add `..Default::default()` whenever using a struct expression for a struct that implements the `Default` trait.

If we go this route I'll add an issue to implement Default for all the bdk structs.

And maybe someday a future version of Rust will let us add the #[non_exhaustive] macro and have it work as we'd like for option 1.

For EsploraBlockchainConfig though, there isn't really a sensible default I am afraid unless you want to default the URL to something?

In that case, providing a constructor that only takes the mandatory arguments (in this case url) requires rather little code and allows the use of #[non_exhaustive] as well. The only difference being that users need to make the struct mutable and override the defaults instead of being able to use struct initializer syntax.

[afilini]

We've talked about this yesterday during our team meeting and we reached the conclusion that it's better to just add the field and technically break the API: the reasoning is that we think it's better to force the user to use the explicit initialization syntax so that they are aware of all the existing fields of the struct.

Ideally we would also implement Default, but as you said we can't since we can't assume a default URL.

[notmandatory]

Needs a rebase and then I think this one is ready to review.

[notmandatory]

ACK 3fe2380

[afilini]

Description

Add support for http/socks proxies in Esplora

Notes to the reviewers

Opening this as a draft, since I think it's gonna break for wasm32.

This is also technically an API break, which according to the new updated guidelines shouldn't happen. On the other hand, I can't think of a better way to do this. Am I supposed to make a different EsploraBlockchainConfig struct with the new field added and only deprecate the current one?

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
• I've updated CHANGELOG.md
• This pull request breaks the existing API

[notmandatory]

On the question of how to make this sort of API change, since it's an additive change with an Option value I think it can be considered backward compatible. It does still technically break the API since anyone adopting the new version will have to do something. But it doesn't change the name or functionality of any existing struct fields or functions or function arguments. The alternative would be to make a whole new struct and new versions of all the functions that use it, which seems more disruptive when the original struct is eventually removed.

@thomaseizinger, as a user, what do you think about his sort of change? would it disrupt your workflow to have to add a new None value for this field after upgrading to a new version of bdk?

[thomaseizinger]

It is technically breaking yes.

I see two ways of changing the code so this change would be non breaking:

1. Put #[non_exhaustive] on such kind of structs which forces users to use ::default or ::new to make an instance.
2. Make all fields private and use a builder pattern to initialize such configurations:

EsploraBlockchainConfig::default().with_xyz().with_abc()

(1) is less code (no builders and getters) and would result in usage such as:

let config = EsploraBlockchainConfig{concurrency:Some(4),
..EsploraBlockchainConfig::default()};

Personally, I think (1) would be the better option unless you want to support Rust < 1.40.

For this particular PR, it is still a breaking change but we might at least take this opportunity to make future changes non-breaking :)

[RCasatta]

is less code (no builders and getters) and would result in usage such as:

 let config = EsploraBlockchainConfig {
concurrency: Some(4),
..EsploraBlockchainConfig::default()
};

It looks this doesn't work for users outside the bdk crate (while logically flawless)

[thomaseizinger]

is less code (no builders and getters) and would result in usage such as:

 let config = EsploraBlockchainConfig {
concurrency: Some(4),
..EsploraBlockchainConfig::default()
};

It looks this doesn't work for users outside the bdk crate (while logically flawless)

Damn, sorry about that. I swear I did this before with non-exhaustive structs. In guess it will have to be something like this then:

letmut config = ElectrumBlockchainConfig::new("foo".to_owned());
config.retry = 1;

[notmandatory]

What about the option:

3. implement the Default trait for structs that are part of our public API? We can then state in the docs that users who want to maintain forward compatibility with new fields must add ..Default::default() whenever using a struct expression for a struct that implements the Default trait.

If we go this route I'll add an issue to implement Default for all the bdk structs.

And maybe someday a future version of Rust will let us add the #[non_exhaustive] macro and have it work as we'd like for option 1.

[thomaseizinger]

What about the option:

1. implement the `Default` trait for structs that are part of our public API? We can then state in the docs that users who want to maintain forward compatibility with new fields must add `..Default::default()` whenever using a struct expression for a struct that implements the `Default` trait.

If we go this route I'll add an issue to implement Default for all the bdk structs.

And maybe someday a future version of Rust will let us add the #[non_exhaustive] macro and have it work as we'd like for option 1.

For EsploraBlockchainConfig though, there isn't really a sensible default I am afraid unless you want to default the URL to something?

In that case, providing a constructor that only takes the mandatory arguments (in this case url) requires rather little code and allows the use of #[non_exhaustive] as well. The only difference being that users need to make the struct mutable and override the defaults instead of being able to use struct initializer syntax.

[afilini]

We've talked about this yesterday during our team meeting and we reached the conclusion that it's better to just add the field and technically break the API: the reasoning is that we think it's better to force the user to use the explicit initialization syntax so that they are aware of all the existing fields of the struct.

Ideally we would also implement Default, but as you said we can't since we can't assume a default URL.

[notmandatory]

Needs a rebase and then I think this one is ready to review.

[notmandatory]

ACK 3fe2380

[afilini]

Description

Add support for http/socks proxies in Esplora

Notes to the reviewers

Opening this as a draft, since I think it's gonna break for wasm32.

This is also technically an API break, which according to the new updated guidelines shouldn't happen. On the other hand, I can't think of a better way to do this. Am I supposed to make a different EsploraBlockchainConfig struct with the new field added and only deprecate the current one?

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
• I've updated CHANGELOG.md
• This pull request breaks the existing API

[notmandatory]

On the question of how to make this sort of API change, since it's an additive change with an Option value I think it can be considered backward compatible. It does still technically break the API since anyone adopting the new version will have to do something. But it doesn't change the name or functionality of any existing struct fields or functions or function arguments. The alternative would be to make a whole new struct and new versions of all the functions that use it, which seems more disruptive when the original struct is eventually removed.

@thomaseizinger, as a user, what do you think about his sort of change? would it disrupt your workflow to have to add a new None value for this field after upgrading to a new version of bdk?

[thomaseizinger]

It is technically breaking yes.

I see two ways of changing the code so this change would be non breaking:

1. Put #[non_exhaustive] on such kind of structs which forces users to use ::default or ::new to make an instance.
2. Make all fields private and use a builder pattern to initialize such configurations:

EsploraBlockchainConfig::default().with_xyz().with_abc()

(1) is less code (no builders and getters) and would result in usage such as:

let config = EsploraBlockchainConfig{concurrency:Some(4),
..EsploraBlockchainConfig::default()};

Personally, I think (1) would be the better option unless you want to support Rust < 1.40.

For this particular PR, it is still a breaking change but we might at least take this opportunity to make future changes non-breaking :)

[RCasatta]

is less code (no builders and getters) and would result in usage such as:

 let config = EsploraBlockchainConfig {
concurrency: Some(4),
..EsploraBlockchainConfig::default()
};

It looks this doesn't work for users outside the bdk crate (while logically flawless)

[thomaseizinger]

is less code (no builders and getters) and would result in usage such as:

 let config = EsploraBlockchainConfig {
concurrency: Some(4),
..EsploraBlockchainConfig::default()
};

It looks this doesn't work for users outside the bdk crate (while logically flawless)

Damn, sorry about that. I swear I did this before with non-exhaustive structs. In guess it will have to be something like this then:

letmut config = ElectrumBlockchainConfig::new("foo".to_owned());
config.retry = 1;

[notmandatory]

What about the option:

3. implement the Default trait for structs that are part of our public API? We can then state in the docs that users who want to maintain forward compatibility with new fields must add ..Default::default() whenever using a struct expression for a struct that implements the Default trait.

If we go this route I'll add an issue to implement Default for all the bdk structs.

And maybe someday a future version of Rust will let us add the #[non_exhaustive] macro and have it work as we'd like for option 1.

[thomaseizinger]

What about the option:

1. implement the `Default` trait for structs that are part of our public API? We can then state in the docs that users who want to maintain forward compatibility with new fields must add `..Default::default()` whenever using a struct expression for a struct that implements the `Default` trait.

If we go this route I'll add an issue to implement Default for all the bdk structs.

And maybe someday a future version of Rust will let us add the #[non_exhaustive] macro and have it work as we'd like for option 1.

For EsploraBlockchainConfig though, there isn't really a sensible default I am afraid unless you want to default the URL to something?

In that case, providing a constructor that only takes the mandatory arguments (in this case url) requires rather little code and allows the use of #[non_exhaustive] as well. The only difference being that users need to make the struct mutable and override the defaults instead of being able to use struct initializer syntax.

[afilini]

We've talked about this yesterday during our team meeting and we reached the conclusion that it's better to just add the field and technically break the API: the reasoning is that we think it's better to force the user to use the explicit initialization syntax so that they are aware of all the existing fields of the struct.

Ideally we would also implement Default, but as you said we can't since we can't assume a default URL.

[notmandatory]

Needs a rebase and then I think this one is ready to review.

[notmandatory]

ACK 3fe2380

[afilini]

Description

Add support for http/socks proxies in Esplora

Notes to the reviewers

Opening this as a draft, since I think it's gonna break for wasm32.

This is also technically an API break, which according to the new updated guidelines shouldn't happen. On the other hand, I can't think of a better way to do this. Am I supposed to make a different EsploraBlockchainConfig struct with the new field added and only deprecate the current one?

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
• I've updated CHANGELOG.md
• This pull request breaks the existing API

[notmandatory]

On the question of how to make this sort of API change, since it's an additive change with an Option value I think it can be considered backward compatible. It does still technically break the API since anyone adopting the new version will have to do something. But it doesn't change the name or functionality of any existing struct fields or functions or function arguments. The alternative would be to make a whole new struct and new versions of all the functions that use it, which seems more disruptive when the original struct is eventually removed.

@thomaseizinger, as a user, what do you think about his sort of change? would it disrupt your workflow to have to add a new None value for this field after upgrading to a new version of bdk?

[thomaseizinger]

It is technically breaking yes.

I see two ways of changing the code so this change would be non breaking:

1. Put #[non_exhaustive] on such kind of structs which forces users to use ::default or ::new to make an instance.
2. Make all fields private and use a builder pattern to initialize such configurations:

EsploraBlockchainConfig::default().with_xyz().with_abc()

(1) is less code (no builders and getters) and would result in usage such as:

let config = EsploraBlockchainConfig{concurrency:Some(4),
..EsploraBlockchainConfig::default()};

Personally, I think (1) would be the better option unless you want to support Rust < 1.40.

For this particular PR, it is still a breaking change but we might at least take this opportunity to make future changes non-breaking :)

[RCasatta]

is less code (no builders and getters) and would result in usage such as:

 let config = EsploraBlockchainConfig {
concurrency: Some(4),
..EsploraBlockchainConfig::default()
};

It looks this doesn't work for users outside the bdk crate (while logically flawless)

[thomaseizinger]

is less code (no builders and getters) and would result in usage such as:

 let config = EsploraBlockchainConfig {
concurrency: Some(4),
..EsploraBlockchainConfig::default()
};

It looks this doesn't work for users outside the bdk crate (while logically flawless)

Damn, sorry about that. I swear I did this before with non-exhaustive structs. In guess it will have to be something like this then:

letmut config = ElectrumBlockchainConfig::new("foo".to_owned());
config.retry = 1;

[notmandatory]

What about the option:

3. implement the Default trait for structs that are part of our public API? We can then state in the docs that users who want to maintain forward compatibility with new fields must add ..Default::default() whenever using a struct expression for a struct that implements the Default trait.

If we go this route I'll add an issue to implement Default for all the bdk structs.

And maybe someday a future version of Rust will let us add the #[non_exhaustive] macro and have it work as we'd like for option 1.

[thomaseizinger]

What about the option:

1. implement the `Default` trait for structs that are part of our public API? We can then state in the docs that users who want to maintain forward compatibility with new fields must add `..Default::default()` whenever using a struct expression for a struct that implements the `Default` trait.

If we go this route I'll add an issue to implement Default for all the bdk structs.

And maybe someday a future version of Rust will let us add the #[non_exhaustive] macro and have it work as we'd like for option 1.

For EsploraBlockchainConfig though, there isn't really a sensible default I am afraid unless you want to default the URL to something?

In that case, providing a constructor that only takes the mandatory arguments (in this case url) requires rather little code and allows the use of #[non_exhaustive] as well. The only difference being that users need to make the struct mutable and override the defaults instead of being able to use struct initializer syntax.

[afilini]

We've talked about this yesterday during our team meeting and we reached the conclusion that it's better to just add the field and technically break the API: the reasoning is that we think it's better to force the user to use the explicit initialization syntax so that they are aware of all the existing fields of the struct.

Ideally we would also implement Default, but as you said we can't since we can't assume a default URL.

[notmandatory]

Needs a rebase and then I think this one is ready to review.

[notmandatory]

ACK 3fe2380

[afilini]

Description

Add support for http/socks proxies in Esplora

Notes to the reviewers

Opening this as a draft, since I think it's gonna break for wasm32.

This is also technically an API break, which according to the new updated guidelines shouldn't happen. On the other hand, I can't think of a better way to do this. Am I supposed to make a different EsploraBlockchainConfig struct with the new field added and only deprecate the current one?

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
• I've updated CHANGELOG.md
• This pull request breaks the existing API

[notmandatory]

On the question of how to make this sort of API change, since it's an additive change with an Option value I think it can be considered backward compatible. It does still technically break the API since anyone adopting the new version will have to do something. But it doesn't change the name or functionality of any existing struct fields or functions or function arguments. The alternative would be to make a whole new struct and new versions of all the functions that use it, which seems more disruptive when the original struct is eventually removed.

@thomaseizinger, as a user, what do you think about his sort of change? would it disrupt your workflow to have to add a new None value for this field after upgrading to a new version of bdk?

[thomaseizinger]

It is technically breaking yes.

I see two ways of changing the code so this change would be non breaking:

1. Put #[non_exhaustive] on such kind of structs which forces users to use ::default or ::new to make an instance.
2. Make all fields private and use a builder pattern to initialize such configurations:

EsploraBlockchainConfig::default().with_xyz().with_abc()

(1) is less code (no builders and getters) and would result in usage such as:

let config = EsploraBlockchainConfig{concurrency:Some(4),
..EsploraBlockchainConfig::default()};

Personally, I think (1) would be the better option unless you want to support Rust < 1.40.

For this particular PR, it is still a breaking change but we might at least take this opportunity to make future changes non-breaking :)

[RCasatta]

is less code (no builders and getters) and would result in usage such as:

 let config = EsploraBlockchainConfig {
concurrency: Some(4),
..EsploraBlockchainConfig::default()
};

It looks this doesn't work for users outside the bdk crate (while logically flawless)

[thomaseizinger]

is less code (no builders and getters) and would result in usage such as:

 let config = EsploraBlockchainConfig {
concurrency: Some(4),
..EsploraBlockchainConfig::default()
};

It looks this doesn't work for users outside the bdk crate (while logically flawless)

Damn, sorry about that. I swear I did this before with non-exhaustive structs. In guess it will have to be something like this then:

letmut config = ElectrumBlockchainConfig::new("foo".to_owned());
config.retry = 1;

[notmandatory]

What about the option:

3. implement the Default trait for structs that are part of our public API? We can then state in the docs that users who want to maintain forward compatibility with new fields must add ..Default::default() whenever using a struct expression for a struct that implements the Default trait.

If we go this route I'll add an issue to implement Default for all the bdk structs.

And maybe someday a future version of Rust will let us add the #[non_exhaustive] macro and have it work as we'd like for option 1.

[thomaseizinger]

What about the option:

1. implement the `Default` trait for structs that are part of our public API? We can then state in the docs that users who want to maintain forward compatibility with new fields must add `..Default::default()` whenever using a struct expression for a struct that implements the `Default` trait.

If we go this route I'll add an issue to implement Default for all the bdk structs.

And maybe someday a future version of Rust will let us add the #[non_exhaustive] macro and have it work as we'd like for option 1.

For EsploraBlockchainConfig though, there isn't really a sensible default I am afraid unless you want to default the URL to something?

In that case, providing a constructor that only takes the mandatory arguments (in this case url) requires rather little code and allows the use of #[non_exhaustive] as well. The only difference being that users need to make the struct mutable and override the defaults instead of being able to use struct initializer syntax.

[afilini]

We've talked about this yesterday during our team meeting and we reached the conclusion that it's better to just add the field and technically break the API: the reasoning is that we think it's better to force the user to use the explicit initialization syntax so that they are aware of all the existing fields of the struct.

Ideally we would also implement Default, but as you said we can't since we can't assume a default URL.

[notmandatory]

Needs a rebase and then I think this one is ready to review.

[notmandatory]

ACK 3fe2380