Problem: client code uses a custom tendermint client (CRO-101) #571

tomtau · 2019-11-11T10:49:11Z

Solution: migrated code to tendermint-rs

client-common/Cargo.toml

codecov · 2019-11-11T11:30:39Z

Codecov Report

Merging #571 into master will increase coverage by 0.18%.
The diff coverage is 68.8%.

@@            Coverage Diff             @@
##           master     #571      +/-   ##
==========================================
+ Coverage   67.97%   68.16%   +0.18%     
==========================================
  Files         124      121       -3     
  Lines       14722    14636      -86     
==========================================
- Hits        10008     9976      -32     
+ Misses       4714     4660      -54

Impacted Files	Coverage Δ
client-cli/src/command.rs	`0% <ø> (ø)`	⬆️
client-rpc/src/server.rs	`9.17% <ø> (ø)`	⬆️
client-rpc/src/rpc/wallet_rpc.rs	`71.91% <0%> (+0.18%)`	⬆️
client-core/src/wallet/default_wallet_client.rs	`59.12% <0%> (ø)`	⬆️
...lient-common/src/tendermint/unauthorized_client.rs	`0% <0%> (ø)`	⬆️
client-core/src/synchronizer/auto_sync_core.rs	`43.96% <0%> (+0.43%)`	⬆️
client-common/src/block_header.rs	`0% <0%> (ø)`	⬆️
...ient-common/src/tendermint/websocket_rpc_client.rs	`0% <0%> (ø)`	⬆️
client-rpc/src/rpc/multisig_rpc.rs	`33.99% <0%> (ø)`	⬆️
client-core/src/handler/default_block_handler.rs	`90.51% <100%> (ø)`	⬆️
... and 15 more

452: Bump serde_json from 1.0.40 to 1.0.41 r=devashishdxt a=dependabot-preview[bot] Bumps [serde_json](https://github.com/serde-rs/json) from 1.0.40 to 1.0.41. <details> <summary>Release notes</summary> *Sourced from [serde_json's releases](https://github.com/serde-rs/json/releases).* > ## v1.0.41 > - Documentation improvements (thanks [@est31](https://github.com/est31) and [@yankuu](https://github.com/yankuu)) </details> <details> <summary>Commits</summary> - [`08b0cde`](serde-rs/json@08b0cde) Release 1.0.41 - [`4c88bef`](serde-rs/json@4c88bef) Document that you can't deserialize &str from reader - [`5bc17fa`](serde-rs/json@5bc17fa) Remove try! macro from tests - [`fbd1434`](serde-rs/json@fbd1434) Merge pull request [crypto-com#571](https://github-redirect.dependabot.com/serde-rs/json/issues/571) from yankuu/patch-1 - [`f168235`](serde-rs/json@f168235) Minor typo in doc - [`05f6489`](serde-rs/json@05f6489) Ignore too_many_lines lint - [`b3d33fe`](serde-rs/json@b3d33fe) Use swap_remove on IndexMap - [`d941bfa`](serde-rs/json@d941bfa) Format with rustfmt 2019-08-19 - [`684bf88`](serde-rs/json@684bf88) Include tests crate in workspace - [`7613c04`](serde-rs/json@7613c04) Enable testing cargo build on old compilers - Additional commits viewable in [compare view](serde-rs/json@v1.0.40...v1.0.41) </details> <br /> [![Dependabot compatibility score](https://api.dependabot.com/badges/compatibility_score?dependency-name=serde_json&package-manager=cargo&previous-version=1.0.40&new-version=1.0.41)](https://dependabot.com/compatibility-score.html?dependency-name=serde_json&package-manager=cargo&previous-version=1.0.40&new-version=1.0.41) Dependabot will resolve any conflicts with this PR as long as you don't alter it yourself. You can also trigger a rebase manually by commenting `@dependabot rebase`. [//]: # (dependabot-automerge-start) [//]: # (dependabot-automerge-end) --- <details> <summary>Dependabot commands and options</summary> <br /> You can trigger Dependabot actions by commenting on this PR: - `@dependabot rebase` will rebase this PR - `@dependabot recreate` will recreate this PR, overwriting any edits that have been made to it - `@dependabot merge` will merge this PR after your CI passes on it - `@dependabot squash and merge` will squash and merge this PR after your CI passes on it - `@dependabot cancel merge` will cancel a previously requested merge and block automerging - `@dependabot reopen` will reopen this PR if it is closed - `@dependabot close` will close this PR and stop Dependabot recreating it. You can achieve the same result by closing it manually - `@dependabot ignore this major version` will close this PR and stop Dependabot creating any more for this major version (unless you reopen the PR or upgrade to it yourself) - `@dependabot ignore this minor version` will close this PR and stop Dependabot creating any more for this minor version (unless you reopen the PR or upgrade to it yourself) - `@dependabot ignore this dependency` will close this PR and stop Dependabot creating any more for this dependency (unless you reopen the PR or upgrade to it yourself) - `@dependabot use these labels` will set the current labels as the default for future PRs for this repo and language - `@dependabot use these reviewers` will set the current reviewers as the default for future PRs for this repo and language - `@dependabot use these assignees` will set the current assignees as the default for future PRs for this repo and language - `@dependabot use this milestone` will set the current milestone as the default for future PRs for this repo and language - `@dependabot badge me` will comment on this PR with code to add a "Dependabot enabled" badge to your readme Additionally, you can set the following in your Dependabot [dashboard](https://app.dependabot.com): - Update frequency (including time of day and day of week) - Pull request limits (per update run and/or open at any time) - Out-of-range updates (receive only lockfile updates, if desired) - Security updates (receive only security updates, if desired) </details> Co-authored-by: dependabot-preview[bot] <27856297+dependabot-preview[bot]@users.noreply.github.com>

tomtau · 2019-11-13T06:26:18Z

wait for @devashishdxt to check, but i guess it's ok to go (as an intermediate step before adding the lite client)?

yihuang · 2019-11-13T06:28:29Z

Yeah, I think so.

devashishdxt

Looks good to me. Just one question: Does this use Json RPC over websockets or HTTP?

yihuang · 2019-11-13T06:32:31Z

The Client trait is basically the same as before, both http and websocket implementation. Only migrated the data types.

tomtau · 2019-11-13T06:33:14Z

bors try

devashishdxt · 2019-11-13T06:34:30Z

The Client trait is basically the same as before, both http and websocket implementation. Only migrated the data types.

Then, I think, we should modify the pull request title.

Solution: - migrate the data types to tendermint-rs library, to integrate lite client feature later on. - Doesn't use the tendermint-rs's client implementation, because ours have more features (batch request, websocket).

yihuang · 2019-11-13T06:52:00Z

The Client trait is basically the same as before, both http and websocket implementation. Only migrated the data types.

Then, I think, we should modify the pull request title.

I edited the commit message to include this information.

bors · 2019-11-13T07:10:43Z

try

Build failed

continuous-integration/travis-ci/push

tomtau · 2019-11-13T08:21:46Z

bors r+

bors · 2019-11-13T09:02:24Z

Merge conflict (retrying...)

571: Problem: client code uses a custom tendermint client (CRO-101) r=tomtau a=tomtau Solution: migrated code to tendermint-rs 580: Bump unicase from 2.5.1 to 2.6.0 r=tomtau a=dependabot-preview[bot] Bumps [unicase](https://github.com/seanmonstar/unicase) from 2.5.1 to 2.6.0. <details> <summary>Release notes</summary> *Sourced from [unicase's releases](https://github.com/seanmonstar/unicase/releases).* > ## v2.6.0 > - Fix `UniCase::eq` in Unicode mode so that it doesn't equal a substring of the other. > - Make crate `no_std`. </details> <details> <summary>Commits</summary> - [`7b116bc`](seanmonstar/unicase@7b116bc) v2.6.0 - [`c14856b`](seanmonstar/unicase@c14856b) Fix Unicode::eq to not equal when one side is a substring of the other ([#39](https://github-redirect.dependabot.com/seanmonstar/unicase/issues/39)) - [`4788cba`](seanmonstar/unicase@4788cba) update version_check - [`fbab380`](seanmonstar/unicase@fbab380) Implement no_std support ([#34](https://github-redirect.dependabot.com/seanmonstar/unicase/issues/34)) - See full diff in [compare view](seanmonstar/unicase@v2.5.1...v2.6.0) </details> <br /> [![Dependabot compatibility score](https://api.dependabot.com/badges/compatibility_score?dependency-name=unicase&package-manager=cargo&previous-version=2.5.1&new-version=2.6.0)](https://dependabot.com/compatibility-score.html?dependency-name=unicase&package-manager=cargo&previous-version=2.5.1&new-version=2.6.0) Dependabot will resolve any conflicts with this PR as long as you don't alter it yourself. You can also trigger a rebase manually by commenting `@dependabot rebase`. [//]: # (dependabot-automerge-start) [//]: # (dependabot-automerge-end) --- <details> <summary>Dependabot commands and options</summary> <br /> You can trigger Dependabot actions by commenting on this PR: - `@dependabot rebase` will rebase this PR - `@dependabot recreate` will recreate this PR, overwriting any edits that have been made to it - `@dependabot merge` will merge this PR after your CI passes on it - `@dependabot squash and merge` will squash and merge this PR after your CI passes on it - `@dependabot cancel merge` will cancel a previously requested merge and block automerging - `@dependabot reopen` will reopen this PR if it is closed - `@dependabot close` will close this PR and stop Dependabot recreating it. You can achieve the same result by closing it manually - `@dependabot ignore this major version` will close this PR and stop Dependabot creating any more for this major version (unless you reopen the PR or upgrade to it yourself) - `@dependabot ignore this minor version` will close this PR and stop Dependabot creating any more for this minor version (unless you reopen the PR or upgrade to it yourself) - `@dependabot ignore this dependency` will close this PR and stop Dependabot creating any more for this dependency (unless you reopen the PR or upgrade to it yourself) - `@dependabot use these labels` will set the current labels as the default for future PRs for this repo and language - `@dependabot use these reviewers` will set the current reviewers as the default for future PRs for this repo and language - `@dependabot use these assignees` will set the current assignees as the default for future PRs for this repo and language - `@dependabot use this milestone` will set the current milestone as the default for future PRs for this repo and language - `@dependabot badge me` will comment on this PR with code to add a "Dependabot enabled" badge to your readme Additionally, you can set the following in your Dependabot [dashboard](https://app.dependabot.com): - Update frequency (including time of day and day of week) - Pull request limits (per update run and/or open at any time) - Out-of-range updates (receive only lockfile updates, if desired) - Security updates (receive only security updates, if desired) </details> Co-authored-by: yihuang <huang@crypto.com> Co-authored-by: dependabot-preview[bot] <27856297+dependabot-preview[bot]@users.noreply.github.com>

bors · 2019-11-13T09:55:27Z

Build failed (retrying...)

continuous-integration/travis-ci/push

tomtau · 2019-11-13T10:35:39Z

bors r-

bors · 2019-11-13T10:35:40Z

Canceled

tomtau · 2019-11-13T10:36:53Z

@yihuang seems based on the integration test: https://travis-ci.org/crypto-com/chain/jobs/611272489#L2007
that the client expect code field in the response, but that may be optional?

yihuang · 2019-11-13T10:41:04Z

@yihuang seems based on the integration test: https://travis-ci.org/crypto-com/chain/jobs/611272489#L2007
that the client expect code field in the response, but that may be optional?

abci_query does need to return a code field, is it testing tendermint rpc node?

tomtau · 2019-11-13T10:42:08Z

@yihuang seems based on the integration test: https://travis-ci.org/crypto-com/chain/jobs/611272489#L2007
that the client expect code field in the response, but that may be optional?

abci_query does need to return a code field, is it testing tendermint rpc node?

yeah, it's running Tendermint (0.32)

yihuang · 2019-11-13T10:42:42Z

@yihuang seems based on the integration test: https://travis-ci.org/crypto-com/chain/jobs/611272489#L2007
that the client expect code field in the response, but that may be optional?

abci_query does need to return a code field, is it testing tendermint rpc node?

yeah, it's running Tendermint (0.32)

Except it returns error I guess.

tomtau · 2019-11-13T10:44:34Z

it may be returning an error -- probably better to run locally and see

leejw51crypto · 2019-11-14T05:56:46Z

please resubmit again,
ci builder will check

tomtau · 2019-11-15T06:05:30Z

I can't resolve the conflicts on / push to @yihuang's repo, so closing this PR for now -- feel free to re-open

tomtau requested review from yihuang, calvinaco, devashishdxt, leejw51crypto, lezzokafka and linfeng-crypto November 11, 2019 10:49

tomtau commented Nov 11, 2019

View reviewed changes

client-common/Cargo.toml Outdated Show resolved Hide resolved

yihuang force-pushed the cro-101 branch 3 times, most recently from 76070d8 to 379fe8a Compare November 11, 2019 12:24

leejw51crypto approved these changes Nov 13, 2019

View reviewed changes

yihuang force-pushed the cro-101 branch from dc36832 to 21765ee Compare November 13, 2019 05:46

yihuang approved these changes Nov 13, 2019

View reviewed changes

yihuang force-pushed the cro-101 branch from 21765ee to a324a8b Compare November 13, 2019 05:47

devashishdxt approved these changes Nov 13, 2019

View reviewed changes

bors bot added a commit that referenced this pull request Nov 13, 2019

Try #571:

c86c7ec

Problem: client code uses a custom tendermint client (CRO-101)

8dce32c

Solution: - migrate the data types to tendermint-rs library, to integrate lite client feature later on. - Doesn't use the tendermint-rs's client implementation, because ours have more features (batch request, websocket).

yihuang force-pushed the cro-101 branch from a324a8b to 8dce32c Compare November 13, 2019 06:51

calvinlauyh approved these changes Nov 14, 2019

View reviewed changes

calvinaco approved these changes Nov 14, 2019

View reviewed changes

tomtau closed this Nov 15, 2019

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Problem: client code uses a custom tendermint client (CRO-101) #571

Problem: client code uses a custom tendermint client (CRO-101) #571

tomtau commented Nov 11, 2019

codecov bot commented Nov 11, 2019 •

edited

Loading

tomtau commented Nov 13, 2019

yihuang commented Nov 13, 2019

devashishdxt left a comment

yihuang commented Nov 13, 2019

tomtau commented Nov 13, 2019

devashishdxt commented Nov 13, 2019

yihuang commented Nov 13, 2019

bors bot commented Nov 13, 2019

tomtau commented Nov 13, 2019

bors bot commented Nov 13, 2019

bors bot commented Nov 13, 2019

tomtau commented Nov 13, 2019

bors bot commented Nov 13, 2019

tomtau commented Nov 13, 2019

yihuang commented Nov 13, 2019

tomtau commented Nov 13, 2019

yihuang commented Nov 13, 2019

tomtau commented Nov 13, 2019

leejw51crypto commented Nov 14, 2019

tomtau commented Nov 15, 2019 •

edited

Loading

Problem: client code uses a custom tendermint client (CRO-101) #571

Problem: client code uses a custom tendermint client (CRO-101) #571

Conversation

tomtau commented Nov 11, 2019

codecov bot commented Nov 11, 2019 • edited Loading

Codecov Report

tomtau commented Nov 13, 2019

yihuang commented Nov 13, 2019

devashishdxt left a comment

Choose a reason for hiding this comment

yihuang commented Nov 13, 2019

tomtau commented Nov 13, 2019

devashishdxt commented Nov 13, 2019

yihuang commented Nov 13, 2019

bors bot commented Nov 13, 2019

try

Build failed

tomtau commented Nov 13, 2019

bors bot commented Nov 13, 2019

Merge conflict (retrying...)

bors bot commented Nov 13, 2019

Build failed (retrying...)

tomtau commented Nov 13, 2019

bors bot commented Nov 13, 2019

Canceled

tomtau commented Nov 13, 2019

yihuang commented Nov 13, 2019

tomtau commented Nov 13, 2019

yihuang commented Nov 13, 2019

tomtau commented Nov 13, 2019

leejw51crypto commented Nov 14, 2019

tomtau commented Nov 15, 2019 • edited Loading

codecov bot commented Nov 11, 2019 •

edited

Loading

tomtau commented Nov 15, 2019 •

edited

Loading