Update LC for 0.12 [ECR-3299] by dmitry-timofeev · Pull Request #1137 · exonum/exonum-java-binding

dmitry-timofeev · 2019-09-30T17:46:16Z

Overview

Update LC for 0.12.

Update get_blocks requests: the time is now included in the block object.
Update get_block request: it now uses a flat format.
Handle 404 when a block out of blockchain is requested.
Migrate getUnconfirmedTransactionsCount to use the new /stats endpoint.
Make Block serialization independent of the naming policy.

See:

Definition of Done

There are no TODOs left in the code
Change is covered by automated tests
The coding guidelines are followed
Public API has Javadoc
Method preconditions are checked and documented in the Javadoc of the method
Changelog is updated if needed (in case of notable or breaking changes)
The continuous integration build passes

The time is now included in the block object.

dmitry-timofeev · 2019-09-30T17:48:48Z

  @Value
  private static class GetBlockResponse {
-    GetBlockResponseBlock block;
+    // todo: (to discuss): Shall we really remove the intermediate class?


@bullet-tooth WDYT? Is being able to provide higher compatibility in some cases worth the extra cost (mapping between identical Block and GetBlockResponseBlock in main code and tests)?

As far as I remember this intermediate object was introduced because the block bay come with and without time included. And the time values were in a separate array in the response.
So GetBlockResponse can be removed if the blocks API changed

I see, thanks! To clarify — I asked about the Block + GetBlockResponseBlock classes, but, probably, we can get rid of the GetBlockResponse too.

Actually, the response format for single block also changed: it is now a flat object with everything included:

{ "proposer_id": 0, "height": 1, "tx_count": 0, "prev_hash": "81abde95e4d1ed118d398067725dc4a85fc060a2df59a6fcae21dba3712bda34", "tx_hash": "c6c0aa07f27493d2f2e5cff56c890a353a20086d6c25ec825128e12ae752b2d9", "state_hash": "2eab5971bd150b6c8057cb294e78c2548e2d6b6aae58399391f32782c34ce0d6", "precommits": [ "bc13dae3e4f95a601aeb0d64f1f2b757d1a2695bf86e271aeb32d69134711e4301001001180222220a20b31cceeffff6df1eac0ef74dfe4ea1637290caf6259f6f094a33208549f476462a220a200f709bd11d876e83492e5db79d42aae6b17d554fe0d175d4a53e6e1b8fe867bb320c08e18fceec0510c8cb93bc026b3ef32da00630ba874e566096570eeff844b1b52826bbba452674399a149a57951bf54ad460d235a71e727eed2b74630dc7577c9ee5cfa28fde45226f05730d" ], "txs": [], "time": "2019-10-01T17:07:45.663021Z" }

I wonder what we shall return to the user:

Just the response object as is (i.e., flat BlockResponse). It is not a block, i.e., there is no common Block for responses of a single and multiple blocks.

(as currently) An object with a Block (as is) and transactions. It allows to share Block with getBlocks which does not return transactions and precommits.

coveralls · 2019-09-30T18:18:25Z

Coverage decreased (-0.2%) to 85.548% when pulling 1d87c13 on dmitry-timofeev:update-lc-for-0.12 into bf56ddb on exonum:master.

The mempool info is provided through /stats endpoint since 0.12.

bullet-tooth · 2019-10-01T08:23:15Z

  @Value
  private static class GetBlockResponse {
-    GetBlockResponseBlock block;
+    // todo: (to discuss): Shall we really remove the intermediate class?


As far as I remember this intermediate object was introduced because the block bay come with and without time included. And the time values were in a separate array in the response.
So GetBlockResponse can be removed if the blocks API changed

bullet-tooth · 2019-10-01T08:26:05Z

+   * A number of {@linkplain TransactionStatus#COMMITTED committed} transactions in the blockchain.
+   */
+  long numCommittedTransactions;
+}


the file should end with an empty line

bullet-tooth · 2019-10-01T08:26:44Z

+// todo: I don't like that this class is vaguely defined (just as the endpoint) as SystemStatistics,
+//    because it is unclear what kind of things a user can find inside unless they look here.
+//    A related consideration is if we shall keep ExonumClient#getNumUnconfirmedTransactions
+//    or replace it with ExonumClient#getSystemStats — which does not tell what kind of stats


I like the approach to slightly redesign the LC and introduce ExonumClient#getSystemStats

Add explicit hamcrest-core dependency to avoid hamcrest-core 1.3 being included as transitive from mockwebserver.

Since 0.12, Exonum will return 404 if out-of-range blocks are requested.

[skip ci]

MakarovS · 2019-10-02T00:26:16Z

    Request request = get(url(/sitelet?url=https%3A%2F%2Fgithub.com%2Fexonum%2Fexonum-java-binding%2Fpull%2FBLOCK%2C%2520query));

-    return blockingExecuteAndParse(request, ExplorerApiHelper::parseGetBlockResponse);
+    return blockingExecute(request, response -> {


Why not move this block to a function if it's duplicated?

It is not duplicated as-is, so could you please suggest a viable abstraction?

Sorry, didn't notice the difference between these two - it's all good then.

MakarovS · 2019-10-02T00:28:43Z

+
+import lombok.Value;
+
+


[skip ci]

Instead of tx hashes exonum now returns an object with a service id and a tx hash.

This reverts commit d88e9aa

This reverts commit 5ce7a8e

Add @SerializedName to all fields so that Block serialization does not depend on the set naming policy.

[skip ci]

dmitry-timofeev · 2019-10-04T17:08:42Z

Finally, our system tests pass too and the last todos are resolved, so it must be ready, please review the updates.

bullet-tooth · 2019-10-07T07:36:59Z

+
+### Versions Support
+- Exonum 0.12.*
+- Exonum Java 0.8.*


Don't we use "binding" any more?

I can't say we don't, but I do skip it sometimes when unambiguous :-)

dmitry-timofeev · 2019-10-07T08:39:17Z

Thanks, merged.

Update the get_block* requests:

127dab8

The time is now included in the block object.

dmitry-timofeev added the work-in-progress 👷‍♂️ Do not expect reviewers to provide any feedback on WIP PRs — please ask for it explicitly! label Sep 30, 2019

dmitry-timofeev requested a review from bullet-tooth September 30, 2019 17:46

dmitry-timofeev commented Sep 30, 2019

View reviewed changes

Fix HealthCheckResponse to be static

df22092

Fix getUnconfirmedTransactionsCount:

a2d4943

The mempool info is provided through /stats endpoint since 0.12.

dmitry-timofeev requested a review from MakarovS September 30, 2019 19:02

Update the Exonum-common version

4e4b8d7

bullet-tooth approved these changes Oct 1, 2019

View reviewed changes

dmitry-timofeev added 5 commits October 1, 2019 12:31

Fix coverage collection

4d6baa2

Fix dependency resolution of hamcrest

9a44e35

Add explicit hamcrest-core dependency to avoid hamcrest-core 1.3 being included as transitive from mockwebserver.

Handle 'block not found' errors:

643d25c

Since 0.12, Exonum will return 404 if out-of-range blocks are requested.

Resolve some todos

7f8e2a9

Update the changelog and the readme

e40e5ee

[skip ci]

MakarovS approved these changes Oct 2, 2019

View reviewed changes

dmitry-timofeev added 11 commits October 2, 2019 20:53

Fix get block.

5c976b1

Add the changelog entry

598831b

Merge remote-tracking branch 'main/master' into update-lc-for-0.12

20e8723

[drop me] Temporarily enable full request/response logging

5ce7a8e

[skip ci]

[drop me] Add Some random tests

d88e9aa

Fix getBlocks:

7e0d555

Instead of tx hashes exonum now returns an object with a service id and a tx hash.

Revert "[drop me] Add Some random tests"

30283d7

This reverts commit d88e9aa

Revert "[drop me] Temporarily enable full request/response logging"

5e2d184

This reverts commit 5ce7a8e

Link the logging interceptor

0135506

Add @SerializedName:

38be7f5

Add @SerializedName to all fields so that Block serialization does not depend on the set naming policy.

rem todo

1d87c13

dmitry-timofeev removed the work-in-progress 👷‍♂️ Do not expect reviewers to provide any feedback on WIP PRs — please ask for it explicitly! label Oct 4, 2019

Whitespace

146e06d

[skip ci]

dmitry-timofeev requested review from MakarovS and bullet-tooth October 7, 2019 08:19

bullet-tooth approved these changes Oct 7, 2019

View reviewed changes

dmitry-timofeev merged commit 3428688 into exonum:master Oct 7, 2019

dmitry-timofeev deleted the update-lc-for-0.12 branch October 7, 2019 08:38


		import lombok.Value;

Conversation

dmitry-timofeev commented Sep 30, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Overview

Definition of Done

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

dmitry-timofeev Oct 1, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

coveralls commented Sep 30, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

dmitry-timofeev commented Oct 4, 2019

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

dmitry-timofeev commented Oct 7, 2019

Uh oh!

Reviewers

Assignees

Labels

Milestone

Development

Uh oh!

4 participants

dmitry-timofeev commented Sep 30, 2019 •

edited

Loading

dmitry-timofeev Oct 1, 2019 •

edited

Loading

coveralls commented Sep 30, 2019 •

edited

Loading