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

Standardize Error Handling #86

Open
a-moreira opened this issue Oct 23, 2023 · 11 comments · May be fixed by #150
Open

Standardize Error Handling #86

a-moreira opened this issue Oct 23, 2023 · 11 comments · May be fixed by #150
Assignees
Labels
chore Cleaning, refactoring, reducing complexity code quality Generally improves code readability and maintainability good first issue Good for newcomers
Milestone

Comments

@a-moreira
Copy link
Collaborator

a-moreira commented Oct 23, 2023

We currently use multiple error handling crates like anyhow, thiserror, and also custom macros.
It'd be good to standardize this and pick one option.

@a-moreira a-moreira converted this from a draft issue Oct 23, 2023
@a-moreira a-moreira added the chore Cleaning, refactoring, reducing complexity label Oct 23, 2023
@Davidson-Souza Davidson-Souza added good first issue Good for newcomers code quality Generally improves code readability and maintainability labels Oct 23, 2023
@Vikaass-08
Copy link
Contributor

Hey, I want to work on this task can you please assign it to me?? @a-moreira @Davidson-Souza

@Davidson-Souza
Copy link
Collaborator

Hey @Vikaass-08 thanks for the interest. You got it!

@a-moreira
Copy link
Collaborator Author

hey @Vikaass-08, first we need to list the crates and where we use them, so we can make sense of the reason they were picked. Then we will decide the best crate to use on the whole project, or if we are ditching them and using our custom macros.

@Vikaass-08
Copy link
Contributor

Ok let's work on that first. @a-moreira

@a-moreira
Copy link
Collaborator Author

@Vikaass-08 nice, let us know if you need any help!

@Vikaass-08
Copy link
Contributor

Vikaass-08 commented Oct 25, 2023

Screenshot 2023-10-25 at 10 16 32 PM

Getting this Error during cargo build, can you help me with this?? @a-moreira

I have gone through the codebase and found Few CustomErrors that uses thisError.

  • BlockchainError
  • Error
  • WireError
  • PeerError

Are we planning to combine these Errors, apart From these are there some other cases??

@Davidson-Souza
Copy link
Collaborator

Oh, this is actually something we need to fix here. This is a known problem with rustc 1.73 IIRC, there's a backport in rust-miniscript, but we're using an out-of-tree version.

For now, try using a compiler <1.73. I'm using 1.72.1.

@Davidson-Souza Davidson-Souza added this to the v0.6.0 milestone Nov 21, 2023
@Vikaass-08
Copy link
Contributor

  • BlockchainError
  • Error
  • WireError
  • PeerError
  • BlockValidationErrors
  • KvDatabaseError
  • WatchOnlyError
  • Socks5Error

These are all the Errors that i Found in the codebase @a-moreira

@shivam-Purohit
Copy link

@Davidson-Souza @a-moreira
if this is going stale can i try it?

@Davidson-Souza
Copy link
Collaborator

I haven't seen any update in a while. You are welcome to open a pr addressing this

@jaoleal
Copy link
Contributor

jaoleal commented May 17, 2024

I made a PR with a strategy that maybe can feel a little like unnecessary work, but to reverse the "Isolating the errors in a crate" is quite easy since the work is done and the project compiles without any bugs...
And all the errors will "enherit" the native error, without the needing to reimplement the errors, but there are ones that i simply dont know who to enherit because is just a enum, for that ones i just displayed their names.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
chore Cleaning, refactoring, reducing complexity code quality Generally improves code readability and maintainability good first issue Good for newcomers
Projects
None yet
Development

Successfully merging a pull request may close this issue.

5 participants