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 params method to Network #2172

Merged
merged 2 commits into from Nov 18, 2023

Conversation

Kixunil
Copy link
Collaborator

@Kixunil Kixunil commented Nov 5, 2023

Writing network.params() is less annoying than Params::network(), so this adds it. Making it return a static could also improve performance.

Didn't do Params -> Network conversions because of #2173

@Kixunil Kixunil added enhancement minor API Change This PR should get a minor version bump API hole Important API missing - significantly degrades usability or idiomatic usage labels Nov 5, 2023
@apoelstra
Copy link
Member

Would suggest drafting this and discussing in #2169 or #2173 (pick one :)) to decide how we want these two types to relate.

@Kixunil
Copy link
Collaborator Author

Kixunil commented Nov 5, 2023

I don't think they are exclusive unless type Network = &'static Params; which I don't like much.

@apoelstra
Copy link
Member

What about struct Network<'a>(pub &'a Params)?

We definitely shouldn't make Network be an alias of &'static Params because that makes non-static Params second-class and kinda defeats the purpose of even having the structure.

@Kixunil
Copy link
Collaborator Author

Kixunil commented Nov 5, 2023

Putting a lifetime everywhere you had Network would be super annoying. I wouldn't be surprised if people rage quit. I'd consider doing so myself.

@apoelstra
Copy link
Member

Hmm, yeah, that's a very good point.

Writing `network.params()` is less annoying than `Params::network()`, so
this adds it. Making it return a static could also improve performance.
@Kixunil
Copy link
Collaborator Author

Kixunil commented Nov 5, 2023

Force push only fixes a typo in the commit message.

@Kixunil Kixunil changed the title Add params method do Network Add params method to Network Nov 5, 2023
@apoelstra
Copy link
Member

9282cc4 looks good to me. I wonder if we should use AsRef rather than From<X> for &'static Y though.

@Kixunil
Copy link
Collaborator Author

Kixunil commented Nov 5, 2023

It felt kinda weird to return a reference from AsRef that isn't stored in the value itself. I guess I will ask at Rust forum what people think.

@Kixunil
Copy link
Collaborator Author

Kixunil commented Nov 6, 2023

https://users.rust-lang.org/t/implementing-asref-for-types-not-storing-the-reference/102147

Now that I wrote it I'm very tempted by the boxes&co argument but still interested in other viewpoints. :)

Copy link
Member

@tcharding tcharding left a comment

Choose a reason for hiding this comment

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

ACK 9282cc4

@tcharding
Copy link
Member

FWIW, in my opinion just simple network.params() seems better than using AsRef or From.

Copy link
Member

@apoelstra apoelstra left a comment

Choose a reason for hiding this comment

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

ACK 9282cc4

@apoelstra apoelstra merged commit c12debf into rust-bitcoin:master Nov 18, 2023
29 checks passed
@Kixunil Kixunil deleted the network-params-method branch November 18, 2023 20:21
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
API hole Important API missing - significantly degrades usability or idiomatic usage enhancement minor API Change This PR should get a minor version bump
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants