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

feat(clippy): Add fallible_impl_from lint #4609

Merged
merged 2 commits into from
Jun 20, 2022
Merged

feat(clippy): Add fallible_impl_from lint #4609

merged 2 commits into from
Jun 20, 2022

Conversation

oxarbitrage
Copy link
Contributor

Motivation

In #2297 we want to add some panic lints to zebra. This is part 1 of 3 where the fallible_impl_from is added.

Solution

Add the lint and fix the code where needed.

Review

Please review carefully, there are error messages that might be changed. I think @dconnolly wrote most of the changed code in orchard and sapling keys but anyone that can take a look is welcome.

Reviewer Checklist

  • Current test pass
  • Errors strings added are correct

Follow Up Work

We want to add 2 more related lints but i think it was too much to add them in a single PR so i splitted into 3.

@oxarbitrage oxarbitrage requested review from a team as code owners June 14, 2022 13:54
@oxarbitrage oxarbitrage requested review from dconnolly and upbqdn and removed request for a team June 14, 2022 13:54
@codecov
Copy link

codecov bot commented Jun 14, 2022

Codecov Report

Merging #4609 (4694c0b) into main (e5c9208) will decrease coverage by 0.14%.
The diff coverage is 66.66%.

@@            Coverage Diff             @@
##             main    #4609      +/-   ##
==========================================
- Coverage   79.07%   78.92%   -0.15%     
==========================================
  Files         304      304              
  Lines       37418    37421       +3     
==========================================
- Hits        29589    29536      -53     
- Misses       7829     7885      +56     

Copy link
Contributor

@teor2345 teor2345 left a comment

Choose a reason for hiding this comment

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

I'd like to check the changes to the network protocol, everything else is optional.

I did not review the cryptography in detail.

zebra-network/src/protocol/external/message.rs Outdated Show resolved Hide resolved
zebra-chain/src/orchard/keys.rs Outdated Show resolved Hide resolved
@teor2345
Copy link
Contributor

I'm updating this PR, so I can see if the CI failures are from an old main branch, or if they are new in this PR.

@teor2345
Copy link
Contributor

@Mergifyio update

@mergify
Copy link
Contributor

mergify bot commented Jun 15, 2022

update

✅ Branch has been successfully updated

zebra-chain/src/orchard/keys.rs Show resolved Hide resolved
@oxarbitrage
Copy link
Contributor Author

Force pushed, this is now just c0c30ed

Copy link
Contributor

@teor2345 teor2345 left a comment

Choose a reason for hiding this comment

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

Thanks, looks good!

@teor2345
Copy link
Contributor

@Mergifyio update

@teor2345 teor2345 dismissed conradoplg’s stale review June 20, 2022 01:41

Changes were done

@mergify
Copy link
Contributor

mergify bot commented Jun 20, 2022

update

✅ Branch has been successfully updated

mergify bot added a commit that referenced this pull request Jun 20, 2022
@mergify mergify bot merged commit b7536c7 into main Jun 20, 2022
@mergify mergify bot deleted the issue2297 branch June 20, 2022 03:57
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.

4 participants