Add missing `ClientType`, `ClientId` validation checks #639

Farhad-Shabani · 2023-04-18T02:57:55Z

Closes: #621

Description

Considering that the create_client handler always adds a counter to the ClientId. This PR uncovered some of our tests were using a wrong ClientId
- Worth noting the spec does not force such a format with counter. Perhaps if someone ask for more generic identifiers later, we should refactor the constructor?!
The ClientType serves being a part of the ClientId (prefix) and the purpose is to construct client identifiers. So, it has been relocated under the ICS24 section. This move will make more sense if you review the changes in the PR, including the associated errors and how it interacts with ClientId. Also, should be noted we already don’t have any ClientType in the spec.
Since ClientId and ClientType are tightly related types, this PR covers missing checks for ClientId too, ensuring we cover all validation checks needed.
IBC-go has more restrictions on the format of ClientId, which seems to be required by SDKs. My goal was to impose less checks allowing for a wider range of identifiers

Note: Similarly, ConnectionId and ChannelId checks will be taken care after the completion of this PR

PR author checklist:

Added changelog entry, using unclog.
Added tests.
Linked to GitHub issue.
Updated code comments and documentation (e.g., docs/).
Tagged one reviewer who will be the one responsible for shepherding this PR.

Reviewer checklist:

Reviewed Files changed in the GitHub PR explorer.
Manually tested (in case integration/unit/mock tests are absent).

codecov · 2023-04-18T03:03:14Z

Codecov Report

Patch coverage: 88.09% and project coverage change: +0.02 🎉

Comparison is base (ee1dbce) 73.07% compared to head (dd42949) 73.09%.

Additional details and impacted files

@@            Coverage Diff             @@
##             main     #639      +/-   ##
==========================================
+ Coverage   73.07%   73.09%   +0.02%     
==========================================
  Files         126      126              
  Lines       15678    15665      -13     
==========================================
- Hits        11456    11450       -6     
+ Misses       4222     4215       -7

Impacted Files	Coverage Δ
crates/ibc/src/core/ics24_host/error.rs	`0.00% <ø> (ø)`
crates/ibc/src/core/ics02_client/client_type.rs	`31.81% <42.85%> (-9.36%)`	⬇️
crates/ibc/src/core/ics24_host/validate.rs	`98.83% <96.55%> (-1.17%)`	⬇️
crates/ibc/src/clients/ics07_tendermint/mod.rs	`100.00% <100.00%> (ø)`
crates/ibc/src/core/ics02_client/events.rs	`70.90% <100.00%> (ø)`
crates/ibc/src/core/ics03_connection/events.rs	`70.62% <100.00%> (ø)`
crates/ibc/src/core/ics24_host/identifier.rs	`57.07% <100.00%> (+0.19%)`	⬆️
crates/ibc/src/mock/client_state.rs	`90.22% <100.00%> (ø)`

... and 29 files with indirect coverage changes

☔ View full report in Codecov by Sentry.
📢 Do you have feedback about the report comment? Let us know in this issue.

…ecks

plafer · 2023-04-24T15:40:55Z

In #642 we removed store_client_type(). Is this PR still relevant?

Farhad-Shabani · 2023-04-24T16:53:36Z

In #642 we removed store_client_type(). Is this PR still relevant?

Yes. Actually, we still have to keep the ClientType and address any related issues, since it's part of our client identifier constructor. The store_client_type not needed anymore because of the recent spec change, though.

crates/ibc/src/core/ics24_host/identifier.rs

plafer · 2023-05-01T14:17:17Z

crates/ibc/src/core/ics24_host/validate.rs

+    let last_index = split_id.len() - 1;
+    let prefix = split_id[..last_index].join("-");
+
+    validate_prefix_identifier(prefix.trim(), min, max)?;


The whole identifier must be between min/max length; not just the prefix

Ah, the validate_prefix_identifier does the length checks with the "full id" using 0 and u64::MAX. Although correct, that's a little confusing.

A more general question: what was wrong with the previous identifier validation?

It is so.
validate_prefix_identifier performs this check just after creating "min/max default identifiers" and passing them to the validate_default_identifier

Before, here we were allowing wrong ClientId creation. E.g. "mock_clientid", "nonexistingclient" were ok.
Also, "07-tendermint-" and " 07-tendermint " as a valid ClientTypes were accepted.

They are okay though; according to the spec, those are all valid identifiers, and this is the only requirement for client identifiers. Our ChainId type also properly handles identifiers that are not in the "epoch" format

I believe ibc-go has these extra restrictions, but only for their ClientType. Other chains are free to name their clients without these extra restrictions

They are okay though; according to the spec, those are all valid identifiers, and this is the only requirement for client identifiers. Our ChainId type also properly handles identifiers that are not in the "epoch" format

Our client creator restricts. #639 (comment)
Thus, Should we move away from the client counter?

plafer · 2023-05-01T14:25:22Z

crates/ibc/src/core/ics02_client/client_type.rs

-#[cfg_attr(feature = "serde", derive(serde::Serialize, serde::Deserialize))]
-/// Type of the client, depending on the specific consensus algorithm.
-#[derive(Clone, Debug, PartialEq, Eq, PartialOrd, Ord, Hash)]
-pub struct ClientType(String);


why does it make more sense for ClientType to be in ics24_host? It’s still used as a prefix to a ClientId; seems to make more sense to live under ics02_client?

Since all the identifiers including the ClientId live in ICS24_host

ICS-24 defines the concept of an identifier. ClientType is, well, the "type" of a client, which turns out is used to create an identifier; it is not an identifier itself. Basically, it is a "client" concept, which is also used to create identifiers. I think it makes more sense for it to live where it was

Maybe naming is getting us confused. We don't have such a type in the spec anymore, and honestly, Looking into the role of/functionality ClientType already serves (being only used as part/prefix of ClientId). It's more of ClientIdPrefix, so I still think it should be placed in ics24. Though, it's not a big deal. Alright

plafer · 2023-05-01T14:36:27Z

crates/ibc/src/core/ics24_host/validate.rs

-/// A valid identifier must be between 9-64 characters and only contain lowercase
-/// alphabetic characters,
+/// A valid client identifier must be between 9-64 characters as specified in
+/// the ICS-24 spec.
 pub fn validate_client_identifier(id: &str) -> Result<(), Error> {
    validate_identifier(id, 9, 64)


This is no longer correct; client identifiers are not required to be in the {client_type}-version format

Indeed. Not required by spec. Though, check here please. This is the only format we allow for client creation.

Another point:
This validate_prefix_identifier will also be used to check the validity of ConnectionId and ChannelId creation from String to be of {identifier}-u64 format. You can see its usage in #650

It is the current way that we create client identifiers, which is a subset of all allowed client identifiers. In other words, if ever we changed how we do this in the future (e.g. to allow for other formats), then this validate_client_identifier() would fail for valid identifiers. This method should check the validity of client identifiers according to the spec, which will make sure that no matter how we create our client identifiers, they will be valid

As per our discussion, needed changes applied. Please check it out again.

plafer

👌

* Add missing ClientType validation * Move ClientType verification under validate.rs to cover all needed checks * Move ClientType tests into the validate.rs * Some adjustments * Fix tests * Mend ClientId for upgrade-client tests * Rename to default_validate_identifier * Refactor for more generic functions * Revise changelog * Add missing counterparty client_id check * Remove duplicate checks and unnecessary methods * Add missing changelog * From<String> instead of new_unchecked * Revert ClientType placement * Removed unnecessary checks and improved naming * Fix some nits

Add missing ClientType validation

e1bee48

Farhad-Shabani changed the title ~~Add missing ClientType validation method~~ Add missing ClientType validations Apr 18, 2023

Farhad-Shabani added 6 commits April 19, 2023 10:43

Move ClientType verification under validate.rs to cover all needed ch…

eef2633

…ecks

Move ClientType tests into the validate.rs

3244dbc

Some adjustments

f2e599f

Fix tests

24877e4

Mend ClientId for upgrade-client tests

e5f6710

Rename to default_validate_identifier

3010b6d

Farhad-Shabani requested a review from plafer April 19, 2023 19:16

Farhad-Shabani marked this pull request as ready for review April 19, 2023 19:17

Refactor for more generic functions

262170e

Farhad-Shabani changed the title ~~Add missing ClientType validations~~ Add missing ClientType, ClientId validation checks Apr 20, 2023

Farhad-Shabani added 4 commits April 20, 2023 07:37

Merge branch 'main' into farhad/client-type-validation

4d437cb

Revise changelog

c6ebd3e

Add missing counterparty client_id check

72f635e

Remove duplicate checks and unnecessary methods

f04699e

Farhad-Shabani added 2 commits April 24, 2023 12:02

Merge branch 'main' into farhad/client-type-validation

5f02452

Add missing changelog

5080510

Farhad-Shabani mentioned this pull request Apr 25, 2023

Add missing validation checks for all IBC message types #650

Merged

7 tasks

plafer reviewed May 1, 2023

View reviewed changes

crates/ibc/src/core/ics24_host/identifier.rs Outdated Show resolved Hide resolved

From<String> instead of new_unchecked

1396ad6

plafer reviewed May 1, 2023

View reviewed changes

Farhad-Shabani added 3 commits May 1, 2023 08:10

Revert ClientType placement

94bfc5e

Removed unnecessary checks and improved naming

6e36c3a

Fix some nits

dd42949

plafer approved these changes May 1, 2023

View reviewed changes

plafer merged commit 89c5298 into main May 1, 2023

plafer deleted the farhad/client-type-validation branch May 1, 2023 21:27

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add missing `ClientType`, `ClientId` validation checks #639

Add missing `ClientType`, `ClientId` validation checks #639

Farhad-Shabani commented Apr 18, 2023 •

edited

Loading

codecov bot commented Apr 18, 2023 •

edited

Loading

plafer commented Apr 24, 2023

Farhad-Shabani commented Apr 24, 2023

plafer May 1, 2023

plafer May 1, 2023 •

edited

Loading

Farhad-Shabani May 1, 2023

Farhad-Shabani May 1, 2023

plafer May 1, 2023

plafer May 1, 2023

Farhad-Shabani May 1, 2023

plafer May 1, 2023

Farhad-Shabani May 1, 2023

plafer May 1, 2023

Farhad-Shabani May 1, 2023

Farhad-Shabani May 1, 2023

plafer May 1, 2023

Farhad-Shabani May 1, 2023

Farhad-Shabani May 1, 2023

plafer May 1, 2023

Farhad-Shabani May 1, 2023

plafer left a comment

Add missing ClientType, ClientId validation checks #639

Add missing ClientType, ClientId validation checks #639

Conversation

Farhad-Shabani commented Apr 18, 2023 • edited Loading

Description

PR author checklist:

Reviewer checklist:

codecov bot commented Apr 18, 2023 • edited Loading

Codecov Report

plafer commented Apr 24, 2023

Farhad-Shabani commented Apr 24, 2023

Choose a reason for hiding this comment

plafer May 1, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

plafer left a comment

Choose a reason for hiding this comment

Add missing `ClientType`, `ClientId` validation checks #639

Add missing `ClientType`, `ClientId` validation checks #639

Farhad-Shabani commented Apr 18, 2023 •

edited

Loading

codecov bot commented Apr 18, 2023 •

edited

Loading

plafer May 1, 2023 •

edited

Loading