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

Don't panic, handle errors. #57

Merged
merged 6 commits into from
Nov 11, 2022
Merged

Don't panic, handle errors. #57

merged 6 commits into from
Nov 11, 2022

Conversation

DanGould
Copy link
Contributor

@DanGould DanGould commented Nov 9, 2022

Fix "unwrap()s everywhere" disclaimer by handling errors. They still need to propagate to the UI.

HttpErrors still need to be returned with proper HTTP status codes

There are still 2 unwraps on mutex locks let mut pj_by_spk = self.pjs.lock().unwrap(); which should probably panic with expect instead. What do you think?

I also need to add @evanlinjin as co-author of the HttpError. Work from #10 finally making it in!

fix #33

#50 could be fixed in this PR by pushing an update to the front end. but this at least responds by passing the error to the sender where they should end up in logs

@DanGould DanGould force-pushed the error branch 2 times, most recently from de06ab1 to 57e8932 Compare November 9, 2022 21:55
@DanGould DanGould marked this pull request as ready for review November 9, 2022 21:56
*not_found.status_mut() = StatusCode::NOT_FOUND;
Ok(not_found)
let headers = Headers(req.headers().to_owned());
let query = {
Copy link
Collaborator

Choose a reason for hiding this comment

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

I couldn't find a cleaner way of getting around not having Copy..

let body = req.into_body();
let bytes = hyper::body::to_bytes(body).await?;
dbg!(&bytes); // this is correct by my accounts
let reader = &*bytes;
Copy link
Collaborator

Choose a reason for hiding this comment

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

Do we actually want to dbg! these bytes?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

It has let us check the original PSBT in the past. Of little use here. We could introduce the log crate to show debug/warn/errors logging soon

Copy link
Contributor Author

Choose a reason for hiding this comment

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

removed it for this pr

let resp = Response::new(Body::from(self.to_string()));
let (mut parts, body) = resp.into_parts();

// TODO respond with well known errors as defined in BIP-0078
Copy link
Collaborator

Choose a reason for hiding this comment

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

Will this also give us more informative errors on most senders?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

The only difference to start would be UI would say

"not-enough-money | The receiver added some inputs but could not bump the fee of the payjoin proposal."

or

"original-psbt-rejected | The receiver rejected the original PSBT. "

otherwise the spec says to put errors into logs only. This becomes more useful when we're using our own sender because we can show can decide to propagate new well-known errors into our UIs.

https://github.com/bitcoin/bips/blob/master/bip-0078.mediawiki#receivers-well-known-errors

@nickfarrow
Copy link
Collaborator

Looks good to me! Great stuff.

Yep expect sounds right to use expects there, probably not worth retries or anything complex

DanGould and others added 6 commits November 11, 2022 09:27
Co-authored-by: evanlinjin <hello@evanlinjin.me>
Co-authored-by: evanlinjin <hello@evanlinjin.me>
Fix unwrap()s and panic!()s by introducing new LndError variants.
@DanGould DanGould merged commit f96a273 into payjoin:master Nov 11, 2022
@DanGould DanGould deleted the error branch November 11, 2022 14:34
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.

Handle "peer <id> is not online"
2 participants