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

Prefer returning errors over panicking where possible #119

Merged
merged 10 commits into from
Jun 21, 2023

Conversation

tnull
Copy link
Collaborator

@tnull tnull commented Jun 6, 2023

Based on #115.
Fixes #50.

@tnull tnull marked this pull request as draft June 6, 2023 10:40
@tnull tnull changed the title WIP: Avoid panicking where possible WIP: Prefer returning errors over panicking where possible Jun 6, 2023
@tnull tnull force-pushed the 2023-06-dont-panic branch 3 times, most recently from 8d85649 to eae4d06 Compare June 14, 2023 07:39
@tnull tnull changed the title WIP: Prefer returning errors over panicking where possible Prefer returning errors over panicking where possible Jun 14, 2023
@tnull tnull marked this pull request as ready for review June 14, 2023 10:01
@tnull tnull force-pushed the 2023-06-dont-panic branch 2 times, most recently from ea7f0e0 to d37923a Compare June 14, 2023 10:39
@tnull tnull mentioned this pull request Jun 13, 2023
47 tasks
@tnull tnull added this to the 0.1 milestone Jun 14, 2023
@tnull
Copy link
Collaborator Author

tnull commented Jun 16, 2023

Rebased after #115 was merged.

While it's not strictly necessary for us internally, it's nice to add to allow the
Node itself to be used in tokio context.
src/builder.rs Outdated Show resolved Hide resolved
src/wallet.rs Outdated Show resolved Hide resolved
Copy link
Contributor

@jkczyz jkczyz left a comment

Choose a reason for hiding this comment

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

LGTM. Please squash fixups.

While we cannot get rid of the panics here, we can make sure the errors
are logged before we (explicitly) panic.
Which we'll be using in the next commit.
While we cannot get rid of the panics here, we can make sure the errors
are logged before we (explicitly) panic.
@tnull
Copy link
Collaborator Author

tnull commented Jun 21, 2023

LGTM. Please squash fixups.

Squashed without further changes.

While we cannot get rid of the panics here, we can make sure the errors
are logged before we (explicitly) panic.
As we now block `start()` on updating fees anyways we don't have to call
`sync_wallets` manually to check we're successfully connected to the
testnet esplora. We here remove this as it can be flaky dependent how
many requests the Esplora server is accepting and how often our CI runs
from the same machines...
@tnull
Copy link
Collaborator Author

tnull commented Jun 21, 2023

Now included another commit cleaning up a unit test that was flaky in CI, depending if Esplora server quotas were hit.

@tnull tnull merged commit 59a425e into lightningdevkit:main Jun 21, 2023
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.

Avoid panics where possible
2 participants