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

Add local network to network enum #1276

Closed

Conversation

Oscar-Pepper
Copy link
Contributor

Adds LocalNetwork variant to Network enum and adds methods for LocalNetwork for defining common network upgrade activation height patterns.

This allows production code of consumers to be tested in a regtest environment by easily switching out network parameters without needing to create their own version of the Network enum with a regtest variant.

Copy link
Contributor

@nuttycom nuttycom left a comment

Choose a reason for hiding this comment

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

NACK. This is not how the Network type is intended to be used; in circumstances where a local network is necessary, you should use a bespoke type that implements the Parameters trait. See how LocalNetwork is used in https://github.com/zcash/librustzcash/blob/main/zcash_client_sqlite/src/testing.rs for an example.

If there are places where we are depending upon the Network type that we shouldn't be, I'm happy to change those. There may be places where instead we should be taking a NetworkType parameter, rather than a Network (where we only need the network constants), otherwise everything should be a polymorphic type having the consensus::Parameters bound.

@Oscar-Pepper
Copy link
Contributor Author

Oscar-Pepper commented Mar 18, 2024

NACK. This is not how the Network type is intended to be used; in circumstances where a local network is necessary, you should use a bespoke type that implements the Parameters trait. See how LocalNetwork is used in https://github.com/zcash/librustzcash/blob/main/zcash_client_sqlite/src/testing.rs for an example.

If there are places where we are depending upon the Network type that we shouldn't be, I'm happy to change those. There may be places where instead we should be taking a NetworkType parameter, rather than a Network (where we only need the network constants), otherwise everything should be a polymorphic type having the consensus::Parameters bound.

Ok thanks for the info, our wallet struct contains an enum similar to NetworkType instead of a generic like WalletDb in testing.rs. It makes sense for us to eventually restructure this so we can be more aligned with librustzcash but for now I will just send the constructors for LocalNetwork so we can use it in place of our own LocalNetwork struct which is essentially the same.

@Oscar-Pepper
Copy link
Contributor Author

closed for #1281

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.

2 participants