Update LC with DS changes [ECR-3860] #1244

MakarovS · 2019-11-27T01:10:04Z

Overview

Update LC with DS changes.

See: https://jira.bf.local/browse/ECR-3860

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

coveralls · 2019-11-27T01:35:22Z

Coverage increased (+0.07%) to 85.946% when pulling 41d708e on ECR-3860 into 9adb4b9 on master.

dmitry-timofeev · 2019-11-27T12:15:15Z

exonum-light-client/src/main/java/com/exonum/client/ExplorerApiHelper.java

-    ERROR,
-    @SerializedName("panic")
-    PANIC
+    ERROR


That doesn't look like a complete definition of possible errors:

If I understand correctly, they are serialized using this enum:
https://github.com/exonum/exonum/blob/86379cdee673dfda94b6b1e23ddedb5d512664a7/exonum/src/runtime/error.rs#L378-L384

and this mapping
https://github.com/exonum/exonum/blob/86379cdee673dfda94b6b1e23ddedb5d512664a7/exonum/src/runtime/error.rs#L386

Support for this may be done in a separate PR — but in that case please document that in Jira properly.

dmitry-timofeev · 2019-11-27T12:15:48Z

exonum-light-client/src/main/java/com/exonum/client/ExplorerApiHelper.java

+        return ExecutionStatuses.success();
      case ERROR:
-        return TransactionResult.error(executionStatus.getCode(),
+        return ExecutionStatuses.serviceError(executionStatus.getCode(),


It is not complete — see below.

dmitry-timofeev · 2019-11-27T12:26:15Z

exonum-light-client/pom.xml

+      <groupId>com.google.protobuf</groupId>
+      <artifactId>protobuf-java</artifactId>
+      <version>${protobuf.version}</version>
+      <scope>provided</scope>


Why this dependency is needed? Doesn't common bring it as a transitive?
It may be specified explicitly, but certainly not as provided. If it isn't already available — please fix the common.

It is explicitly marked as an optional dependency and is not passed on transitively - https://github.com/exonum/exonum-java-binding/blob/master/exonum-java-binding/common/pom.xml#L47

Added a PR that makes protobuf dependency transitive - #1246

exonum-light-client/src/main/lombok/com/exonum/client/response/TransactionResponse.java

bullet-tooth · 2019-11-27T13:32:12Z

exonum-light-client/src/main/java/com/exonum/client/ExplorerApiHelper.java

 import com.exonum.client.response.BlocksResponse;
 import com.exonum.client.response.TransactionResponse;
 import com.exonum.client.response.TransactionStatus;
+import com.exonum.core.messages.Runtime.ExecutionStatus;


how does it come here? Is it a part of exonum-java-binding-common? I mean exonum.core. package

Is it a part of exonum-java-binding-common?

Yes, it is, but, ideally, it mustn't be a part of it, but a dependency of it. I think we shall extract the messages into a separate module (or modules, if we believe that some messages are not needed everywhere, e.g., supervisor messages, or merkledb messages) that will have not just have its own package, but its own release cadence (since the main reason for their release is a new Exonum release).

dmitry-timofeev · 2019-11-29T14:19:47Z

exonum-light-client/src/test/java/com/exonum/client/ExplorerApiHelperTest.java

+    String errorDescription = "Unexpected error";
+    String json = "{\n"
+        + "    'type': 'committed',\n"
+        + "    'content': {\n"


There is no content.debug in 0.13, please check the format in the core.

dmitry-timofeev · 2019-11-29T14:25:34Z

exonum-light-client/src/test/java/com/exonum/client/ExplorerApiHelperTest.java

+        + "    'status': {\n"
+        + "        'type': 'panic',\n"
+        + "        'description': \"" + errorDescription + "\""
+        + "    }\n"


This is the only variable thing in this and the following tests. It seems like the main and/or test code have to be refactored to not duplicate the rest needlesly. E.g.,

A template JSON for testing various results + a source of (JSONs, expected execution status) + a parameterized test asserting on the expected status (and the constants, if needed).

A modification of main code that allows to test the various results independently from the transaction response.

dmitry-timofeev · 2019-12-02T11:07:34Z

exonum-light-client/src/test/java/com/exonum/client/ExplorerApiHelperTest.java

+        + "}\n";
+
+    return Stream.of(
+        Arguments.of(ExecutionStatuses.success(), successStatus),


Arguments.arguments(...) (statically-imported)

MakarovS · 2019-12-02T22:26:23Z

exonum-light-client/src/main/java/com/exonum/client/ExplorerApiHelper.java

    TransactionLocation location;
    JsonObject locationProof; //TODO: in scope of LC P3
    GetTxResponseExecutionResult status;
+    String time;


@dmitry-timofeev should we add this time to the TransactionResponse? If yes, I'll create a Jira task

Yes, I'd go with it. But if we don't add it here, please remove the String time field — it won't affect the JSON parsing.

Added - ECR-3942

MakarovS · 2019-12-03T04:20:21Z

exonum-light-client/src/test/java/com/exonum/client/ExplorerApiHelperTest.java

+      + "        'block_height': " + BLOCK_HEIGHT + ",\n"
+      + "        'position_in_block': " + INDEX_IN_BLOCK + "\n"
+      + "    },\n"
+      + "    'location_proof': {\n"


I took this location_proof from some core tests. It looks weird, but we don't use it in any way, so as long as it's a valid json, we are good.

dmitry-timofeev · 2019-12-03T10:07:51Z

exonum-light-client/pom.xml

    <!-- Project dependencies -->
    <okhttp.version>4.2.2</okhttp.version>
    <lombok.version>1.18.10</lombok.version>
+    <protobuf.version>3.10.0</protobuf.version>


ECR-3860 Update LC with DS changes

b35a48e

MakarovS requested review from bullet-tooth and dmitry-timofeev November 27, 2019 01:10

Merge remote-tracking branch 'origin/master' into ECR-3860

76b4050

dmitry-timofeev suggested changes Nov 27, 2019

View reviewed changes

bullet-tooth reviewed Nov 27, 2019

View reviewed changes

ECR-3860 Review fixes

dbc08c9

MakarovS mentioned this pull request Nov 27, 2019

Make protobuf dependency in common transitive #1246

Merged

3 tasks

MakarovS added 3 commits November 27, 2019 20:21

ECR-3860 Minor changelog fix

589f861

ECR-3860 Add other possible execution statuses

cc470a0

Merge remote-tracking branch 'origin/master' into ECR-3860

9fb0e60

dmitry-timofeev reviewed Nov 29, 2019

View reviewed changes

ECR-3860 Refactor ExplorerApiHelperTest

453c623

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

dmitry-timofeev reviewed Dec 2, 2019

View reviewed changes

MakarovS added 3 commits December 2, 2019 14:36

ECR-3860 Use arguments method

9c1a4a8

ECR-3860 Change Transaction format

dfde8ce

Merge remote-tracking branch 'remotes/origin/master' into ECR-3860

003a8e1

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

MakarovS commented Dec 2, 2019

View reviewed changes

MakarovS commented Dec 3, 2019

View reviewed changes

dmitry-timofeev approved these changes Dec 3, 2019

View reviewed changes

MakarovS added 2 commits December 3, 2019 12:05

ECR-3860 Remove time and redundant pom property

2bff988

Merge remote-tracking branch 'remotes/origin/master' into ECR-3860

41d708e

dmitry-timofeev merged commit bd5b362 into master Dec 4, 2019

MakarovS deleted the ECR-3860 branch December 4, 2019 18:30

Update LC with DS changes [ECR-3860] #1244

Update LC with DS changes [ECR-3860] #1244

Uh oh!

Conversation

MakarovS commented Nov 27, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Overview

Definition of Done

Uh oh!

coveralls commented Nov 27, 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!

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!

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!

Uh oh!

MakarovS commented Nov 27, 2019 •

edited

Loading

coveralls commented Nov 27, 2019 •

edited

Loading