Mend the counterparty state check of ChanOpenConfirm handler

[Farhad-Shabani]

Closes: #396

---

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]

Codecov Report

Base: 58.45% // Head: 58.45% // No change to project coverage 👍

Coverage data is based on head (75d8e07) compared to base (1fb3547).
Patch coverage: 50.00% of modified lines in pull request are covered.

Additional details and impacted files

@@           Coverage Diff           @@
##             main     #397   +/-   ##
=======================================
  Coverage   58.45%   58.45%           
=======================================
  Files         124      124           
  Lines       16259    16259           
=======================================
  Hits         9505     9505           
  Misses       6754     6754           

Impacted Files	Coverage Δ
...rc/core/ics04_channel/handler/chan_open_confirm.rs	44.17% <50.00%> (ø)

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.

[hu55a1n1]

Nice catch! 🙌
We're currently unable to test such proof verification in unit tests because that would require us to be able to add Tm clients to the mock chain which I believe is currently not supported but would be nice to add a test for this in the future. Also phantom unit types/tags can help avoid such bugs (future improvement - #201).

(PS: Trying to figure out why this worked on basecoin-rs before and it seems both this bug and #353 are regressions introduced in #217.)

[Farhad-Shabani]

🙏

We're currently unable to test such proof verification in unit tests because that would require us to be able to add Tm clients to the mock chain which I believe is currently not supported but would be nice to add a test for this in the future. Also phantom unit types/tags can help avoid such bugs (future improvement - #201).

Similarly, I thought about adding unit tests for such cases, but, yea, it's not possible without Tm clients, so maybe we also can have it checked through integrating basecoin-rs into our CI. By the way, I've opened an issue #398 for this.

(PS: Trying to figure out why this worked on basecoin-rs before and it seems both this bug and #353 are regressions introduced in #217.)

Yes, exactly.