paritytech / parity-common

Collection of crates used in Parity projects
https://www.paritytech.io/
Apache License 2.0
282 stars 213 forks source link

Add bounded-collections crate #708

Closed KiChjang closed 1 year ago

KiChjang commented 1 year ago

This new crate contains the bounded types that we see in Substrate, and traits that are used to support the implementation of such types.

Related to paritytech/polkadot#6572.

mrcnski commented 1 year ago

Looks great overall! My one question is why are we moving all these traits into bounded-collections? For some e.g. TryCollect it makes sense, because in Substrate they are only used in the context of bounded collections. But for others like ConstBool it doesn't seem right to me, because in Substrate they are used in other contexts too, just based on a quick rg.

KiChjang commented 1 year ago

@mrcnski I originally thought of putting the Const* types into primitive-types, but I read the crate description and it said "Primitive types shared between Substrate and Ethereum", and since the Const* types and Get traits aren't in Ethereum, I ultimately decided against it. Do you still think it's a better fit to move these types and traits there though? What other places do you have in mind instead?

mrcnski commented 1 year ago

@KiChjang Gotcha, I just think it would be odd to depend upon bounded-collections if I wanted to use e.g. ConstBool. I don't know the dependency stack well enough yet to offer an existing alternative. But maybe there could be another new crate here in parity-common called traits?

For example, here we use a bunch of these traits, but this crate doesn't use bounded collections, so it would be odd to have it depend on bounded-collections, IMO.

KiChjang commented 1 year ago

So, the name of the crate needs to be unique, as they will be published on crates.io, thus I can't just call it traits and be done with it. I've searched in Substrate and found out that primitive-types is depended upon by sp-core, sp-arithmetic and sp-runtime-interface.

What would likely happen next is that bounded-collections will be re-exported from sp-core, and for crates that are critical in memory usage, we'd just depend directly upon bounded-collections rather than going through sp-core.

mrcnski commented 1 year ago

Maybe instead of traits it could be primitive-traits? Or maybe keep them in this crate, but rename it from bounded-collections to common-types/core-types? Or maybe I'm just being OCD. 😛

It makes sense that these will be re-exported, which makes it not such a big deal, but still feels a little wrong (i.e. unintuitive and not very discoverable).

KiChjang commented 1 year ago

me gusta core-types, let's do so.

niklasad1 commented 1 year ago

I think core-types is too generic as we already got primitive_types the distinction between is blurry IMO (by crate names). I prefer bounded-collections as you did initially or something like substrate-types

I requested a review from @ordian he is the main maintainer of this repo :)

ordian commented 1 year ago

I do not quite get why this crate should live in parity-common and not in Substrate? It looks quite stand-alone and non of the workspace crates here depend on it (or is it going to change?).

The historical purpose of parity-common was to share code between substrate and ethereum client code bases and nowadays it's substrate and frontier/evm.

KiChjang commented 1 year ago

It looks quite stand-alone and non of the workspace crates here depend on it (or is it going to change?)

Yes, we're essentially pulling out some common types that are going to be used in Substrate and later on the XCM crate when it lives in its own repository, independent of both Polkadot and Substrate.