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

core: fixing the esplora error match #417

Merged
merged 1 commit into from
Dec 4, 2024

Conversation

vincenzopalazzo
Copy link
Contributor

A recent version of esplora (from v0.10) changed the error type, so this commit is reflecting the change.

Fixes #412

We may want also consider to removing the FIXME comment in a separate commit?

Recent version of esplora (from v0.10) changed the
error type, so this commit is reflecting the change.

Link: lightningdevkit#412
Signed-off-by: Vincenzo Palazzo <[email protected]>
@vincenzopalazzo
Copy link
Contributor Author

Ci look like unrelated?

Copy link
Collaborator

@tnull tnull left a comment

Choose a reason for hiding this comment

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

Thanks, LGTM!

We may want also consider to removing the FIXME comment in a separate commit?

Mhh, while I still think it would be great to differentiate some error variants here (e.g. the 'transaction already known' error when trying to broadcast transactions that the Bitcoin Core node behind the Esplora server already has in its mempool), I suspect doing so via string matching might prove to be error prone/flaky after all. We could consider just dropping the FIXME, but also not sure if we shouldn't revisit in the future..

In any case, I'll go ahead and land this for now.

(Btw, minor nit: The commit message could be a bit more verbose in general, but in particular it's also esplora-client 0.7 that started shipping the HttpResponse error variant I believe, we just couldn't update for the longest time).

@tnull tnull merged commit 2095d87 into lightningdevkit:main Dec 4, 2024
4 of 13 checks passed
@vincenzopalazzo vincenzopalazzo deleted the macros/esplora-api branch December 4, 2024 11:11
@vincenzopalazzo
Copy link
Contributor Author

(Btw, minor nit: The commit message could be a bit more verbose in general, but in particular it's also esplora-client 0.7 that started shipping the HttpResponse error variant I believe, we just couldn't update for the longest time).

Oh so probably I had to git blame a little bit better in esplora-client. Sorry, if you can git commit --amend and change it, would be good to keep the history sane. Sorry for not paying attention to it.

@tnull
Copy link
Collaborator

tnull commented Dec 4, 2024

(Btw, minor nit: The commit message could be a bit more verbose in general, but in particular it's also esplora-client 0.7 that started shipping the HttpResponse error variant I believe, we just couldn't update for the longest time).

Oh so probably I had to git blame a little bit better in esplora-client. Sorry, if you can git commit --amend and change it, would be good to keep the history sane. Sorry for not paying attention to it.

No worries, really nbd.!

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.

Failed to broadcast transaction, status 400, logged at ERROR level
2 participants