search

bitcoindevkit / bitcoin-ffi

Other
12 stars 7 forks source link

Rename `Script` to `ScriptBuf` #18

Open tnull opened 2 weeks ago

tnull commented 2 weeks ago

Previously, we exposed bitcoin::ScriptBuf as Script which is pretty confusing given bitcoin::Script exists.

Here, we rename Script to ScriptBuf to align the type names with rust-bitcoin.

IMO, we should stick as close as possible to the rust-bitcoin API to avoid confusion and naming collisions in the future.

reez commented 2 weeks ago

ACK 142ea5dd209c982c092110bea521f3f8924ffc6a

Sounds good to me.

Davidson-Souza commented 1 week ago

I think it makes sense to call it Script because we won't expose the pair Script/ScriptBuf. And the name only makes sense for people using rust-bitcoin that know what they are. But I don't have any strong preferences.

thunderbiscuit commented 1 week ago

Sorry I saw this last week and forgot to get back to it.

I see this one as balancing two ideas: (a) our usual rule of "don't name types differently than their Rust counterparts", and (b) providing a type that's appropriately named for the target languages (is the "spirit of the law" vs. "letter of the law" a good analogy here?)

I'm not 100% against using ScriptBuf, but I do think it has downsides:

  1. It surfaces "rusty" stuff that's not useful nor available in the target languages (at least for now as we expose it). The concept of the buffer and the fact that one type is owned and the other a reference for optimization purposes for example.
  2. If it's the only script-like-type, then you're just left wondering why it's not named better, aka Script. The answer "well that's what it's named in rust-bitcoin" is unsatisfying IMO.

The questions I have echo what @Davidson-Souza asked: did you intend to offer both types to the target languages? Will there be advantages for them?

Unless we have a clear use case for having both, I think in this very specific instance, naming the type Script keeps it closer to what it is in spirit than naming it correctly. Just my 2 sats.