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.

Sign up for GitHub

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

Jump to bottom

test: tendermint client update when client expires or validator set has changed #921

Merged
merged 26 commits into from
Oct 18, 2023
Merged

test: tendermint client update when client expires or validator set has changed #921

merged 26 commits into from
Oct 18, 2023

Conversation

rnbguy
Copy link
Collaborator

@rnbguy rnbguy commented Oct 13, 2023 •
edited
Loading

Closes: #538

Description

This PR adds and modifies a few test scenarios.

  1. Client expiry while client state update.
  2. Client state update with validator set changes (happy and sad).

PR author checklist:

  • Added changelog entry, using unclog.
  • Create a new issue for parameterized mock Header, Client, and Context configuration.
  • 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).

@rnbguy rnbguy requested a review from Farhad-Shabani October 13, 2023 16:08
@rnbguy rnbguy self-assigned this Oct 13, 2023
@codecov
Copy link

codecov bot commented Oct 14, 2023 •
edited
Loading

Codecov Report

Attention: 29 lines in your changes are missing coverage. Please review.

Comparison is base (245d2fc) 67.51% compared to head (57180f9) 67.56%.
Report is 1 commits behind head on main.

Additional details and impacted files 
@@            Coverage Diff             @@
##             main     #921      +/-   ##
==========================================
+ Coverage   67.51%   67.56%   +0.04%     
==========================================
  Files         131      131              
  Lines       16091    16283     +192     
==========================================
+ Hits        10864    11001     +137     
- Misses       5227     5282      +55
Files Coverage Δ
crates/ibc/src/clients/ics07_tendermint/header.rs 72.81% <100.00%> (+0.13%) ⬆️
...ibc/src/core/ics02_client/handler/update_client.rs 95.37% <100.00%> (+0.13%) ⬆️
crates/ibc/src/mock/host.rs 92.10% <97.36%> (+0.50%) ⬆️
...s/ibc/src/clients/ics07_tendermint/client_state.rs 74.23% <90.47%> (+1.40%) ⬆️
crates/ibc/src/mock/context.rs 72.81% <83.54%> (-2.41%) ⬇️

... and 2 files with indirect coverage changes

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@rnbguy
Copy link
Collaborator Author

rnbguy commented Oct 14, 2023

I am not sure what is going on. The earlier commits failed on gh action, although they were successful on my machine (cargo test --all-features --no-run and act). I added 0f4503c as a fix - even though with it, the build fails on my machine - but it makes gh actions successful!

@Farhad-Shabani can you please take a look later?

@rnbguy
Copy link
Collaborator Author

rnbguy commented Oct 16, 2023

Did I hit a rare issue with Git?

This is from this PR

ibc-rs/crates/ibc/src/mock/context.rs

Line 240 in 0f4503c

max_history_size: usize,

This is from main branch

ibc-rs/crates/ibc/src/mock/context.rs

Line 203 in 245d2fc

max_history_size: u64,

Although it's shown when two commits are compared.

@rnbguy
Copy link
Collaborator Author

rnbguy commented Oct 16, 2023 •
edited
Loading

Please ignore my previous comments. I just realized the main branch progressed from what I had.

The new changes did not conflict with my PR syntactically but they conflicted semantically. 😅

@rnbguy rnbguy force-pushed the rano/test-client-update branch from 0f4503c to 89ffdbf Compare October 16, 2023 12:47
@rnbguy rnbguy marked this pull request as ready for review October 16, 2023 15:03
Farhad-Shabani
Farhad-Shabani requested changes Oct 17, 2023
View reviewed changes
crates/ibc/src/core/ics02_client/handler/update_client.rs Outdated Show resolved Hide resolved
crates/ibc/src/mock/context.rs Outdated Show resolved Hide resolved
crates/ibc/src/mock/host.rs Outdated Show resolved Hide resolved
crates/ibc/src/mock/host.rs Outdated Show resolved Hide resolved
crates/ibc/src/mock/host.rs Show resolved Hide resolved
crates/ibc/src/core/ics02_client/handler/update_client.rs Outdated Show resolved Hide resolved
crates/ibc/src/core/ics02_client/handler/update_client.rs Outdated Show resolved Hide resolved
crates/ibc/Cargo.toml Outdated Show resolved Hide resolved
crates/ibc/src/mock/context.rs Outdated Show resolved Hide resolved
crates/ibc/src/mock/context.rs Show resolved Hide resolved
rnbguy
rnbguy commented Oct 17, 2023
View reviewed changes
crates/ibc/Cargo.toml Outdated Show resolved Hide resolved
@rnbguy rnbguy force-pushed the rano/test-client-update branch from d36e9be to 1d3bba8 Compare October 18, 2023 14:33
@rnbguy
Copy link
Collaborator Author

rnbguy commented Oct 18, 2023

@Farhad-Shabani I have made the changes. I also added a new test to check max_clock_drift. I will create the issue about the TODO comments after this PR gets merged.

Do I need to add a changelog entry?

@Farhad-Shabani
Copy link
Member

@Farhad-Shabani I have made the changes. I also added a new test to check max_clock_drift. I will create the issue about the TODO comments after this PR gets merged.

Thanks a lot @rnbguy.

Do I need to add a changelog entry?

Yes please!

Farhad-Shabani
Farhad-Shabani approved these changes Oct 18, 2023
View reviewed changes
Copy link
Member

@Farhad-Shabani Farhad-Shabani left a comment

Choose a reason for hiding this comment

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

All set 🙏🏻

@Farhad-Shabani Farhad-Shabani merged commit 476937b into main Oct 18, 2023
13 checks passed
@Farhad-Shabani Farhad-Shabani deleted the rano/test-client-update branch October 18, 2023 15:11
Farhad-Shabani pushed a commit that referenced this pull request Sep 9, 2024
@rnbguy
test: tendermint client update when client expires or validator set h…
72d43eb 
…as changed (#921)

* add client expiry test

* tm hostblock supports trusted next validator set

* fix validator change update test

* fix incorrect validator updates at each block

* add sad client update for validator change

* cargo fmt

* catch up with main branch changes

* update MockContextConfig with validator set history

* refactor tests with updated MockContextConfig

* rm duplicate def of `on`

* add todo for max_history_size and validator_set_history

* consistent variable naming in tests

* bump typed-builder version

* rm redundant builder arguments

* replace todo with panic

* mv Tendermint ClientStateConfig under ics07

* use ctx_a with ctx_b instead of only ctx

* use client_id consistently

* use mocks feature directly in dev-deps

* include trusting_period and max_clock_drift in mock light client config

* revert advance chain height with timestamp

* update client expiry test

* add test to check max_clock_drift

* rm TODO comments in favor of gh issue

* revert ctx_a renaming

* add changelog entry
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@Farhad-Shabani Farhad-Shabani Farhad-Shabani approved these changes

Assignees

@rnbguy rnbguy

Labels
None yet
Projects
ibc-rs
Status: ✅ Done
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

[ICS02] Cover client frozen/expired conditions with unit tests
2 participants
@rnbguy @Farhad-Shabani