Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add needed unit or integration tests for Tendermint proof verifications #398

Closed
Tracked by #554
Farhad-Shabani opened this issue Feb 6, 2023 · 4 comments · Fixed by #1109 or #1215
Closed
Tracked by #554

Add needed unit or integration tests for Tendermint proof verifications #398

Farhad-Shabani opened this issue Feb 6, 2023 · 4 comments · Fixed by #1109 or #1215
Assignees
Labels
O: testing Objective: aims to improve testing coverage

Comments

@Farhad-Shabani
Copy link
Member

Farhad-Shabani commented Feb 6, 2023

Summary

There is a need to cover Tendermint proof verifications with proper tests after issue #396.
Mock Chain passes the proof verifications and we are not able to catch Tendermint-specific errors with the existing tests.

To Do

To ensure Tendermint proof verifications are correctly executed, we should add corresponding unit tests or somehow integrate basecoin-rs into our CI process

Blocked on: #677

@Farhad-Shabani Farhad-Shabani added the O: testing Objective: aims to improve testing coverage label Feb 6, 2023
@github-project-automation github-project-automation bot moved this to 📥 To Do in ibc-rs Feb 6, 2023
@plafer
Copy link
Contributor

plafer commented Feb 8, 2023

#396 was a core IBC bug; not Tendermint-specific. Did you mean "core handler validation needs to be tested" instead of "Tendermint proof verifications"?

@Farhad-Shabani
Copy link
Member Author

It occurred in the core section, but catching such errors needs that tests go through a proof verification process (e.g. here), which is a client-specific action. So, we can match the output with the fed test data.
Note that we already bypass this check for MockClientState by setting the result of such methods to Ok() (e.g. here)

@plafer
Copy link
Contributor

plafer commented Feb 8, 2023

Ah I see; yes that's right.

@Farhad-Shabani Farhad-Shabani added the A: blocked Admin: blocked by another (internal/external) issue or PR label Jan 2, 2024
@rnbguy rnbguy removed the A: blocked Admin: blocked by another (internal/external) issue or PR label Apr 8, 2024
@rnbguy
Copy link
Member

rnbguy commented May 4, 2024

closed by #1109

@rnbguy rnbguy closed this as completed May 4, 2024
@github-project-automation github-project-automation bot moved this from 📥 To Do to ✅ Done in ibc-rs May 4, 2024
@rnbguy rnbguy reopened this May 6, 2024
@github-project-automation github-project-automation bot moved this from ✅ Done to 📥 To Do in ibc-rs May 6, 2024
@github-project-automation github-project-automation bot moved this from 📥 To Do to ✅ Done in ibc-rs May 10, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
O: testing Objective: aims to improve testing coverage
Projects
None yet
3 participants