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

Re-use bitcoin crate exported by gl-client #779

Merged
merged 1 commit into from
Feb 8, 2024

Conversation

ok300
Copy link
Contributor

@ok300 ok300 commented Feb 8, 2024

This PR removes the explicit dependency on bitcoin in our Cargo.toml. Instead of it, we use the version re-exported by GL at gl_client::bitcoin.

This helps us avoid having to track and re-align the bitcoin version with each gl_client update.

@JssDWt
Copy link
Contributor

JssDWt commented Feb 8, 2024

When is this an issue? When gl_client updates their bitcoin version? Only on minor version upgrades, or any upgrade? Looks like this here contains a downgrade of the bitcoin crate? #777

@ok300
Copy link
Contributor Author

ok300 commented Feb 8, 2024

When is this an issue? When gl_client updates their bitcoin version? Only on minor version upgrades, or any upgrade?

There are different uses for the bitcoin crate:

  • sometimes we use it directly
  • sometimes we use structs from the bitcoin crate of other dependencies (i.e. its a transitive dependency), so there rust will choose the right version
  • sometimes, things can be serialized with one version, then deserialized with another, which can be tricky to spot

My goal here is to reduce these possible combinations. If we use GL's version, the matrix of combinations has one less row.

Looks like this here contains a downgrade of the bitcoin crate? #777

GL is internally using 3 different bitcoin versions, each coming from different dependencies. I don't know if they upgrade or downgrade, but IMO GL is the most important dependency we have, so its best if we align with the version they use. This PR does that automatically.

Copy link
Contributor

@JssDWt JssDWt left a comment

Choose a reason for hiding this comment

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

LGTM, but madness that this is necessary ofcourse.

@ok300 ok300 merged commit 59e561a into main Feb 8, 2024
7 checks passed
@ok300 ok300 deleted the ok300-consolidate-bitcoin-dependency branch February 8, 2024 11:47
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.

None yet

2 participants