Support DS changes in cryptocurrency module [ECR-3587] #1196

MakarovS · 2019-10-31T01:51:40Z

Overview

Support DS changes in cryptocurrency module.

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

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

…CR-3571

Add configuration usage in TestService initialization and update TestService initialization.

…CR-3571

…CR-3587

coveralls · 2019-11-02T03:53:46Z

Coverage decreased (-0.5%) to 85.602% when pulling 0f7dd2f on ECR-3587 into a57c0a5 on master.

dmitry-timofeev · 2019-11-05T11:38:08Z

exonum-java-binding/cryptocurrency-demo/pom.xml

+          <systemPropertyVariables>
+            <it.artifactFilename>${artifactFilename}.jar</it.artifactFilename>
+            <it.artifactId>${groupId}:${artifactId}:${version}</it.artifactId>
+            <it.serviceName>cryptocurrency-demo</it.serviceName>


Why is that specified in POM?

dmitry-timofeev · 2019-11-05T11:39:13Z

exonum-java-binding/cryptocurrency-demo/pom.xml

+            <it.artifactId>${groupId}:${artifactId}:${version}</it.artifactId>
+            <it.serviceName>cryptocurrency-demo</it.serviceName>
+            <it.serviceId>46</it.serviceId>
+            <it.artifactsDirectory>target</it.artifactsDirectory>


Please use the standard variable ${project.build.directory} so that it works regardless of the current directory.

dmitry-timofeev · 2019-11-05T11:39:59Z

...ryptocurrency-demo/src/main/java/com/exonum/binding/cryptocurrency/CryptocurrencySchema.java

-  public CryptocurrencySchema(View view) {
+  public CryptocurrencySchema(View view, String serviceName) {
    this.view = checkNotNull(view);
+    this.namespace = "cryptocurrency_service_namespace." + serviceName + ".";


Why isn't already unique service name enough for a namespace?

dmitry-timofeev · 2019-11-05T11:41:05Z

...currency-demo/src/main/java/com/exonum/binding/cryptocurrency/CryptocurrencyServiceImpl.java


    return node.withSnapshot((view) -> {
-      CryptocurrencySchema schema = new CryptocurrencySchema(view);
+      CryptocurrencySchema schema = new CryptocurrencySchema(view, serviceInstanceName);


createDataSchema(view) -> CryptocurrencySchema

dmitry-timofeev · 2019-11-05T11:42:24Z

...rrency-demo/src/main/java/com/exonum/binding/cryptocurrency/transactions/CreateWalletTx.java

   */
-  static CreateWalletTx fromRawTransaction(RawTransaction rawTransaction) {
-    checkTransaction(rawTransaction, ID);
+  static CreateWalletTx from(int txId, byte[] arguments) {


Why does it accept the id if it isn't needed?

dmitry-timofeev · 2019-11-05T11:46:24Z

...ency-demo/src/test/java/com/exonum/binding/cryptocurrency/transactions/TransactionUtils.java

-            .build()
-            .toByteArray())
-        .build();
+  static byte[] createCreateWalletTxPayload(long initialBalance) {


new seems to be a more appropriate prefix in this case.

dmitry-timofeev · 2019-11-05T11:51:40Z

...mmon/src/main/java/com/exonum/binding/common/serialization/ProtobufReflectiveSerializer.java

  ProtobufReflectiveSerializer(Class<MessageT> messageType) {
-    MethodHandles.Lookup lookup = MethodHandles.publicLookup();
+    MethodHandles.Lookup lookup = MethodHandles.publicLookup()
+        .in(messageType);


Such implementation must be clarified.

dmitry-timofeev · 2019-11-07T12:31:15Z

exonum-java-binding/cryptocurrency-demo/pom.xml

        <configuration>
          <argLine>
            ${jacoco.args}
            -Djava.library.path=${ejb-core.nativeLibPath}


Is it still needed?

No, removed it, thanks

bullet-tooth · 2019-11-07T16:08:32Z

.../test/java/com/exonum/binding/cryptocurrency/transactions/CreateWalletTxIntegrationTest.java


  @Test
  @RequiresNativeLibrary
+  @Disabled("Disabled until transaction results fix")


add ticket number if any

Transaction result is already in (#1174)

Enabled these

dmitry-timofeev · 2019-11-08T14:22:45Z

...mmon/src/main/java/com/exonum/binding/common/serialization/ProtobufReflectiveSerializer.java

  ProtobufReflectiveSerializer(Class<MessageT> messageType) {
-    MethodHandles.Lookup lookup = MethodHandles.publicLookup();
+    MethodHandles.Lookup lookup = MethodHandles.publicLookup()
+        .in(messageType);


Such implementation must be clarified.

dmitry-timofeev

Looks good, the build is red though.

MakarovS added 30 commits October 1, 2019 02:46

ECR-3571 Add dynamic services support in TestKit

5fc4b9b

ECR-3571 Update tests

33b87e8

ECR-3571 Remove redundant files

594213c

ECR-3571 Fix test

e65d021

ECR-3571 Review fixes

5042a28

Merge remote-tracking branch 'remotes/origin/dynamic-services' into E…

a5606fb

…CR-3571

ECR-3571 Use MessageLite

8f6c30b

ECR-3571 Minor fix

3b2331e

ECR-3571 Add a comment

afa48a2

ECR-3571 Review fixes (WIP)

f91d753

ECR-3571 Review fixes

8bd9a8c

ECR-3571 Minor fixes

3ab7116

ECR-3571 Test fixes

70b9ea2

ECR-3571 Review fixes

cd27376

ECR-3571 Minor fixes

dc654e1

ECR-3571 Remove server port, use app as a dependency

62b23b2

ECR-3571 Fix tests

cfc7431

ECR-3571 Add testing dependency

949a4f7

ECR-3571 Remove multiple time services, review fixes

98c0757

ECR-3571 Fix service names

1094d56

ECR-3571 Fix tests and change service instance spec

ec7fd10

ECR-3571 Checkstyle fixes

5f70ec5

ECR-3571 Various minor fixes

74f0f75

Add configuration usage in TestService initialization [ECR-3652] (#1166)

8521b39

Add configuration usage in TestService initialization and update TestService initialization.

Merge remote-tracking branch 'remotes/origin/dynamic-services' into E…

ea116e0

…CR-3571

Merge remote-tracking branch 'remotes/origin/ECR-3571' into ECR-3571

2dc0c14

Merge remote-tracking branch 'remotes/origin/dynamic-services' into E…

7b4b160

…CR-3571

Remove getService() from TestKit (#1184)

8e864a7

Merge remote-tracking branch 'remotes/origin/dynamic-services' into E…

533d19a

…CR-3571

Merge remote-tracking branch 'remotes/origin/ECR-3571' into ECR-3571

1002090

MakarovS requested review from bullet-tooth and dmitry-timofeev October 31, 2019 01:51

MakarovS added 5 commits October 30, 2019 21:52

ECR-3587 Minor fixes

15bfa72

Merge remote-tracking branch 'remotes/origin/dynamic-services' into E…

1b31cb5

…CR-3571

ECR-3571 Revert spotbugs fix

651f539

Merge remote-tracking branch 'remotes/origin/ECR-3571' into ECR-3587

5285b08

ECR-3587 Fix tests

edd7713

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

MakarovS changed the base branch from ECR-3571 to dynamic-services November 2, 2019 03:14

Merge remote-tracking branch 'remotes/origin/dynamic-services' into E…

d4a6ada

…CR-3587

dmitry-timofeev suggested changes Nov 5, 2019

View reviewed changes

MakarovS added 3 commits November 6, 2019 19:41

ECR-3587 Review fixes

ff32f8f

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

19e032c

ECR-3587 Minor fix

43ef512

MakarovS changed the base branch from dynamic-services to master November 7, 2019 04:53

ECR-3587 Fix checkstyle

aae42bf

dmitry-timofeev reviewed Nov 7, 2019

View reviewed changes

dmitry-timofeev approved these changes Nov 7, 2019

View reviewed changes

bullet-tooth approved these changes Nov 7, 2019

View reviewed changes

MakarovS and others added 2 commits November 7, 2019 23:43

ECR-3587 Enable all cryptocurrency tests

e1380e6

Build cryptocurrency-demo

eedfe91

dmitry-timofeev suggested changes Nov 8, 2019

View reviewed changes

MakarovS added 2 commits November 13, 2019 01:21

ECR-3587 Add a comment in ProtobufReflectiveSerializer

b923e89

Merge remote-tracking branch 'remotes/origin/ECR-3587' into ECR-3587

9c702b5

dmitry-timofeev approved these changes Nov 13, 2019

View reviewed changes

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

0f7dd2f

dmitry-timofeev approved these changes Nov 14, 2019

View reviewed changes

dmitry-timofeev merged commit e02da8b into master Nov 14, 2019

dmitry-timofeev deleted the ECR-3587 branch November 14, 2019 17:32

Support DS changes in cryptocurrency module [ECR-3587] #1196

Support DS changes in cryptocurrency module [ECR-3587] #1196

Uh oh!

Conversation

MakarovS commented Oct 31, 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 2, 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!

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 left a comment

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Milestone

Development

Uh oh!

4 participants

MakarovS commented Oct 31, 2019 •

edited

Loading

coveralls commented Nov 2, 2019 •

edited

Loading