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

Improve error handling #2

Open
lrettig opened this issue Apr 26, 2023 · 1 comment
Open

Improve error handling #2

lrettig opened this issue Apr 26, 2023 · 1 comment
Labels
help wanted Extra attention is needed

Comments

@lrettig
Copy link
Member

lrettig commented Apr 26, 2023

Error handling in this library should be improved. Rather than printing errors in the FFI extern function, they should be returned to the caller, and likely some of the error checking logic should also be moved to the caller. See below notes:

          > would it make sense to implement this checks on consumer side? i thought that this is ok to panic, as consumer of this lib should do this validation or use valid key/path

Good question. The issue with panic here is that AFAICT it's not possible to trap or handle a panic across the FFI boundary - the process just abruptly dies with basically no information about what's wrong. Yes, we could try to recreate the error-checking logic on the consumer side, but that's a duplication of code and logic and if the upstream library changes, the consumer logic will not. I'd be okay with removing the "path too short", "path too long", etc. error checking to the consumer but I'd prefer to leave the other errors (failed to convert string, failed to parse path) in place.

In general the error handling here could be improved a bunch - rather than printing errors, they should be returned, but we'd need a mechanism for returning errors across the FFI boundary. I suppose just returning a string explanation along with the null ptr would be sufficient. I'll open an issue so we can follow up on this later.

Originally posted by @lrettig in spacemeshos/ed25519_bip32#5 (comment)

@lrettig lrettig added the help wanted Extra attention is needed label Apr 26, 2023
@lrettig lrettig transferred this issue from spacemeshos/ed25519_bip32 May 5, 2023
@lrettig
Copy link
Member Author

lrettig commented May 25, 2023

The .map_err(...) could be improved with use of wonderful crates like eyre or anyhow that allow providing context easily and record error backtrace for easier debugging.

Originally posted by @poszu in #4 (comment)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
help wanted Extra attention is needed
Projects
None yet
Development

No branches or pull requests

1 participant