Move out `tendermint::config` to `tendermint-config` crate by soareschen · Pull Request #986 · cometbft/tendermint-rs

soareschen · 2021-09-20T10:46:33Z

Fixes #983 and #985.

Referenced an issue explaining the need for the change
Updated all relevant documentation in docs
Updated all code comments where relevant
Wrote tests
Added entry in .changelog/

codecov-commenter · 2021-09-20T11:01:13Z

Codecov Report

Merging #986 (fde78cd) into master (e3e4c88) will decrease coverage by 0.0%.
The diff coverage is 94.3%.

❗ Current head fde78cd differs from pull request most recent head 097b434. Consider uploading reports for the commit 097b434 to get more accurate results

@@           Coverage Diff            @@
##           master    #986     +/-   ##
========================================
- Coverage    72.7%   72.7%   -0.1%     
========================================
  Files         204     206      +2     
  Lines       16631   16641     +10     
========================================
+ Hits        12101   12107      +6     
- Misses       4530    4534      +4

Impacted Files	Coverage Δ
config/src/config.rs	`63.4% <ø> (ø)`
config/src/error.rs	`0.0% <0.0%> (ø)`
config/src/node_key.rs	`56.5% <0.0%> (ø)`
rpc/src/client/transport/http.rs	`0.0% <ø> (ø)`
rpc/src/client/transport/websocket.rs	`63.7% <ø> (-0.2%)`	⬇️
tendermint/src/error.rs	`0.0% <ø> (ø)`
tendermint/src/lib.rs	`100.0% <ø> (ø)`
config/src/net.rs	`92.3% <83.3%> (ø)`
config/tests/mod.rs	`96.1% <96.1%> (ø)`
config/src/lib.rs	`100.0% <100.0%> (ø)`
... and 17 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update e3e4c88...097b434. Read the comment docs.

xla

The metadata in the new cargo toml should be catered to the actual crate instead of a verbatim copy of the tendermint cargo manifest.

soareschen · 2021-09-21T12:05:06Z

The metadata in the new cargo toml should be catered to the actual crate instead of a verbatim copy of the tendermint cargo manifest.

Yeah I missed updating that after copying the boilerplate. Fixed it now.

soareschen · 2021-09-21T12:07:06Z

Rehashing my note in #983:

I have taken a look at the use of net::Address, and it does not seem like the actual details are used anywhere. In #986 I moved out tendermint::net to tendermint_config::net, and changed the field in tendermint::node::OtherInfo::rpc_address to just String. If anyone needs to do the actual URL parsing, they can do it at the call site instead.

The main difference I see of using String instead of Address for rpc_address is the way it is deserialized. With the Deserialize implementation provided by Address, an invalid URL would result in an error. However I also do not see how this can be solved using feature flag. In the absense of the url crate, should the deserialization just succeed? That wouldn't be good since the crate would effectively be behaving differently with no invariant in place. Otherwise if it is possible to implement the parsing without using url, then it is better off to just not url in both cases in the first place.

Note that the other alternative is to just wait for servo/rust-url#717 to get merged and released.

Co-authored-by: Thane Thomson <thane@informal.systems>

soareschen · 2021-09-28T11:20:26Z


    /// RPC address
-    pub rpc_address: net::Address,
+    pub rpc_address: String,


One unresolved issue is whether using String instead of net::Address for rpc_address will break any functionality. The main difference is in the implementations of Deserialize and Display. Any idea of how this field is used in practice?

@thanethomson Might have some input here.

No idea here actually.

* Fix `cargo test` and add check to CI (#990) * Relocate ABCI test to fix broken doctest Signed-off-by: Thane Thomson <connect@thanethomson.com> * Use tokio_test for mock client doctest Signed-off-by: Thane Thomson <connect@thanethomson.com> * Add CI test for default features Signed-off-by: Thane Thomson <connect@thanethomson.com> * Add `block_search` RPC endpoint (#991) * Add block_search RPC endpoint and tests * Add .changelog entry * Fix comments * tools: Fix `block_search` endpoint integration tests (#999) Closes #998 * Bump integration test tendermint to v0.34.13 * Fix kvstore integration tests * Bump tendermint version to v0.34.13 in CI Signed-off-by: Thane Thomson <connect@thanethomson.com> * ci: Build and check tools (#997) So far only the kvstore tests ran as part of the Github workfows. This would leave opportunity for changes to introduce breakage to the builds of the tools. In this change the same build and clippy stages are introduced for the tools workspace that currently run for the top-level one. Signed-off-by: xla <self@xla.is> * tools: Add `block_search` method to RPC probe (#1002) * Add missing block_search endpoint * Bump tendermint version to v0.34.13 Signed-off-by: Thane Thomson <connect@thanethomson.com> Co-authored-by: Shoaib Ahmed <sufialhussaini@gmail.com> Co-authored-by: xla <a.simmerl@gmail.com>

Signed-off-by: Thane Thomson <connect@thanethomson.com>

thanethomson

I'd say let's go ahead and merge this. Thanks @soareschen! 🎉

soareschen added 2 commits September 20, 2021 12:34

Move out tendermint::config to tendermint_config crate

9c431c6

Introduce own error type in tendermint-config

07a698a

Move tendermint::net to tendermint_config::net

4b6888b

seanchen1991 mentioned this pull request Sep 20, 2021

url crate used by tendermint does not support no_std #985

Closed

soareschen mentioned this pull request Sep 20, 2021

Move config module out from tendermint into its own crate #983

Closed

Fix tendermint-rpc error

4e49034

soareschen force-pushed the soares/move-tendermint-config branch from ba35c63 to 4e49034 Compare September 20, 2021 14:43

xla reviewed Sep 21, 2021

View reviewed changes

Update Cargo.toml for tendermint-config

8fc00bb

soareschen requested a review from seanchen1991 September 21, 2021 12:05

seanchen1991 reviewed Sep 21, 2021

View reviewed changes

Comment thread config/src/net.rs

Comment thread light-client/src/components/verifier.rs

Comment thread light-client/src/components/verifier.rs

Comment thread tendermint/src/lib.rs Outdated

Comment thread config/src/priv_validator_key.rs

Remove #![allow(dead_code)]

ba205e2

soareschen force-pushed the soares/move-tendermint-config branch from 96a51f4 to ba205e2 Compare September 22, 2021 13:44

thanethomson reviewed Sep 25, 2021

View reviewed changes

Comment thread config/Cargo.toml Outdated

Comment thread config/Cargo.toml Outdated

Comment thread rpc/Cargo.toml Outdated

Comment thread config/tests/mod.rs Outdated

soareschen and others added 4 commits September 27, 2021 10:34

Apply suggestions from code review

218ab17

Co-authored-by: Thane Thomson <thane@informal.systems>

Merge branch 'master' into soares/move-tendermint-config

0f83703

Move tendermint_config::files::test to tendermint_config::test

4c7a293

Remove config/tests/config.rs

29ff94f

soareschen mentioned this pull request Sep 27, 2021

Full support of no_std in tendermint crate #988

Closed

5 tasks

thanethomson reviewed Sep 27, 2021

View reviewed changes

Comment thread abci/src/lib.rs Outdated

Minor fixes

ddd7d40

soareschen commented Sep 28, 2021

View reviewed changes

soareschen mentioned this pull request Sep 28, 2021

Tracking issue for no-std compliance for dependencies in ibc-rs and tendermint-rs informalsystems/hermes#1359

Closed

18 tasks

Thane Thomson and others added 4 commits October 6, 2021 09:32

Merge branch 'master' into soares/move-tendermint-config

fabd123

Fix abci-test

2c2f814

Add changelog entry

4cf19e9

Signed-off-by: Thane Thomson <connect@thanethomson.com>

thanethomson added 2 commits October 7, 2021 17:11

Add more changelog entries for other breaking changes

d00ad51

Signed-off-by: Thane Thomson <connect@thanethomson.com>

Add tendermint-config to release script

12d67c9

Signed-off-by: Thane Thomson <connect@thanethomson.com>

soareschen mentioned this pull request Oct 7, 2021

Tracking issue for no-std support informalsystems/hermes#1158

Closed

25 tasks

Merge branch 'master' into soares/move-tendermint-config

2ad95d0

soareschen added the no_std no_std compatibility label Oct 7, 2021

Fix merge mistakes

097b434

thanethomson approved these changes Oct 7, 2021

View reviewed changes

thanethomson merged commit 1ab5e86 into master Oct 7, 2021

thanethomson deleted the soares/move-tendermint-config branch October 7, 2021 22:05

Conversation

soareschen commented Sep 20, 2021 • edited by thanethomson Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

codecov-commenter commented Sep 20, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Codecov Report

Uh oh!

xla left a comment

Choose a reason for hiding this comment

Uh oh!

soareschen commented Sep 21, 2021

Uh oh!

soareschen commented Sep 21, 2021

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

soareschen Sep 28, 2021

Choose a reason for hiding this comment

Uh oh!

xla Sep 29, 2021

Choose a reason for hiding this comment

Uh oh!

thanethomson Oct 5, 2021

Choose a reason for hiding this comment

Uh oh!

thanethomson left a comment

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants

soareschen commented Sep 20, 2021 •

edited by thanethomson

Loading

codecov-commenter commented Sep 20, 2021 •

edited

Loading