Allow the host implementation to specify the generateIdentifier function for connection and channel identifiers

[hu55a1n1]

Summary of Bug

We currently define trait methods like ConnectionReader::connection_counter() and use them to generate the connection identifier ->
https://github.com/informalsystems/ibc-rs/blob/57b8af939eedab3a5ad55d7045a108ed0dfab1f6/modules/src/core/ics03_connection/handler/conn_open_init.rs#L42-L44

This restricts host implementations' ability to generate custom identifiers and is a deviation from the spec.

Version

v0.16.0

Acceptance Criteria

Either provide a way for host impls to specify their own generateIdentifier or document this as a divergence (in divergence from the ICS).

---

For Admin Use

• Not duplicate issue
• Appropriate labels applied
• Appropriate milestone (priority) applied
• Appropriate contributors tagged
• Contributor assigned/self-assigned

[hu55a1n1]

Related to #39

[Farhad-Shabani]

Closer Look

Having a closer look at the related part of the specs:
Following clauses ALLOW users to define custom identifiers:

ICS24: Identifiers are not intended to be valuable resources — to prevent name squatting, minimum length requirements or pseudorandom generation MAY be implemented, but particular restrictions are not imposed by this specification.

ICS02: Clients are stored under a unique Identifier prefix. This ICS does not require that client identifiers be generated in a particular manner, only that they be unique. However, it is possible to restrict the space of Identifiers if required.

In addition, looking at the CreateClient process in ICS02, users would pass their own identifier, while neither ibc-rs nor ibc-go already implement this capability.

However, ICS03 and ICS04 RESTRICTS and a bit bypass the above definitions. Identifiers must somehow contain an integer and be incrementable according to the followings:

ICS03: Chains MUST implement a function generateIdentifier which chooses an identifier, e.g. by incrementing a counter

ICS04: Chains MUST implement a function generateIdentifier which chooses an identifier, e.g. by incrementing a counter

And also, there is no space for users to pick their own identifier and pass it to the connOpenInit and chanOpenInit processes. it is forged using the generateIdentifier() function.

Next?

If we wanted to add an identifier field to the client creation and accept users’ choice, it requires an update on protos and would be a breaking one. Maybe this can be left (shouldn’t be an immediate need!) in the “divergence from the ICS” list.
But, as for the channel and connection identifiers, it seems we are within the specifications. Hw, I'd like to apply some revisions to how we handle identifier creation and keep the implementation more similar to IBC-GO. You may agree if?

[plafer]

But, as for the channel and connection identifiers, it seems we are within the specifications.

Not exactly; the spec mandates that the host returns an identifier, and gives an example of how the host could implement that function (by incrementing a counter). So technically speaking, we are against the spec here since we mandate the identifier-generation process (i.e. we mandate that it be of the form "{client-type}-{counter}").

However I don't think we need change it, nor even declare it as a divergence from the spec, since the set of identifiers we generate is a subset of the allowed set of identifiers (so identifier validation on counterparty chains will never fail). An added benefit of the current approach is that it prevents chains from generating the same channel identifier twice which would break ICS-20 (this part is currently under-specified IMO).

I think we should just leave things as is, and only change it if a user would like the ability to name their identifiers differently. WDYT?

[Farhad-Shabani]

Hmm, one thing I find out from your comment... I looked at the issue more from the users' perspective (who operate the cmds), not the ibc-rs implementors.
Btw, It doesn't look like a deviation from the specs. It makes sense, imo.
So, can we treat this issue as an improvement rather than a bug?
Is there anything that needs to be done on this right now?

[plafer]

Nope, if you're okay with it, I would just close it.