-
Notifications
You must be signed in to change notification settings - Fork 96
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
delete(code): remove dead code from zebra-chain #5464
Conversation
Codecov Report
Additional details and impacted files@@ Coverage Diff @@
## main #5464 +/- ##
==========================================
+ Coverage 79.11% 79.12% +0.01%
==========================================
Files 308 308
Lines 39324 39350 +26
==========================================
+ Hits 31111 31137 +26
Misses 8213 8213 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think we can also remove the SpendingKey
and derived keys from sprout, sapling, and orchard. As far as I can tell, they are only used in tests.
We also want to remove all the code that is impacted by these known bugs:
- Stop assuming testnet when parsing keys and addresses #4691
- the key parsing panics we found in PR lint(clippy): add
unwrap_used
lint #4732 - anything else?
Then I guess we can close those tickets? Maybe with a note that the buggy code was removed?
https://github.com/ZcashFoundation/zebra/actions/runs/3308216160/jobs/5461039042 @gustavovalverde any idea why we occasionally lose GCP instances like this? |
@Mergifyio update |
☑️ Nothing to do
|
Are any of these issues happening because of cryptographic implementations in zebra-chain that are unused? If not, they would be out of scope of #5446 which is what this PR is trying to fix |
The Do we want to remove the structs, implementations and tests alltogether ? do we really need to do this ? |
Yes, and I think there might be another 1-2 bugs in unused cryptographic code. I did a quick check of closed tickets and I couldn't find them. Maybe @dconnolly can help us find the buggy code we want to remove? |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This seems fine for now, let's work out what other code we need to delete in another PR.
Those aren't test-only markers for the struct. The
I think we might be able to delete all 6 shielded keys and addresses modules, because we don't implement a full node wallet yet. |
@Mergifyio update |
✅ Branch has been successfully updated |
https://github.com/ZcashFoundation/zebra/actions/runs/3316596195/jobs/5478977357#step:6:106:
Same problem here https://github.com/ZcashFoundation/zebra/actions/runs/3316596195/jobs/5478977357:
|
Let's continue this conversation on PR #5476? |
Co-authored-by: mergify[bot] <37929162+mergify[bot]@users.noreply.github.com>
Motivation
There is some dead code we can remove from zebra-chain "safely" as part of #5446 , however i am not sure if there is more to delete to close the ticket.
Solution
Remove unused code from zebra-chain.
Review
Reviewer Checklist