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

Replace snafu with this_error and remove global error enum #1823

Merged
merged 7 commits into from
Aug 7, 2024

Conversation

imabdulbasit
Copy link
Contributor

@imabdulbasit imabdulbasit commented Aug 6, 2024

This PR:

  • Removes global error enum by moving the variants to a BlockBuildingError enum
  • removes snafu dependency from everywhere except from the availability endpoint, as hotshot query service still uses it
  • moves error types into their relevant modules
  • use parse_duration and parse_size from espresso_types::utils

This PR does not remove snafu dependency completely, as we need to replace snafu with this_error in hotshot query service

@imabdulbasit imabdulbasit linked an issue Aug 6, 2024 that may be closed by this pull request
@imabdulbasit imabdulbasit changed the title remove snafu dep and global error enum Replace snafu with this_error and remove global error enum Aug 6, 2024
Copy link
Contributor

@tbro tbro left a comment

Choose a reason for hiding this comment

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

Looks good. I left a couple of comments.

u64::from(chain_config.max_block_size)
.try_into()
.map_err(|_| {
<Self as BlockPayload<SeqTypes>>::Error::Custom("cast u64 to usize".to_string())
Copy link
Contributor

Choose a reason for hiding this comment

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

I don't really see the point of Custom. If we know we have a potential error, why would we not add it to the enum?

On the other hand, I think this can only ever potentially fail on 32 bit architecture, so the failure case would be something like "too large max block size for architecture". But if that is the case, then user would never be able to build payloads, so maybe instead of returning that error we should panic w/ it...?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yeah, I agree. I'll remove custom and panic here

types/src/eth_signature_key.rs Outdated Show resolved Hide resolved
@imabdulbasit imabdulbasit enabled auto-merge August 7, 2024 13:01
@tbro
Copy link
Contributor

tbro commented Aug 7, 2024

I think this addresses #1539, and Closes #1681 and #1699. Is that right?

@imabdulbasit
Copy link
Contributor Author

I think this addresses #1539, and Closes #1681 and #1699. Is that right?

Yes

@tbro
Copy link
Contributor

tbro commented Aug 7, 2024

So it doesn't completely remove the snafu , as the query service is using it and we are extending the availability endpoint. However, I have replaced it with this_error everywhere else

Ok so #1539 will remain open for now

@imabdulbasit imabdulbasit merged commit e52425a into main Aug 7, 2024
15 checks passed
@imabdulbasit imabdulbasit deleted the ab/rm-global-err branch August 7, 2024 14:41
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.

Ensure error types are in correct impl Remove global Error
2 participants