You signed in with another tab or window. Reload to refresh your session.You signed out in another tab or window. Reload to refresh your session.You switched accounts on another tab or window. Reload to refresh your session.Dismiss alert
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.
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.
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:
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)
The text was updated successfully, but these errors were encountered: