ICS24 Spec inconsistencies and improvements

[ebuchman]

Surfaced from Informal Systems IBC Audit

Summary of Bug

The path space laid out in the ICS24 spec doesn't fully match the ics24 code. It also might be helpful if each path in the spec had a name (which could correspond to function names in the code), and if the code was a bit more clearly organized for direct comparison with the spec.

Presumably things to fix in the spec:

• clientType from spec is missing in code (it's part of ClientState, so doesn't have its own path in store)
• commitments/ports/{identifier}/channels/{identifier}/packets/{sequence} in the spec is commitments/ports/{identifier}/channels/{identifier}/sequences/{sequence} in the code (and same for the receipts/ and acks/ paths.
• ChannelCapabilityPath in the code is missing from the spec

There's also some redundancy in the code with prefix functions, like PacketAcknowledgementPath could use PacketAcknowledgementPrefixPath and FullConsensusStatePath could use ConsensusStatePath.

For Admin Use

• Not duplicate issue
• Appropriate labels applied
• Appropriate contributors tagged
• Contributor assigned/self-assigned

[ebuchman]

On a related ICS24 minor code dedup note, PathValidator is just a NewPathValidator with a noOp idValidator arg. Ie it could be written:

func PathValidator(path string) error {
	f := NewPathValidator(func(path string) error {
		return nil
	})
	return f(path)
}

Instead of duplicating the code in NewPathValidator

[colin-axner]

It also might be helpful if each path in the spec had a name (which could correspond to function names in the code), and if the code was a bit more clearly organized for direct comparison with the spec.

This sounds good to me. We could split our ...Key vs ...Path functions into different files as well and presumably one of them would match the spec which would make verification easier.

* clientType from spec is missing in code (it's part of ClientState, so doesn't have its own path in store)

We can add documentation for this.

* `commitments/ports/{identifier}/channels/{identifier}/packets/{sequence}` in the spec is `commitments/ports/{identifier}/channels/{identifier}/sequences/{sequence}` in the code (and same for the `receipts/` and `acks/` paths.

This needs to be updated in the spec, cosmos/cosmos-sdk#7873 we changed it recently to avoid redundancy

* `ChannelCapabilityPath` in the code is missing from the spec

We don't prove capabilities on counterparty chains so I don't know if we need to add it? I think we just use this path as the name for the capability in x/capability

cc @cwgoes

[cwgoes]

We don't prove capabilities on counterparty chains so I don't know if we need to add it? I think we just use this path as the name for the capability in x/capability

The spec could still include it, although the access-control-mechanism is not necessarily shared by all chains (it's not a "requirement" of implementing the IBC protocol, just an optional component).

[crodriguezvega]

clientType from spec is missing in code (it's part of ClientState, so doesn't have its own path in store)

The ClientType path was removed in the specs in this PR.

commitments/ports/{identifier}/channels/{identifier}/packets/{sequence} in the spec is commitments/ports/{identifier}/channels/{identifier}/sequences/{sequence} in the code (and same for the receipts/ and acks/ paths.

The paths now match between spec and implementation.

ChannelCapabilityPath in the code is missing from the spec

The channelCapabilityPath exists in the specs, although it doesn't match the path used in ibc-go (but it doesn't matter since it doesn't need to be proved by the counterparty).

PathValidator is just a NewPathValidator with a noOp idValidator arg

I can't find the PathValidator function, so I guess this doesn't apply anymore.

So the only thing left would be splitting the ...Key and ...Path functions into different files. I will open a PR for this; if the team doesn't agree with the change, I will the just close the issue.