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

Fix 729 #732

Merged
merged 5 commits into from
Jun 27, 2023
Merged

Fix 729 #732

merged 5 commits into from
Jun 27, 2023

Conversation

DaviRain-Su
Copy link
Contributor

@DaviRain-Su DaviRain-Su commented Jun 26, 2023

Closes: #729

Description


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).

…hain_id from a raw string slice to a chain identifier type in several code locations.
@codecov
Copy link

codecov bot commented Jun 26, 2023

Codecov Report

Patch coverage: 64.28% and project coverage change: -0.02 ⚠️

Comparison is base (7a57224) 72.35% compared to head (4e05177) 72.34%.

❗ Current head 4e05177 differs from pull request most recent head 0a81b8e. Consider uploading reports for the commit 0a81b8e to get more accurate results

Additional details and impacted files
@@            Coverage Diff             @@
##             main     #732      +/-   ##
==========================================
- Coverage   72.35%   72.34%   -0.02%     
==========================================
  Files         116      116              
  Lines       15526    15532       +6     
==========================================
+ Hits        11234    11236       +2     
- Misses       4292     4296       +4     
Impacted Files Coverage Δ
...ibc/src/core/ics02_client/handler/update_client.rs 95.12% <ø> (ø)
crates/ibc/src/core/ics24_host/identifier.rs 58.57% <0.00%> (-0.59%) ⬇️
...nts/ics07_tendermint/client_state/update_client.rs 79.51% <40.00%> (-2.77%) ⬇️
...ents/ics07_tendermint/client_state/misbehaviour.rs 83.33% <85.71%> (-0.28%) ⬇️
...s/ibc/src/clients/ics07_tendermint/client_state.rs 70.01% <100.00%> (ø)

☔ View full report in Codecov by Sentry.
📢 Do you have feedback about the report comment? Let us know in this issue.

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.

Thanks for taking care of this. Left a few comments.
Plus, it needs a changelog entry.
Would you also include a changelog for #731 along with this PR?

@@ -329,7 +329,9 @@ mod tests {
let client_state = {
#[allow(deprecated)]
let raw_client_state = RawTmClientState {
chain_id: ChainId::from(tm_block.header().chain_id.clone()).to_string(),
chain_id: ChainId::from_str(tm_block.header().chain_id.to_string().as_str())
Copy link
Member

@Farhad-Shabani Farhad-Shabani Jun 26, 2023

Choose a reason for hiding this comment

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

Here we can go away with: ChainId::from(tm_block.header().chain_id.to_string()).to_string()

Copy link
Contributor Author

Choose a reason for hiding this comment

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

have fixed!

Comment on lines 164 to 168
impl From<ChainId> for String {
fn from(id: ChainId) -> Self {
tendermint::chain::Id::from_str(id.as_str()).unwrap()
}
}

impl From<tendermint::chain::Id> for ChainId {
fn from(id: tendermint::chain::Id) -> Self {
ChainId::from(id.to_string())
id.to_string()
}
}
Copy link
Member

Choose a reason for hiding this comment

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

This conversation looks redundant imo. to_string() is straightforward enough if anyone needs it.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

have removed

@@ -1127,7 +1127,7 @@ pub mod test_util {

pub fn new_dummy_from_header(tm_header: Header) -> ClientState {
ClientState::new(
tm_header.chain_id.clone().into(),
tm_header.chain_id.to_string().try_into().unwrap(),
Copy link
Member

@Farhad-Shabani Farhad-Shabani Jun 26, 2023

Choose a reason for hiding this comment

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

Likewise, we can use .to_string().into() instead

Copy link
Contributor Author

Choose a reason for hiding this comment

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

have fixed!

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.

🙏🏻

@Farhad-Shabani Farhad-Shabani merged commit 1c5b50b into cosmos:main Jun 27, 2023
@Farhad-Shabani Farhad-Shabani added this to the v0.42.0 milestone Jun 27, 2023
@DaviRain-Su DaviRain-Su deleted the fix-729 branch June 27, 2023 13:51
Farhad-Shabani pushed a commit that referenced this pull request Sep 9, 2024
* Refactor chain_id to avoid cloning to chain_id.to_string(). Convert chain_id from a raw string slice to a chain identifier type in several code locations.

* feat(ibc): remove `tendermint::chain::Id` dependency and use `FromStr` to construct `ClientType` in `ics07_tendermint` and `ics02_client` module (#729 and #731).
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
Status: Done
Development

Successfully merging this pull request may close these issues.

ChainId should serialize itself without using tendermint::chain::Id
2 participants