Skip to content

simln-lib: refactor ForwardingError to nested results #265

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

Merged
merged 1 commit into from
May 13, 2025

Conversation

elnosh
Copy link
Collaborator

@elnosh elnosh commented May 12, 2025

doing as a prefactor for #261

Previously we were checking for critical errors with is_critical on the ForwardingError to trigger a simulation shutdown. Added a CriticalError type that includes the errors we considered critical and now returning them throughout the payment propagation in nested results to better differentiate between the 2 types of errors.

@carlaKC carlaKC requested review from carlaKC and removed request for carlaKC May 13, 2025 15:22
@elnosh elnosh requested a review from carlaKC May 13, 2025 15:39
Copy link
Contributor

@carlaKC carlaKC left a comment

Choose a reason for hiding this comment

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

One vague musing about error handling + some nits, nice PR 🌵

)
}
}
type ForwardResult = Result<Result<(), ForwardingError>, CriticalError>;
Copy link
Contributor

Choose a reason for hiding this comment

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

Will the plan here to be to add a variant for interceptors in both CriticalError and ForwardingError which external implementations would use?

An alternative could be to just make this a BoxError instead of CriticalError so that callers don't have to wrap their errors. There's no getting round adding a variant for ForwardingError so seems reasonable to just do this consistently.

Mostly musing out loud, think we could go either way here.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Will the plan here to be to add a variant for interceptors in both CriticalError and ForwardingError which external implementations would use?

yeah. If they encountered some regular error, provide it in something like ForwardingError::InterceptError(error) and if they encountered an error that warranted a simulation shutdown CriticalError::InterceptError(critical_error_msg)

There's no getting round adding a variant for ForwardingError so seems reasonable to just do this consistently.

sorry, not quite sure I understood this part. Could you expand here a bit more 😅

Copy link
Contributor

Choose a reason for hiding this comment

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

sorry, not quite sure I understood this part. Could you expand here a bit more 😅

Just meaning that we're definitely going to have to add ForwardingError::InterceptorError(...) so that interceptor errors can provide "inner" errors - basically what you said above!

Previously we were mixing regular fowarding errors like
insufficient balance and critical errors that warranted a shutdown
in the same ForwardingError type. Use nested results when
propagating a payment to differentiate between them.
@carlaKC carlaKC merged commit 8cf2042 into bitcoin-dev-project:main May 13, 2025
2 checks passed
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.

2 participants