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

feat: use FromStr in client_type functions to construct ClientType #731

Merged
merged 5 commits into from
Jun 26, 2023
Merged

feat: use FromStr in client_type functions to construct ClientType #731

merged 5 commits into from
Jun 26, 2023

Conversation

DaviRain-Su
Copy link
Contributor

Closes: #XXX

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

@codecov
Copy link

codecov bot commented Jun 26, 2023

Codecov Report

Patch coverage: 100.00% and project coverage change: +0.03 🎉

Comparison is base (e2c3d13) 72.32% compared to head (762692d) 72.35%.

Additional details and impacted files
@@            Coverage Diff             @@
##             main     #731      +/-   ##
==========================================
+ Coverage   72.32%   72.35%   +0.03%     
==========================================
  Files         116      116              
  Lines       15524    15526       +2     
==========================================
+ Hits        11227    11234       +7     
+ Misses       4297     4292       -5     
Impacted Files Coverage Δ
crates/ibc/src/core/ics24_host/identifier.rs 59.15% <ø> (ø)
crates/ibc/src/clients/ics07_tendermint/mod.rs 100.00% <100.00%> (ø)
crates/ibc/src/core/ics02_client/client_type.rs 68.18% <100.00%> (+22.72%) ⬆️
crates/ibc/src/core/ics02_client/events.rs 71.33% <100.00%> (+0.09%) ⬆️
crates/ibc/src/core/ics03_connection/events.rs 70.73% <100.00%> (+0.10%) ⬆️
crates/ibc/src/mock/client_state.rs 94.03% <100.00%> (ø)

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

Comment on lines 41 to 47
impl TryFrom<String> for ClientType {
type Error = IdentifierError;

fn try_from(s: String) -> Result<Self, Self::Error> {
Self::new(s)
}
}
Copy link
Member

Choose a reason for hiding this comment

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

Can we use FromStr instead of TryFrom here? to maintain consistency with the existing conversion methods for ClientId, ConnectionId, and ChannelId. Also, won't have to deal with to_string() everywhere.

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

…ics02_client`, `ics02_client/events`, `ics03_connection/events` and `mock/client_state`
@DaviRain-Su DaviRain-Su changed the title feat: use TryFrom in client_type functions to construct ClientType feat: use FromStr in client_type functions to construct ClientType Jun 26, 2023
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 5bbf3ad into cosmos:main Jun 26, 2023
@DaviRain-Su DaviRain-Su deleted the use-TryFrom-replace-From branch June 26, 2023 16:00
@Farhad-Shabani Farhad-Shabani mentioned this pull request Jun 26, 2023
7 tasks
Farhad-Shabani pushed a commit that referenced this pull request Jun 27, 2023
* 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).
@Farhad-Shabani Farhad-Shabani added this to the v0.42.0 milestone Jun 27, 2023
Farhad-Shabani pushed a commit that referenced this pull request Sep 9, 2024
…pe` (#731)

* feat: use `TryFrom` in `client_type` functions to construct `ClientType`

* Fix ClientType conversion method in identifier.rs

* Refactor client type parsing using `FromStr` in `ics07_tendermint`, `ics02_client`, `ics02_client/events`, `ics03_connection/events` and `mock/client_state`

* refactor: use core::str instead of std::str in mod.rs file of ICS07 Tendermint client
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.

2 participants