make Keyset Id's member variables public

[codingpeanut157]

Description

Aligns with other structures by exposing Id's member variables, allowing developers to create Ids that don't follow the CDK protocol. [1]

[1] https://github.com/cashubtc/nuts/blob/main/02.md#deriving-the-keyset-id

[ok300]

NACK.

This would allow callers to bypass all the From implementations for Id ( From<&Keys>, FromStr, from_bytes etc) when creating Ids, which is unnecessary and dangerous.

[codingpeanut157]

NACK.

This would allow callers to bypass all the From implementations for Id ( From<&Keys>, FromStr, from_bytes etc) when creating Ids, which is unnecessary and dangerous.

That was the point of this PR: allow others to implement their own keyset Id generator algorithms.
Regarding the potential danger of exposing internals of Id: I don't see any extra danger, as you expose from_bytes method which substantially allows users to put anything that starts with Version00 in the Id.
Moreover, exposing internals of other structures that use Id like MintKeysInfo or MintKeyset do not mitigate that same danger.
And besides that, exposing internals of structs like Proof or MintKeyPair is equally dangerous IMHO.
So why not exposing Id and opening up cdk to custom implementations of eCash protocol...

[crodas]

Maybe it is worth making it a trait or something with a default implementation, something more idiomatic and easy to extend.

Also, FYI, perhaps this should be part of the signatory, and the mintd just uses whatever is given to it by the signatory, as it may eventually run as its crate either in the same memory space or with an external service through grpc (#509) /cc @thesimplekid

[codingpeanut157]

as the codebase currently is, I reckon, converting Id into a trait is unfeasible: this small struct is used in so many places that converting all of them into generic code over a parameter impl<ID> [...] where ID: Id would be a bloodbath.

Side note: if ( and I hope it will) the signatory and key management subsystem will become an external service communicating with the mint via grpc, protecting the internals of the keyset Id and how it is built is going to become even less relevant for the security level of the mint itself

[thesimplekid]

I do agree with codingpeanut that this would end up being a pretty big change. However, we are going to need to something to support keysetv2 whether that be making it a trait or an enum like we do with token, if I remember correctly I started trying to do token as a trait and ended up with an enum forget exactly why.

Also, FYI, perhaps this should be part of the signatory

We still need the type in cashu as we want this crate to be an implementation of all the cashu types and can be used without the other crates.

[thesimplekid]

This would allow callers to bypass all the From implementations for Id ( From<&Keys>, FromStr, from_bytes etc) when creating Ids, which is unnecessary and dangerous.

I agree with this and don't think the fields should or need to be used directly, however most of the fields on other structs have pub fields, so I don't have an strong argument for drawing the line here and keeping this one private.

That being said I think attempting to use cdk for a non cashu protocol is going to inevitability lead to more changes being needed to cdk for it to work that won't be merged even if this is, as it is an implementation of the cashu protocol.

[codingpeanut157]

hello @thesimplekid
is there any chance this commit is going to land sooner or later?
Thanks

[ok300]

Guys, this is bikeshedding.

It brings no value to CDK or CDK callers, to expose these inner fields and allow custom keyset ID generation algorithms. The cashu NUTs specify exactly the algorithm for keyset ID generation. If there's a problem in the CDK algo, then a PR for that is more appropriate.

Dev time is limited, so I'd suggest we keep it simple and close this, to avoid unnecessary changes.

[thesimplekid]

I am going to close this. As @ok300 has pointed out, Keyset Id generation is specifically defined in the nuts, so there is no need for the inner to be public and the Id type should be used. It is true that currently CDK does have many pub types, so this could be seen as inconsistent; however, I think we should make more private instead of making this one public, this will ensure that fields and types are used as we expect. Where access to inner fields is needed, we can provide a read-only getter or a Deref.

Furthermore, we will need to add support for IDV2 so making this change will be almost immediately be overwritten by adding support for this.