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

change(chain): Remove Copy trait impl from Network #8354

Merged

Conversation

idky137
Copy link
Contributor

@idky137 idky137 commented Mar 15, 2024

Motivation

This PR continues work towards implementing Regtest mode in Zebrad.

Removing Derive(copy) from Network is required for adding a NetworkParameters struct to Testnet. This breaks a lot of functions that take Network as an input. This PR changes these functions to take &Network where possible and clones Network where either not possible or requires significant refactoring to avoid.

Prerequisite of #7968 and part 3 of #7845.

PR Author Checklist

Check before marking the PR as ready for review:

  • Will the PR name make sense to users?
  • Does the PR have a priority label?
  • Have you added or updated tests?
  • Is the documentation up to date?
For significant changes:
  • Is there a summary in the CHANGELOG?
  • Can these changes be split into multiple PRs?

If a checkbox isn't relevant to the PR, mark it as done.

Solution

Removes Derive(Copy) from Network and any structs that contain Network.
Changes functions that take Network to take &Network where possible and clones network where not.

Testing

Reviewer Checklist

Check before approving the PR:

  • Does the PR scope match the ticket?
  • Are there enough tests to make sure it works? Do the tests cover the PR motivation?
  • Are all the PR blockers dealt with?
    PR blockers can be dealt with in new tickets or PRs.

And check the PR Author checklist is complete.

@idky137
Copy link
Contributor Author

idky137 commented Mar 18, 2024

conversion now complete. all tests pass (workspace wide). I'll go through the review now and make the changes requested. Ye i have been worried that its been getting big but now its complete maybe it will be possible?

@idky137
Copy link
Contributor Author

idky137 commented Mar 19, 2024

all clippy warnings fixed and changes from review applied

@idky137 idky137 marked this pull request as ready for review March 19, 2024 14:11
@idky137 idky137 requested review from a team as code owners March 19, 2024 14:11
@idky137 idky137 requested review from upbqdn and removed request for a team March 19, 2024 14:11
@idky137
Copy link
Contributor Author

idky137 commented Mar 19, 2024

sorry I just saw your comment about linking the commit that addresses a comment. I'll make sure to include this from now on.

@arya2
Copy link
Contributor

arya2 commented Mar 19, 2024

@idky137
Copy link
Contributor Author

idky137 commented Mar 19, 2024

fixed build error (in commit: b09707c)

@arya2 arya2 changed the title Remove derive copy from network change(chain): Remove derive copy from network Mar 19, 2024
Copy link
Contributor

@arya2 arya2 left a comment

Choose a reason for hiding this comment

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

Let's merge it!

Thank you for the extensive changes.

@arya2
Copy link
Contributor

arya2 commented Mar 19, 2024

Once it stabilizes, Ref::leak may be useful for similar cases in the future, but for now, I'm glad we were able to avoid OnceCell or being constrained to hard-coded constants. Thanks again.

@arya2 arya2 added A-consensus Area: Consensus rule updates C-enhancement Category: This is an improvement A-network Area: Network protocol updates or fixes NU-6 Network Upgrade: NU6 specific tasks P-Medium ⚡ and removed extra-reviews This PR needs at least 2 reviews to merge labels Mar 19, 2024
@arya2 arya2 changed the title change(chain): Remove derive copy from network change(chain): Remove Copy trait impl from Network Mar 19, 2024
@mergify mergify bot merged commit 4579722 into ZcashFoundation:main Mar 19, 2024
102 checks passed
@arya2 arya2 added this to the Regtest Network support milestone Mar 20, 2024
@mpguerra mpguerra linked an issue May 21, 2024 that may be closed by this pull request
11 tasks
@mpguerra mpguerra removed a link to an issue May 21, 2024
11 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A-consensus Area: Consensus rule updates A-network Area: Network protocol updates or fixes C-enhancement Category: This is an improvement NU-6 Network Upgrade: NU6 specific tasks P-Medium ⚡
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants