ica: wrong handshake flow tests

[colin-axner]

Description

closes: #550
closes: #522

---

Before we can merge this PR, please make sure that all the following items have been
checked off. If any of the checklist items are not applicable, please leave them but
write a little note why.

• Targeted PR against correct branch (see CONTRIBUTING.md)
• Linked to Github issue with discussion and accepted design OR link to spec that describes this work.
• Code follows the module structure standards.
• Wrote unit and integration tests
• Updated relevant documentation (docs/) or specification (x/<module>/spec/)
• Added relevant godoc comments.
• Added a relevant changelog entry to the Unreleased section in CHANGELOG.md
• Re-reviewed Files changed in the Github PR explorer
• Review Codecov Report in the comment section below once CI passes

[codecov-commenter]

Codecov Report

Merging #538 (247f322) into interchain-accounts (7efc384) will increase coverage by 0.14%.
The diff coverage is 100.00%.

@@                   Coverage Diff                   @@
##           interchain-accounts     #538      +/-   ##
=======================================================
+ Coverage                79.10%   79.25%   +0.14%     
=======================================================
  Files                      137      137              
  Lines                    10469    10459      -10     
=======================================================
+ Hits                      8282     8289       +7     
+ Misses                    1775     1758      -17     
  Partials                   412      412              

Impacted Files	Coverage Δ
...interchain-accounts/controller/keeper/handshake.go	83.09% <100.00%> (+0.74%)	⬆️
.../apps/27-interchain-accounts/host/keeper/keeper.go	84.40% <0.00%> (-1.55%)	⬇️
...s/apps/27-interchain-accounts/host/keeper/relay.go	82.35% <0.00%> (-0.34%)	⬇️
...ps/27-interchain-accounts/controller/ibc_module.go	54.05% <0.00%> (+10.81%)	⬆️
...les/apps/27-interchain-accounts/host/ibc_module.go	100.00% <0.00%> (+43.33%)	⬆️

[damiannolan]

Nice! If you want to go ahead and merge this I can try port these tests to #541.
Or if you think it's easier to wait until after we can do that too :)

[colin-axner]

Nice! If you want to go ahead and merge this I can try port these tests to #541. Or if you think it's easier to wait until after we can do that too :)

I will wait!

[colin-axner]

Updated to add invalid flow tests to both controller/host where applicable. Only Ack/Timeout needs code coverage in controller now

[damiannolan]

Awesome! Really appreciate you holding off on this for the submodules too, many thanks 🙌

[damiannolan]

So this QueryProof on the testchain just calls into ABCI for a proof that this channelKey is set in the store?
Do I understand that correctly?

What happens if it doesn't exist? Do you get an empty []byte and height of -1 or something?

Just asking out of my own curiosity :)

[colin-axner]

So this QueryProof on the testchain just calls into ABCI for a proof that this channelKey is set in the store?
Do I understand that correctly?

Correct! That the key is set in store with the expected value

What happens if it doesn't exist? Do you get an empty []byte and height of -1 or something?

It returns a non existence proof

[damiannolan]

Ah okay, great! So if you were to use the non existence proof then as argument to some other call, an error would eventually be propagated back to you I assume?

[colin-axner]

correct, the non existence proof should be rejected by the tendermint client

[damiannolan]

It looks like there's a failing test in the CI actually.

[colin-axner]

Clicking auto-merge. The only non test code change has been there since the beginning (2 approvals). The test code changes seem fine to be merged with 1 approval