Consider not deriving serde impls from units types

[tcharding]

Types in units are thin wrappers around integer values. If users of these types want to put them in structs they can, and possibly should, stipulate how they want them serialized (eg, amount could be as sats or as BTC).

Shall we remove serde::Deserialize and serde::Serialize impls from units types? Note this does not mean remove the serde module from Amount, just remove #[cfg_attr(feature = "serde", derive(Serialize, Deserialize))] from Amount (and from all other units types).

[apoelstra]

I don't understand. Right now we have e.g. the amount::serde module which lets them choose between sats and BTC. We should definitely keep that.

For the locktime types things are less clear (should we have serde on Height/Time or just on LockTime).

[tcharding]

You were too quick. I've updated the issue

[sanket1729]

Having an implementation is definitely valuable for auto-derive users. I can imagine something like

struct GetTransactionInfo {
    pub reciever: Address, 
    pub fee: Amount,
    pub sent_amount: Amount,
}

requiring an auto-derive.

[tcharding]

We have that usecase covered already, one can do:

struct GetTransactionInfo {
    ...
    #[serde(with = "bitcoin::amount::serde::as_sat")]
    pub fee: Amount,
    ...
}

I haven't thought it through for other unit types but we want to provide some way to be able to derive them too.

[tcharding]

I just had a play and if we remove the serde imples it is annoying to not be able to do

#[derive(Debug, Serialize, Deserialize)]
struct Foo {
    height: absolute::Height,
    time: absolute::Time,
}

And it is surprising that

#[derive(Debug, Serialize, Deserialize)]
struct Bar {
    lt: absolute::LockTime
}

is fine but using Height and Time is not (if we remove the impls).

Do we gain anything by not allowing Foo to be defined like that? Seems to me like we will just be annoying users.

[sanket1729]

Amount is probably the most important one. If we have that covered, I am okay dropping the serde requirement from units. Users dealing with Height and Time are advanced users that can do their own serde.

[apoelstra]

Agreed. I do not think we should change Amount. We spent forever implementing it, many people are using it (including me) and we have covered the multiple natural ways to implement it.

Meanwhile Height and Time have no natural ways to implement serde because they basically just exist to compensate for Rusts' lack of privacy on enum internals, and their serde implementations are confusingly inconsistent with that for LockTime.

I'm fine keeping them, but they are a weird bit of API surface that if we commit to, we're stuck with forever.

You were too quick. I've updated the issue

It still seems to imply that we should get rid of serde entirely in the units crate.

[15IITian]

But It is possible that some users can just use Height struct for simply representing the height of a block -> so will require its serde implementation.

[tcharding]

It still seems to imply that we should get rid of serde entirely in the units crate.

I re-edited the description, third time lucky.

[tcharding]

But It is possible that some users can just use Height struct for simply representing the height of a block -> so will require its serde implementation.

I agree, absolute::Height seems totally likely to be put into a struct that wants to derive serde traits.

[tcharding]

Closing, this does not seem to be going anywhere. We can re-visit this at a later time if needed.

[tcharding]

Note at this stage we do have serde traits implemented and the bug that started this conversation is fixed by: #2582