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

tx_builder: Relax generic constraints on TxBuilder #1344

Merged

Conversation

stevenroose
Copy link
Contributor

Closes #1312

Copy link
Contributor

@storopoli storopoli left a comment

Choose a reason for hiding this comment

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

It would be nice to add some test cases for the functionality you are fixing (similar to the code in the linked issue).

@stevenroose
Copy link
Contributor Author

I don't think it's needed here. With generic constraints: if it compiles, it works. The lower the better.

Is there something I can/should do about the canceled CI builds?

@LLFourn
Copy link
Contributor

LLFourn commented Feb 19, 2024

I agree there's no need for tests on this and its best to relax generic constraints as much as possible in rust. No idea what CI is doing. @notmandatory any ideas?

It is not needed anymore. Discovered in bitcoindevkit#1344
@danielabrozzoni
Copy link
Member

To be fair I'm not sure why the CI is failing only in this PR, while others are passing (maybe it's just that other PRs aren't touching the bdk code, and the CI cache is ✨ caching ✨ it?)

Anyways, I opened #1357 to fix, once that's merged, can you rebase please?

@stevenroose
Copy link
Contributor Author

stevenroose commented Feb 21, 2024

Rebased on top of #1357, so let's see if that fixes CI.

@notmandatory
Copy link
Member

#1357 is merged and it looks like all tests are now passing.

Copy link
Member

@danielabrozzoni danielabrozzoni left a comment

Choose a reason for hiding this comment

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

ACK 2efa299

@danielabrozzoni danielabrozzoni merged commit d77a7f2 into bitcoindevkit:master Feb 22, 2024
12 checks passed
@notmandatory notmandatory mentioned this pull request Mar 26, 2024
25 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
Archived in project
Development

Successfully merging this pull request may close these issues.

Relax generic requirements on TxBuilder
5 participants