feat: migrate `core`, `clients` modules into `ibc-core`, `ibc-clients` crates #969

Farhad-Shabani · 2023-11-17T16:03:35Z

Part of: #965

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

feat: migrate `core` module under the `ibc` crate into `ibc-core` feat: migrate `applications` module under the `ibc` crate into `ibc-apps` (#967) * feat: restructure and split off applications codebase into ibc-apps dir * imp: rename transfer dir to ics20_transfer * feat: add ibc-apps crate * fix: remove redundant dep + fix cargo doc * docs: add README and descriptions * docs: update main README page * nit: docstrings * nit: docstrings * imp: rename folder to ics20-transfer * chore: move serializers into ics20-transfer/types * fix: apply reviewer comments * imp: add docstring for cosmos_adr028_escrow_address * fix: add missing features + use workspace deps for ibc crates * imp: place re-exports under mod * nit: apply suggestions from code review Co-authored-by: Sean Chen <seanchen11235@gmail.com> Signed-off-by: Farhad Shabani <Farhad.Shabani@gmail.com> * fix: cargo fmt --------- Signed-off-by: Farhad Shabani <Farhad.Shabani@gmail.com> Co-authored-by: Sean Chen <seanchen11235@gmail.com> feat: restructure and split off applications codebase into ibc-apps dir refactor: nits from nightly clippy (#962) * use enum tuple variants directly * use first() over get(0) * use infallible conversion * rm redundant export * cargo fmt ci: spell check on GitHub workflows (#964) * typos github action * cutom config for typos * fix spelling to counterparty * fix spelling to transfer imp: rename transfer dir to ics20_transfer feat: add ibc-apps crate fix: remove redundant dep + fix cargo doc docs: add README and descriptions docs: update main README page nit: docstrings nit: docstrings imp: rename folder to ics20-transfer chore: move serializers into ics20-transfer/types fix: apply reviewer comments imp: add docstring for cosmos_adr028_escrow_address fix: add missing features + use workspace deps for ibc crates imp: place re-exports under mod feat: ibc core codebase overhaul (part-1) feat: ibc core codebase overhaul (part-2) chore: comment out ibc-query & ibc-testkit fix: add missing features nit

codecov · 2023-11-17T20:40:16Z

Codecov Report

All modified and coverable lines are covered by tests ✅

❗ No coverage uploaded for pull request base (feat/restructure-ibc@68b4ca6). Click here to learn what that means.

Additional details and impacted files

@@                   Coverage Diff                   @@
##             feat/restructure-ibc     #969   +/-   ##
=======================================================
  Coverage                        ?   70.25%           
=======================================================
  Files                           ?      175           
  Lines                           ?    17700           
  Branches                        ?        0           
=======================================================
  Hits                            ?    12436           
  Misses                          ?     5264           
  Partials                        ?        0

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

…ore-host-tendermint

Cargo.toml

* Migrate crates/ibc-clients * Fix dependencies in types directory * Fix dependencies in ibc-clients crate * Delete ibc/ics07_tendermint directory * Resolve dependencies in ibc crate * Resolve dependencies in ibc-clients crate * Wrap ClientState type in a newtype wrapper * Address all compilation errors * Migrate crates/ibc-clients * Fix dependencies in types directory * Fix dependencies in ibc-clients crate * Delete ibc/ics07_tendermint directory * Resolve dependencies in ibc crate * Resolve dependencies in ibc-clients crate * Wrap ClientState type in a newtype wrapper * Address all compilation errors * Add default feature `std` for ibc-clients crate * Change cfg derivation * Fixing import paths * Fix import paths for ibc-client-tendermint crate * Fix merge conflicts * Fix merge conflicts * Fix merge conflicts * Fix remaining compilation errors in ics07-tendermint crate * chore: adjust import paths, features and READMEs * fix: CI catches * fix: add serde dep for ibc-client-tendermint * fix: serde_tests * imp: rename ClientState to ClientStateWrapper --------- Co-authored-by: Farhad Shabani <farhad.shabani@gmail.com>

rnbguy

Awesome work, Farhad and Sean, for working on this huge PR 🎉

Suggested few changes.

crates/ibc-apps/README.md

crates/ibc-apps/ics20-transfer/Cargo.toml

crates/ibc-clients/README.md

crates/ibc-core/README.md

crates/ibc-apps/ics20-transfer/src/handler/mod.rs

rnbguy · 2023-11-21T09:52:18Z

crates/ibc-clients/ics07-tendermint/src/client_state.rs

+/// Newtype wrapper around the `ClientState` type imported from the `ibc-client-tendermint-types`
+/// crate. This wrapper exists so that we can bypass Rust's orphan rules and implement traits
+/// from `ibc::core::client::context` on the `ClientState` type.
+#[cfg_attr(feature = "serde", derive(serde::Serialize, serde::Deserialize))]
+#[derive(Clone, Debug, PartialEq)]
+pub struct ClientStateWrapper(TmClientState);


Interesting that we need this wrapper. Can we implement the traits in the trait-crates e.g. context crates?

Do you mean the context module in ics07-tendermint? We would still need the ClientStateWrapper to get around the orphan rule. Unless there's a different reason you had in mind for moving this trait implementation to the context module?

Also should note that trait implementations for the ClientStateWrapper are tailored specifically for Tendermint clients. Not appropriate to maintain those, under the ibc-core-client-context crate.

Do you mean the context module in ics07-tendermint?

Yes.

I see your point now. Personally, I don't prefer the wrapper types 😅 but I guess that's unavoidable here.

crates/ibc-core/ics02-client/types/src/msgs/create_client.rs

rnbguy · 2023-11-21T10:40:42Z

crates/ibc-core/ics03-connection/types/src/version.rs

-                Order::Ordered.as_str().to_owned(),
-                Order::Unordered.as_str().to_owned(),
-            ],
+            features: vec!["ORDER_ORDERED".to_string(), "ORDER_UNORDERED".to_string()],


I would rather keep these as this makes the string constants maintained separately.

I see what you mean. We ran into the cyclic dependency here (connection crates would depend on channel crates, while channel crates depend on connection crates) and couldn't effectively resolve it.

Ah I see. Good point! But I feel, this Default should be not be existed in that case and moved to ibc-core.

I see. It's not coming well with other implementations. But we're currently unable to switch from the default one. And placing it within ibc-core causes cyclic dependencies too.

crates/ibc-core/ics24-host/src/utils.rs

rnbguy

LGTM. Thanks 😃

rnbguy · 2023-11-21T17:16:59Z

crates/ibc-core/ics03-connection/types/src/version.rs

-                Order::Ordered.as_str().to_owned(),
-                Order::Unordered.as_str().to_owned(),
-            ],
+            features: vec!["ORDER_ORDERED".to_string(), "ORDER_UNORDERED".to_string()],


Ah I see. Good point! But I feel, this Default should be not be existed in that case and moved to ibc-core.

Cargo.toml

crates/ibc-core/ics02-client/types/src/msgs/create_client.rs

rnbguy · 2023-11-21T17:23:28Z

crates/ibc-clients/ics07-tendermint/src/client_state.rs

+/// Newtype wrapper around the `ClientState` type imported from the `ibc-client-tendermint-types`
+/// crate. This wrapper exists so that we can bypass Rust's orphan rules and implement traits
+/// from `ibc::core::client::context` on the `ClientState` type.
+#[cfg_attr(feature = "serde", derive(serde::Serialize, serde::Deserialize))]
+#[derive(Clone, Debug, PartialEq)]
+pub struct ClientStateWrapper(TmClientState);


Do you mean the context module in ics07-tendermint?

Yes.

I see your point now. Personally, I don't prefer the wrapper types 😅 but I guess that's unavoidable here.

* feat: migrate `applications` module under the `ibc` crate into `ibc-apps` (#967) * feat: restructure and split off applications codebase into ibc-apps dir * imp: rename transfer dir to ics20_transfer * feat: add ibc-apps crate * fix: remove redundant dep + fix cargo doc * docs: add README and descriptions * docs: update main README page * nit: docstrings * nit: docstrings * imp: rename folder to ics20-transfer * chore: move serializers into ics20-transfer/types * fix: apply reviewer comments * imp: add docstring for cosmos_adr028_escrow_address * fix: add missing features + use workspace deps for ibc crates * imp: place re-exports under mod * nit: apply suggestions from code review Co-authored-by: Sean Chen <seanchen11235@gmail.com> Signed-off-by: Farhad Shabani <Farhad.Shabani@gmail.com> * fix: cargo fmt --------- Signed-off-by: Farhad Shabani <Farhad.Shabani@gmail.com> Co-authored-by: Sean Chen <seanchen11235@gmail.com> * docs: ADR-008 to restructure the `ibc` crate (#966) * docs: write ADR for ibc crate restructure * fix: mistaken adr007 file naming * chore: markdown adjustment * fix: apply suggestions from code review Co-authored-by: Sean Chen <seanchen11235@gmail.com> Signed-off-by: Farhad Shabani <Farhad.Shabani@gmail.com> * docs: explanation for *-types interaction with ibc-proto --------- Signed-off-by: Farhad Shabani <Farhad.Shabani@gmail.com> Co-authored-by: Sean Chen <seanchen11235@gmail.com> * feat: migrate `core`, `clients` modules into `ibc-core`, `ibc-clients` crates (#969) * feat: migrate `core` module under the `ibc` crate into `ibc-core` feat: migrate `core` module under the `ibc` crate into `ibc-core` feat: migrate `applications` module under the `ibc` crate into `ibc-apps` (#967) * feat: restructure and split off applications codebase into ibc-apps dir * imp: rename transfer dir to ics20_transfer * feat: add ibc-apps crate * fix: remove redundant dep + fix cargo doc * docs: add README and descriptions * docs: update main README page * nit: docstrings * nit: docstrings * imp: rename folder to ics20-transfer * chore: move serializers into ics20-transfer/types * fix: apply reviewer comments * imp: add docstring for cosmos_adr028_escrow_address * fix: add missing features + use workspace deps for ibc crates * imp: place re-exports under mod * nit: apply suggestions from code review Co-authored-by: Sean Chen <seanchen11235@gmail.com> Signed-off-by: Farhad Shabani <Farhad.Shabani@gmail.com> * fix: cargo fmt --------- Signed-off-by: Farhad Shabani <Farhad.Shabani@gmail.com> Co-authored-by: Sean Chen <seanchen11235@gmail.com> feat: restructure and split off applications codebase into ibc-apps dir refactor: nits from nightly clippy (#962) * use enum tuple variants directly * use first() over get(0) * use infallible conversion * rm redundant export * cargo fmt ci: spell check on GitHub workflows (#964) * typos github action * cutom config for typos * fix spelling to counterparty * fix spelling to transfer imp: rename transfer dir to ics20_transfer feat: add ibc-apps crate fix: remove redundant dep + fix cargo doc docs: add README and descriptions docs: update main README page nit: docstrings nit: docstrings imp: rename folder to ics20-transfer chore: move serializers into ics20-transfer/types fix: apply reviewer comments imp: add docstring for cosmos_adr028_escrow_address fix: add missing features + use workspace deps for ibc crates imp: place re-exports under mod feat: ibc core codebase overhaul (part-1) feat: ibc core codebase overhaul (part-2) chore: comment out ibc-query & ibc-testkit fix: add missing features nit * fix: re-export ibc_apps in the ibc crate * chore: adjust features + add description for crates * fix: CI catches * fix: no_std * imp: define ics25-handler + move tendermint-specific impls into ibc-core-host-tendermint * fix: cargo doc * docs: add and edit READMEs * fix: cargo hack catches * fix: cargo udeps catches * chore: ajdust README tables * docs: further README adjustments * fix: typo * chore: another review of comments & READMEs * imp: move Any and Protobuf to ibc-primitives * fix: missing std features * Split out ibc-clients crate (#971) * Migrate crates/ibc-clients * Fix dependencies in types directory * Fix dependencies in ibc-clients crate * Delete ibc/ics07_tendermint directory * Resolve dependencies in ibc crate * Resolve dependencies in ibc-clients crate * Wrap ClientState type in a newtype wrapper * Address all compilation errors * Migrate crates/ibc-clients * Fix dependencies in types directory * Fix dependencies in ibc-clients crate * Delete ibc/ics07_tendermint directory * Resolve dependencies in ibc crate * Resolve dependencies in ibc-clients crate * Wrap ClientState type in a newtype wrapper * Address all compilation errors * Add default feature `std` for ibc-clients crate * Change cfg derivation * Fixing import paths * Fix import paths for ibc-client-tendermint crate * Fix merge conflicts * Fix merge conflicts * Fix merge conflicts * Fix remaining compilation errors in ics07-tendermint crate * chore: adjust import paths, features and READMEs * fix: CI catches * fix: add serde dep for ibc-client-tendermint * fix: serde_tests * imp: rename ClientState to ClientStateWrapper --------- Co-authored-by: Farhad Shabani <farhad.shabani@gmail.com> * docs: improve crates descriptions * fix: apply reviewer comments * Delete some unused modules and add some documentation * Add TODO to use u64::dev_ceil * Formatting of ibc-primitives README * fix: ibc-primitives serde feature --------- Co-authored-by: Sean Chen <seanchen11235@gmail.com> * imp: move crates into the root directory * chore: add unclogs + fix broken links * fix: additional deps and features adjustment * fix: cargo fmt, clippy * imp: review ibc-core-host-cosmos crate * fix: move consensus_state API implementation into ibc-client-tendermint * Delete empty ics07-tendermint/types README * fix: apply suggestions from code review Co-authored-by: Sean Chen <seanchen11235@gmail.com> Signed-off-by: Farhad Shabani <Farhad.Shabani@gmail.com> * fix: adjust comments, markdowns * nit: further touchups --------- Signed-off-by: Farhad Shabani <Farhad.Shabani@gmail.com> Co-authored-by: Sean Chen <seanchen11235@gmail.com>

Farhad-Shabani force-pushed the farhad/migrate-ibc-core branch from 8973428 to 10c316d Compare November 17, 2023 16:04

Farhad-Shabani added 2 commits November 17, 2023 08:15

fix: re-export ibc_apps in the ibc crate

a576426

chore: adjust features + add description for crates

b0464d4

Farhad-Shabani added 12 commits November 17, 2023 13:06

fix: CI catches

3439019

fix: no_std

c212436

imp: define ics25-handler + move tendermint-specific impls into ibc-c…

5436c99

…ore-host-tendermint

fix: cargo doc

283cf01

docs: add and edit READMEs

8f28546

fix: cargo hack catches

0b34ceb

fix: cargo udeps catches

e7975e7

chore: ajdust README tables

c0be9f0

docs: further README adjustments

8e768b9

fix: typo

d9defcf

chore: another review of comments & READMEs

5d808c7

imp: move Any and Protobuf to ibc-primitives

0bf5bb8

Farhad-Shabani marked this pull request as ready for review November 20, 2023 06:24

Farhad-Shabani mentioned this pull request Nov 20, 2023

Split out ibc-clients crate #971

Merged

7 tasks

fix: missing std features

60fd38d

Farhad-Shabani requested a review from rnbguy November 20, 2023 07:11

rnbguy reviewed Nov 20, 2023

View reviewed changes

Cargo.toml Show resolved Hide resolved

Farhad-Shabani mentioned this pull request Nov 20, 2023

feat: turn ClientConsensusStatePath::haight into Height #968

Closed

7 tasks

Farhad-Shabani changed the title ~~feat: migrate core module under the ibc crate into ibc-core~~ feat: migrate core, clients modules under the ibc crate into ibc-core, ibc-clients Nov 21, 2023

Farhad-Shabani changed the title ~~feat: migrate core, clients modules under the ibc crate into ibc-core, ibc-clients~~ feat: migrate core, clients modules into ibc-core, ibc-clients crates Nov 21, 2023

docs: improve crates descriptions

ec18401

rnbguy requested changes Nov 21, 2023

View reviewed changes

fix: apply reviewer comments

4a2b85e

Farhad-Shabani requested a review from seanchen1991 November 21, 2023 15:11

seanchen1991 added 3 commits November 21, 2023 09:44

Delete some unused modules and add some documentation

9a74b4b

Add TODO to use u64::dev_ceil

9eebafc

Formatting of ibc-primitives README

950927b

rnbguy approved these changes Nov 21, 2023

View reviewed changes

fix: ibc-primitives serde feature

cdac411

Farhad-Shabani merged commit 3cc9753 into feat/restructure-ibc Nov 21, 2023
14 checks passed

Farhad-Shabani deleted the farhad/migrate-ibc-core branch November 21, 2023 17:58

Farhad-Shabani added this to the v0.48.0 milestone Nov 24, 2023

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat: migrate `core`, `clients` modules into `ibc-core`, `ibc-clients` crates #969

feat: migrate `core`, `clients` modules into `ibc-core`, `ibc-clients` crates #969

Farhad-Shabani commented Nov 17, 2023

codecov bot commented Nov 17, 2023 •

edited

Loading

rnbguy left a comment

rnbguy Nov 21, 2023

seanchen1991 Nov 21, 2023

Farhad-Shabani Nov 21, 2023

rnbguy Nov 21, 2023

rnbguy Nov 21, 2023

Farhad-Shabani Nov 21, 2023

rnbguy Nov 21, 2023

Farhad-Shabani Nov 21, 2023

rnbguy left a comment

rnbguy Nov 21, 2023

rnbguy Nov 21, 2023

feat: migrate core, clients modules into ibc-core, ibc-clients crates #969

feat: migrate core, clients modules into ibc-core, ibc-clients crates #969

Conversation

Farhad-Shabani commented Nov 17, 2023

Description

PR author checklist:

Reviewer checklist:

codecov bot commented Nov 17, 2023 • edited Loading

Codecov Report

rnbguy left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

rnbguy left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

feat: migrate `core`, `clients` modules into `ibc-core`, `ibc-clients` crates #969

feat: migrate `core`, `clients` modules into `ibc-core`, `ibc-clients` crates #969

codecov bot commented Nov 17, 2023 •

edited

Loading