Additional tests for counterparty conn & chan ids at initialization #360

Farhad-Shabani · 2023-01-19T05:38:36Z

Closes: #175

Summary

Ensure that disallows empty strings for counterparty connection and channel ids work correctly at the initialization phases (conn_open_init and chan_open_init)

Description

Firstly, It looks like the ICS24_host statement around identifiers implicitly refers to successfully made (open) connections. However, it’s true... it does not consider the transient state of the *ends during the opening process.

Btw, I also tested the opening of a connection using basecoin-rs and Hermes and it worked properly.
The counterparty connection id was set to None for the sent MsgConnectionOpenInit.

In this PR, a few comments and tests have been added to check the corner scenarios.
Also, while skimming the codes, I changed the naming of the client_counter, connection_counter, and channel_counter methods to correspond with the exact work they do. Counting is their side job, but they're basically used for identifier creation all over the repository. This way it would also align with IBC-go. Though… take it as a suggestion.

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).

* Investigate initialization with empty string for counterparty ids * Add some unit tests for counterparty conn & chan ids * Rename methods that creates identifiers

Signed-off-by: Farhad Shabani <Farhad.Shabani@gmail.com>

codecov · 2023-01-19T05:59:02Z

Codecov Report

Base: 57.68% // Head: 57.71% // Increases project coverage by +0.03% 🎉

Coverage data is based on head (30727a0) compared to base (7f62c1b).
Patch coverage: 90.74% of modified lines in pull request are covered.

❗ Current head 30727a0 differs from pull request most recent head e2ea605. Consider uploading reports for the commit e2ea605 to get more accurate results

Additional details and impacted files

@@            Coverage Diff             @@
##             main     #360      +/-   ##
==========================================
+ Coverage   57.68%   57.71%   +0.03%     
==========================================
  Files         124      124              
  Lines       15333    15357      +24     
==========================================
+ Hits         8845     8864      +19     
- Misses       6488     6493       +5

Impacted Files	Coverage Δ
crates/ibc/src/core/context.rs	`5.81% <0.00%> (-0.04%)`	⬇️
crates/ibc/src/core/ics02_client/context.rs	`85.96% <ø> (ø)`
crates/ibc/src/core/ics03_connection/context.rs	`100.00% <ø> (ø)`
crates/ibc/src/core/ics04_channel/context.rs	`74.54% <ø> (ø)`
...bc/src/core/ics04_channel/handler/chan_open_ack.rs	`49.11% <ø> (ø)`
...rc/core/ics04_channel/handler/chan_open_confirm.rs	`44.17% <ø> (ø)`
...bc/src/core/ics04_channel/handler/chan_open_try.rs	`52.91% <ø> (ø)`
crates/ibc/src/core/ics26_routing/handler.rs	`91.11% <ø> (ø)`
...rc/core/ics03_connection/handler/conn_open_init.rs	`56.07% <50.00%> (-0.24%)`	⬇️
crates/ibc/src/core/ics03_connection/connection.rs	`29.46% <100.00%> (+0.22%)`	⬆️
... and 10 more

Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here.

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

plafer · 2023-01-19T15:11:42Z

crates/ibc/src/core/context.rs

@@ -255,7 +255,7 @@ mod val_exec_ctx {

        /// Returns a natural number, counting how many clients have been created thus far.
        /// The value of this counter should increase only via method `ClientKeeper::increase_client_counter`.
-        fn client_counter(&self) -> Result<u64, ContextError>;
+        fn generate_client_identifier(&self) -> Result<u64, ContextError>;


Note that we have an issue (#14) that tracks this. But also if the function were to be named generate_client_identifier, I would expect it to return a ClientId as opposed to an "id counter".

Can we make that change in another PR that addresses #14, in order to have self-contained PRs?

Didn't notice that issue.
Sure. Will do so

plafer · 2023-01-19T22:07:07Z

Note that I simplified the expression that confused me previously. Let me know if you also find that cleaner.

70c6c0d
30727a0

Farhad-Shabani · 2023-01-19T22:16:52Z

Note that I simplified the expression that confused me previously. Let me know if you also find that cleaner.

70c6c0d 30727a0

Now it is easier to catch 👍

plafer

🙏 thank you for all these tests!

crates/ibc/src/core/context.rs

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

crates/ibc/src/core/ics03_connection/context.rs

crates/ibc/src/core/ics03_connection/handler/conn_open_init.rs

crates/ibc/src/core/ics04_channel/context.rs

crates/ibc/src/core/ics04_channel/handler/chan_open_init.rs

plafer

🎉

…360) * Additional tests for counterparty conn & chan ids at initialization * Investigate initialization with empty string for counterparty ids * Add some unit tests for counterparty conn & chan ids * Rename methods that creates identifiers * Fix cargo check & add changelog entry * Revert *counter naming changes * Fix a comment in MockIbcStore * Change unclog section to improvements * Fix some comments from reverting *_counters names * Simplify conn counterparty parsing * simplify channel counterparty parsing * Fix comment issues Signed-off-by: Farhad Shabani <Farhad.Shabani@gmail.com> Co-authored-by: Philippe Laferriere <plafer@protonmail.com>

Farhad-Shabani and others added 2 commits January 18, 2023 21:17

Additional tests for counterparty conn & chan ids at initialization

a11663f

* Investigate initialization with empty string for counterparty ids * Add some unit tests for counterparty conn & chan ids * Rename methods that creates identifiers

Merge branch 'main' into farhad/conn_id_investigation

6b1731f

Signed-off-by: Farhad Shabani <Farhad.Shabani@gmail.com>

Farhad-Shabani self-assigned this Jan 19, 2023

Fix cargo check & add changelog entry

81cac5f

plafer reviewed Jan 19, 2023

View reviewed changes

Revert *counter naming changes

dc80ad0

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

Farhad-Shabani and others added 5 commits January 19, 2023 09:05

Fix a comment in MockIbcStore

ae5fb33

Change unclog section to improvements

ac5e451

Fix some comments from reverting *_counters names

70d787c

Simplify conn counterparty parsing

70c6c0d

simplify channel counterparty parsing

30727a0

plafer suggested changes Jan 19, 2023

View reviewed changes

Fix comment issues

e2ea605

plafer approved these changes Jan 20, 2023

View reviewed changes

plafer merged commit b807d8f into main Jan 20, 2023

plafer deleted the farhad/conn_id_investigation branch January 20, 2023 14:42

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Additional tests for counterparty conn & chan ids at initialization #360

Additional tests for counterparty conn & chan ids at initialization #360

Farhad-Shabani commented Jan 19, 2023 •

edited

Loading

codecov bot commented Jan 19, 2023 •

edited

Loading

plafer Jan 19, 2023

Farhad-Shabani Jan 19, 2023

plafer commented Jan 19, 2023

Farhad-Shabani commented Jan 19, 2023

plafer left a comment

plafer left a comment

Additional tests for counterparty conn & chan ids at initialization #360

Additional tests for counterparty conn & chan ids at initialization #360

Conversation

Farhad-Shabani commented Jan 19, 2023 • edited Loading

Summary

Description

PR author checklist:

Reviewer checklist:

codecov bot commented Jan 19, 2023 • edited Loading

Codecov Report

plafer Jan 19, 2023

Choose a reason for hiding this comment

Farhad-Shabani Jan 19, 2023

Choose a reason for hiding this comment

plafer commented Jan 19, 2023

Farhad-Shabani commented Jan 19, 2023

plafer left a comment

Choose a reason for hiding this comment

plafer left a comment

Choose a reason for hiding this comment

Farhad-Shabani commented Jan 19, 2023 •

edited

Loading

codecov bot commented Jan 19, 2023 •

edited

Loading