Skip to content

Conversation

dmitry-timofeev
Copy link
Contributor

@dmitry-timofeev dmitry-timofeev commented Sep 30, 2019

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 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
@Value
private static class GetBlockResponse {
GetBlockResponseBlock block;
// todo: (to discuss): Shall we really remove the intermediate class?
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@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)?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Contributor Author

@dmitry-timofeev dmitry-timofeev Oct 1, 2019

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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:

  1. 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.
  2. (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
Copy link

coveralls commented Sep 30, 2019

Coverage Status

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.
@Value
private static class GetBlockResponse {
GetBlockResponseBlock block;
// todo: (to discuss): Shall we really remove the intermediate class?
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

* A number of {@linkplain TransactionStatus#COMMITTED committed} transactions in the blockchain.
*/
long numCommittedTransactions;
} No newline at end of file
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

the file should end with an empty line

// 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
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.
Request request = get(url(BLOCK, query));

return blockingExecuteAndParse(request, ExplorerApiHelper::parseGetBlockResponse);
return blockingExecute(request, response -> {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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


import lombok.Value;


Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Extra line

@dmitry-timofeev 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
[skip ci]
@dmitry-timofeev
Copy link
Contributor Author

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


### Versions Support
- Exonum 0.12.*
- Exonum Java 0.8.*
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Don't we use "binding" any more?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

@dmitry-timofeev dmitry-timofeev merged commit 3428688 into exonum:master Oct 7, 2019
@dmitry-timofeev dmitry-timofeev deleted the update-lc-for-0.12 branch October 7, 2019 08:38
@dmitry-timofeev
Copy link
Contributor Author

Thanks, merged.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Development

Successfully merging this pull request may close these issues.

4 participants