SecretKey::secret_bytes() and Index<FullRante> for SecretKey contradict each other

[Kixunil]

So we have a method named secret_bytes instead of more intuitive serialize to alert the caller that the value is secret and needs to be handled with care but at the same time we have the [..] operator to make sure extracting the bytes purposely or accidentally is as easy as possible.

We should remove one of them and I vote for removing Index.

BTW I'm quite annoyed about the unintuitive secret_bytes which I can never remember how it's called. I do understand why though and don't want to change it. I think the most we can do right now is put an alias on it. (It won't help me but maybe someone else at least.)

[apoelstra]

Agreed, let's drop the indexing trait. I'm not certain what it's useful for.

As for secret_bytes I might suggest using to_byte_array_secret or something, which I think the rust compiler should suggest if somebody tries to_byte_array. (I don't particularly like serialize even though we use it elsewhere in this library because it has a connotation of allocating a vector. Or at least, doing something more than returning the internal serialization.)

But yes, we should alias it to all the obvious things to guess.

I would also be ok with just using to_byte_array and letting users use their own judgement. After all, we have a serde impl for this type which is also easy to use blithely. But I mildly prefer to have the scary name.

[Kixunil]

I'm not certain what it's useful for.

Me neither, but I can tell you where it's used: in bitcoin::crypto::keys a few times (twice I think).

I don't particularly like serialize even though we use it elsewhere in this library because it has a connotation of allocating a vector.

It doesn't to me, at most it has connotations with writing to a writer. But maybe I'm biased having rewritten a bunch of APIs to return arrays instead of Vec<u8>.

Conceptually I like secret_bytes but it's inconsistent with other uses for historical reasons. We have to_byte_array for hashes but I was thinking of renaming it to to_bytes (or just soft deprecating the old one).

I would also be ok with just using to_byte_array and letting users use their own judgement. After all, we have a serde impl for this type which is also easy to use blithely. But I mildly prefer to have the scary name.

That's pretty much what I was thinking too, except I forgot about serde which makes it make more sense which makes it stronger argument towards not doing anything different. We could remove the serde impl in favor of with but that seems too much even to me and some people would complain. ;)

[apoelstra]

but it's inconsistent with other uses for historical reasons. We have to_byte_array for hashes but I was thinking of renaming it to to_bytes (or just soft deprecating the old one).

to_byte_array has grown on me and because Rust's array support is still not as nice as its slice support, it helps to see that we're explicitly returning an array. Though sure, a soft-deprecation (or a "deprecation when bitcoin_pedantic cfg is turned on") would be fine by me.

In any case, I think that the accessors for SecretKey should be the same as those for hashes. Which for now I think means using to_byte_array and as_byte_array.

[Kixunil]

helps to see that we're explicitly returning an array

In the presence of other methods this is true. Though the only other thing we could "reasonably" return from that method is Vec<u8> or some kind of Box which we never do in hashes, so it should be clear at this point, I guess?

I'm thinking we should also remove all vec-returning methods that are not named to_vec in favor of array-returning ones to minimize confusion. People can still call .to_vec() on the arrays. And the question is if we even need to_vec but at least it non-confusingly allocates. (Soft) deprecating it still has the advantage that you don't allocate by accident because you've missed a newly added method during the upgrade.

[apoelstra]

Well, another point of confusion is that as_bytes looks like it returns &[u8] because that's pretty-much always what such a method would return. So if it returns a [u8; N] that's a point of confusion -- but then added to that, to_bytes should be the "owned version" of as_bytes, so people won't know what to expect.

Agreed though that throughout the crate ecosystem we should use to_vec for vectors. But I still think we should have to_byte_array where it makes sense, and (soft) deprecate to_bytes wherever it occurs.

I don't think "we never do that in hashes" is a useful API heuristic. Anyone familiar enough with the library to do that is familiar enough to know what the API looks like without looking.

[Kixunil]

another point of confusion is that as_bytes looks like it returns &[u8] because that's pretty-much always what such a method would return.

It shouldn't be a big deal in practice because it coerces to &[u8] so the only situation where things would be weird is if you pass it to a function expecting a trait that's implemented for &[u8] and isn't for &[u8; N]. Probably io::Read is an example.

I don't think "we never do that in hashes" is a useful API heuristic.

I'd hope that the crate being no-alloc would help here. But maybe people don't see such things.