Collin/chain name enum

[cbrit]

No description provided.

[philipjames44]

Not necessarily requesting changes, just some points to discuss potentially.

[tony-iqlusion]

A slightly different way to approach this would be to have a type which provides a const fn constructor which validates the chain ID, which would allow you to have a single type where you provide a number of constants for "well known" chains, but anyone else could bind their own constants for chains not in the registry. Something like this:

impl ChainId {
    const AGORIC: ChainId = ChainId::new("agoric");
    const AKASH: ChainId = ChainId::new("akash");
    const ARKH: ChainId = ChainId::new("arkh");
    [...]
}

Using inherent constants the syntax is nearly identical to an enum: ChainId::AGORIC vs Chains::Agoric in this PR. It gives you convenience of a registry but in a way that's extensible so anyone else can define their own chain ID. And if any chain IDs are added to the registry later, the value is compatible with what a user might define themselves.

https://github.com/iqlusioninc/crates/blob/main/bip32/src/prefix.rs

I'd otherwise recommend using tendermint::chain::Id for this, but it lacks the sort of const fn support I just described which would allow you to create a registry of chain ID constants.

I can open a PR upstream for that if this sounds interesting.

[philipjames44]

A slightly different way to approach this would be to have a type which provides a const fn constructor which validates the chain ID, which would allow you to have a single type where you provide a number of constants for "well known" chains, but anyone else could bind their own constants for chains not in the registry. Something like this:

impl ChainId {
    const AGORIC: ChainId = ChainId::new("agoric");
    const AKASH: ChainId = ChainId::new("akash");
    const ARKH: ChainId = ChainId::new("arkh");
    [...]
}

Using inherent constants the syntax is nearly identical to an enum: ChainId::AGORIC vs Chains::Agoric in this PR. It gives you convenience of a registry but in a way that's extensible so anyone else can define their own chain ID. And if any chain IDs are added to the registry later, the value is compatible with what a user might define themselves.

https://github.com/iqlusioninc/crates/blob/main/bip32/src/prefix.rs

I'd otherwise recommend using tendermint::chain::Id for this, but it lacks the sort of const fn support I just described which would allow you to create a registry of chain ID constants.

I can open a PR upstream for that if this sounds interesting.

'+ 1' on the const fn's

I think that sounds pretty good.

[tony-iqlusion]

I opened a PR to add a const fn constructor to tendermint::chain::Id: informalsystems/tendermint-rs#1105

[adizere]

A slightly different way to approach this would be to have a type which provides a const fn constructor which validates the chain ID, which would allow you to have a single type where you provide a number of constants for "well known" chains, but anyone else could bind their own constants for chains not in the registry.

+1, neat solution. I'd expect chains to change relatively fast, cadence of say ~1/month, so I can imagine binding new constants while 99% of time using the well-known definitions.

If we can rely on tendermint::chain::Id that would be even better.

[tony-iqlusion]

Why not just call this ChainId to match the tendermint name?

Also note that prefixing a type with the module it's contained in is non-idiomatic:

https://rust-lang.github.io/rfcs/0356-no-module-prefixes.html

I would suggest just re-exporting tendermint::chain::Id here:

Suggested change

	pub type ChainName = tendermint::chain::Id;
	pub use tendermint::chain::Id;

[cbrit]

My concern is that referring to the reference name as an ID will be conflating the chain-id in the registry with the chain name, thus the type alias and rename to make it match the field in the registry chains.json, rather than to prefix it with the module name. That's just a coincidence. Does that change your opinion?