search

cosmos / ibc-rs

Rust implementation of the Inter-Blockchain Communication (IBC) protocol.
Apache License 2.0
182 stars 74 forks source link

chore: ics20 types on types only dep + toml fmt #1162

Closed dzmitry-lahoda closed 2 months ago

dzmitry-lahoda commented 2 months ago

Closes: #1162

Description

PR author checklist:

Reviewer checklist:

codecov[bot] commented 2 months ago

Codecov Report

All modified and coverable lines are covered by tests :white_check_mark:

Project coverage is 63.79%. Comparing base (2eecb5f) to head (0e75d95).

:exclamation: Current head 0e75d95 differs from pull request most recent head 481be37. Consider uploading reports for the commit 481be37 to get more accurate results

Additional details and impacted files ```diff @@ Coverage Diff @@ ## main #1162 +/- ## ========================================= + Coverage 0 63.79% +63.79% ========================================= Files 0 219 +219 Lines 0 21391 +21391 ========================================= + Hits 0 13647 +13647 - Misses 0 7744 +7744 ```

:umbrella: View full report in Codecov by Sentry.
:loudspeaker: Have feedback on the report? Share it here.

rnbguy commented 2 months ago

Thanks for the PR. Apart from the formatting, can you please explain why you made the dependencies modular?

dzmitry-lahoda commented 2 months ago

ics20 types were depending on ibc-core which depended on dozen crates including whole tendermint. ics20 types has broad usage context, without tenderming and impl details. i believe change increase (re)compilation speed and reduces client only binary(wasm) size. similar type only deps are used in types only deps crates here too.

formatting, i did not found how to format way it was auto, seems it was personal dev setup no enforced by ci. so added more ciable alternative.

rnbguy commented 2 months ago

ics20 types has broad usage context, without tenderming and impl details.

Makes sense ! @Farhad-Shabani, am I correct in believing that nothing breaks if we go ahead which this PR?

formatting

Adding some formatting rules in the repo for cargo.toml files makes sense. Just one nitpick that the align_entries doesn't align all entries.

I think the taplo.toml needs two more extra options,

array_auto_expand = false
array_auto_collapse = false

Also, would it be okay if I moved the taplo formatting rules in a separate PR? Feel free to open one by yourself.

dzmitry-lahoda commented 2 months ago

align_entries aligns entries, but not they way manual alignment was done (try to space after version, and it will align). so align of taplo is less aggresive than manual or what developer used to align before. problem is that I do not have that tool nor know its name. seems local developer setup.

rnbguy commented 2 months ago

Let's take care of the toml formatting in a separate PR. It requires updating the repo-wide all Cargo.toml and running toml formatte check on CI.

Apart from that, can you provide an example case for the following:

i believe change increase (re)compilation speed and reduces client only binary(wasm) size.

I would like to follow what dependency is causing the bloat. The way I see, cargo build --release should optimize away all the redundant dependencies.

dzmitry-lahoda commented 2 months ago

I would like to follow what dependency is causing the bloat. The way I see, cargo build --release should optimize away all the redundant dependencies.

I disagree, may you expand how effort on run such experiment will help with PR review?

dzmitry-lahoda commented 2 months ago

Let's take care of the toml formatting in a separate PR. It requires updating the repo-wide all Cargo.toml and running toml formatte check on CI.

Would it be better just remove taplo.toml file from this PR? It is best I can do.

rnbguy commented 2 months ago

I disagree, may you expand how effort on run such experiment will help with PR review?

I am worried that adding granular sub-crates dependencies adds extra overhead for maintenance. As you have noticed, the feature flag set increased - and it's easy to miss a flag in the future this way. We want to keep the ibc crate dependencies as succinct as possible.

But if this structure is making your wasm blob bigger or affecting runtime performance - we would definitely consider your changes. So an example would help us to make an appropriate decision.

Would it be better just remove taplo.toml file from this PR? It is best I can do.

Yes, please. Thanks :pray: Remove the changes because of the taplo formatting - including the changes in Cargo.toml. Feel free to open a separate PR for this.

dzmitry-lahoda commented 2 months ago

I am worried that adding granular sub-crates dependencies adds extra overhead for maintenance. As you have noticed, the feature flag set increased - and it's easy to miss a flag in the future this way.

it is not easy. each crate in ibc-rs covered by multi feature test (afaik there are 2-3 tools to do so). is not that the case?

dzmitry-lahoda commented 2 months ago

We want to keep the ibc crate dependencies as succinct as possible.

that what I did, replaced big dependency to smaller dependency graph in total crates count and code size referenced. is not this the case?

dzmitry-lahoda commented 2 months ago

Yes, please. Thanks 🙏 Remove the changes because of the taplo formatting - including the changes in Cargo.toml. Feel free to open a separate PR for this.

do you know what tool to use which does original formatting?

dzmitry-lahoda commented 2 months ago

I would like to follow what dependency is causing the bloat.

you can use cargo tree and other tools to look into tree, also you can see issues and prs raised on ibc-rs about big deps raised by others

The way I see, cargo build --release should optimize away all the redundant dependencies.

it does not, you can check wasm size using this before and after my changes

https://github.com/cosmos/ibc-rs/blob/main/ci/cw-check/shell.nix

rnbguy commented 2 months ago

I just run cargo build --target wasm32-unknown-unknown --no-default-features with and without --release. Here are the wasm blob sizes in bytes.

Debug Release
main 46,595,642 1,874,534
pr-1162 46,595,631 1,874,534

It looks like your PR slightly improves blob size in debug - but the sizes are all the same in release.

Here are the commands I used for reproducibility on your side.

rnbguy commented 2 months ago

do you know what tool to use which does original formatting?

I am not aware. I think that @Farhad-Shabani was manually maintaining them (probably via VSCode plugin).

dzmitry-lahoda commented 2 months ago

it usually goes via optimizer https://github.com/CosmWasm/optimizer/blob/main/Dockerfile , size would be less than 1MB.

rnbguy commented 2 months ago

We want to keep the ibc crate dependencies as succinct as possible.

I meant - keeping the Cargo.tomls brief . The dependencies get optimized away in wasm blobs or binaries.

it usually goes via optimizer

My point was that the optimizer will process almost exact wasm blobs. Anyway, I still ran cosmwasm/rust-optimizer:0.15.1 using ibc deps v0.51.0, main, and the latest commit from this PR. Here are the wasm blob sizes:

ibc dep blob size
v0.51.0 (crates-io) 284131
d5e38877 (main) 285847
481be370 (this pr) 285847

Let's keep the old changes. This way, we can manage the Cargo.tomls succinctly. I will create a separate PR for toml formatting.

Farhad-Shabani commented 2 months ago

Hey @rnbguy @dzmitry-lahoda , Apologies for the delay in responding!

As for the formatting, at the time I restructured the ibc repository, I manually adjusted the Cargo.toml files, assuming they would inherit most, if not all, fields with { workspace = true } and wouldn't need frequent updates.

Indeed, using a formatting tool would be more efficient. We should decide on the right tool and apply it to all crates (not just this one) as a separate PR. TBH, given everything else going on with ibc-rs, this is currently a lower priority.

As for the optimization, I agree with @rnbguy. Apparently, changes here don't significantly reduce the size. If there isn’t any crucial change for your project, I'd rather keep features concise and maintain crates consistent. So can’t accept changes here.

By the way, appreciate your contribution, and Feel free to re-open if anything here is necessary.

dzmitry-lahoda commented 2 months ago

Currently core and types depend on tendermint-rs. Core may be unlikely undependent easy. So types can be easy. Time representation references from tendermint. Without undepending types on tendermint, cannot undepend ics20 app types too. So undepding ics20 app types from core is required to undepend on tenderming, but not enough. So really size test did not tested that undepend condition. Just FYI.