Skip to content

Conversation

@t-bast
Copy link
Member

@t-bast t-bast commented Sep 25, 2025

We add wrapper classes to simplify usability from Scala for:

  • musig2 nonces
  • taproot script trees
  • schnorr signature tweaks
  • taproot signature tweaks

We also update the library version to include those changes in a minor release.

@t-bast t-bast requested a review from sstone September 25, 2025 14:02
@t-bast t-bast marked this pull request as ready for review September 25, 2025 14:02
Copy link
Member

@sstone sstone left a 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 ?

@t-bast
Copy link
Member Author

t-bast commented Sep 29, 2025

I'm not sure about creating wrappers for taproot tweaks, or taproot script trees: we end up duplicating logic

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 sealed trait: for example, if we had to pattern match on SchnorrTweak inside eclair, without this wrapper we couldn't have complete pattern matching, we would always need a case _ => ??? // cannot happen clause, which would be very ugly if used in many places. By exposing a scala wrapper that is a real sealed trait we remove this issue entirely for consumers of bitcoin-lib (and only need it once in the converters, which is hidden from eclair).

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 bitcoin-kmp methods (which required pattern matching on kmp classes), so IMO this is fine.

the size check in the new IndividualNonce scala class feels wrong.

I don't understand why you think the size check feel wrong, can you explain? We have the same size check in bitcoin-kmp, why should it be different?

Also, I'm wondering whether we should just add the scalacompat package to eclair now ?

Why would we add this to eclair? This is useful for anyone using bitcoin-lib from Scala, not just eclair, isn't it?

@sstone
Copy link
Member

sstone commented Sep 29, 2025

I'm not sure about creating wrappers for taproot tweaks, or taproot script trees: we end up duplicating logic

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 sealed trait: for example, if we had to pattern match on SchnorrTweak inside eclair, without this wrapper we couldn't have complete pattern matching, we would always need a case _ => ??? // cannot happen clause, which would be very ugly if used in many places. By exposing a scala wrapper that is a real sealed trait we remove this issue entirely for consumers of bitcoin-lib (and only need it once in the converters, which is hidden from eclair).

Ok it makes sense.

the size check in the new IndividualNonce scala class feels wrong.

I don't understand why you think the size check feel wrong, can you explain? We have the same size check in bitcoin-kmp, why should it be different?

because we're exposing Secp256k1.MUSIG2_PUBLIC_NONCE_SIZE. But using an inner class as for SecretNonce would be clumsy, and we need this for serialisation purposes anyway.

Also, I'm wondering whether we should just add the scalacompat package to eclair now ?

Why would we add this to eclair? This is useful for anyone using bitcoin-lib from Scala, not just eclair, isn't it?

Because eclair is most probably the only project using it now that eclair-mobile has been discontinued, so maybe moving it into eclair would make it easier to spot and fix scala compatibility issues ?

@t-bast
Copy link
Member Author

t-bast commented Sep 29, 2025

we need this for serialisation purposes anyway

Yeah, that's why it's necessary anyway because in eclair we serialize IndividualNonce and we thus need to know the number of bytes to read...

Because eclair is most probably the only project using it now that eclair-mobile has been discontinued, so maybe moving it into eclair would make it easier to spot and fix scala compatibility issues ?

But if we do that, we'd need to move the whole bitcoin-lib project inside eclair, because bitcoin-lib actually depends on scalacompat in many places? If we do that, we would completely delete the bitcoin-lib separate project? I'm honestly not sure it's worth it, it's still potentially useful to have a separate Scala bitcoin library. We've been updating it a lot recently which has been quite cumbersome, but that's because we're working on using a new segwit version (taproot) which is something that happens once every few years at most, so it shouldn't be much of a hassle in the next couple of years?

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 bitcoin-kmp / bitcoin-lib?

@sstone
Copy link
Member

sstone commented Sep 29, 2025

Because eclair is most probably the only project using it now that eclair-mobile has been discontinued, so maybe moving it into eclair would make it easier to spot and fix scala compatibility issues ?

But if we do that, we'd need to move the whole bitcoin-lib project inside eclair, because bitcoin-lib actually depends on scalacompat in many places? If we do that, we would completely delete the bitcoin-lib separate project? I'm honestly not sure it's worth it, it's still potentially useful to have a separate Scala bitcoin library. We've been updating it a lot recently which has been quite cumbersome, but that's because we're working on using a new segwit version (taproot) which is something that happens once every few years at most, so it shouldn't be much of a hassle in the next couple of years?

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 bitcoin-kmp / bitcoin-lib?

Yes we would have to bring in the whole scalacompat package, it can wait and maintaining bitcoin-lib is not very expensive but I think that it would make things a bit easier.

@t-bast t-bast merged commit 7514347 into master Sep 29, 2025
1 check passed
@t-bast t-bast deleted the taproot-wrapper-classes branch September 29, 2025 12:33
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.

3 participants