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

Check header and update state #1120

Closed
wants to merge 41 commits into from
Closed

Conversation

cezarad
Copy link
Contributor

@cezarad cezarad commented Jun 22, 2021

Closes: #XXX

Description


For contributor use:

  • Updated the Unreleased section of CHANGELOG.md with the issue.
  • If applicable: Unit tests written, added test to CI.
  • Linked to Github issue with discussion and accepted design OR link to spec that describes this work.
  • Updated relevant documentation (docs/) and code comments.
  • Re-reviewed Files changed in the Github PR explorer.

@cezarad cezarad marked this pull request as draft June 22, 2021 19:15
@cezarad cezarad marked this pull request as ready for review July 5, 2021 18:03
@adizere adizere self-assigned this Jul 20, 2021
Copy link
Member

@adizere adizere left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This seems almost ready, nice work Cezara!

Two questions:

  • some tests are failing when I run locally cargo test, in particular

test ics02_client::handler::update_client::tests::test_update_client_ok ... FAILED
...
test ics02_client::handler::update_client::tests::test_update_client_ok_multiple ... FAILED
test ics26_routing::handler::tests::routing_module_and_keepers ... FAILED
test ics18_relayer::utils::tests::client_update_ping_pong ... FAILED

It seems that the cause is the newly added checks in the update client handler. Can you confirm that the tests are failing and see how we can fix them please?

  • how do you think we should proceed with the code in modules/src/ics07_tendermint/header.rs which seems duplicating logic from tendermint-rs? Should we keep it there, or would it be better to work with the tendermint-rs team to adapt their interfaces, so that we avoid duplicating code?

@romac
Copy link
Member

romac commented Jul 21, 2021

  • Should we keep it there, or would it be better to work with the tendermint-rs team to adapt their interfaces, so that we avoid duplicating code?

I think we should sort this out in tendermint-rs directly and adapt the light client code so that we do not need to duplicate this code.

@romac
Copy link
Member

romac commented Jul 21, 2021

Note: there is also duplicated code between modules/src/ics07_tendermint/header.rs and modules/src/ics07_tendermint/predicates.rs

@cezarad
Copy link
Contributor Author

cezarad commented Jul 26, 2021

Note: there is also duplicated code between modules/src/ics07_tendermint/header.rs and modules/src/ics07_tendermint/predicates.rs

We can remove the predicates.rs I put all In headers

@cezarad
Copy link
Contributor Author

cezarad commented Aug 2, 2021

moved to PR verification update #1252

@cezarad cezarad closed this Aug 2, 2021
@romac romac deleted the Check-Header-And-Update-State branch January 26, 2022 14:20
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants