-
Notifications
You must be signed in to change notification settings - Fork 47
Add wrapper classes for various Taproot objects #97
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
Conversation
sstone
left a comment
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.
Adding a LocalNonce class would be very useful, but I'm not sure about creating wrappers for taproot tweaks, or taproot script trees: we end up duplicating logic, and the size check in the new IndividualNonce scala class feels wrong.
Scala interop issues are almost always caused by methods that use collections/Etiher ... and for which implicit conversions won't work, but these should already be addressed by scalacompat's Musig2 methods ? For example, are our kotlin taproot script tree/leaf classes hard to use from Scala (I don't think so except they require using new) ?
Also, I'm wondering whether we should just add the scalacompat package to eclair now ?
I agree they're not a game changer for usability in scala, but it is conceptually cleaner to offer APIs that only accept scala classes as arguments. And it is particularly useful for I agree that we shouldn't duplicate non-trivial logic, but the only logic that is duplicated here is DFS in a binary tree, which is a trivial two-liner: it was more concise than wrapping the
I don't understand why you think the size check feel wrong, can you explain? We have the same size check in
Why would we add this to |
Ok it makes sense.
because we're exposing
Because eclair is most probably the only project using it now that |
Yeah, that's why it's necessary anyway because in
But if we do that, we'd need to move the whole I think we shouldn't do that right now, and we should reconsider it when we start working on a next batch on "large" changes to |
Yes we would have to bring in the whole |
We add wrapper classes to simplify usability from Scala for:
We also update the library version to include those changes in a minor release.