Support configure interface [ECR-3590] #1234

dmitry-timofeev · 2019-11-22T14:12:22Z

Overview

Add Configurable interface corresponding to exonum.Configure
and its support to ServiceRuntime.

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

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

Add Configurable interface corresponding to `exonum.Configure` and its support to ServiceWrapper

Also implement caller authorization

dmitry-timofeev · 2019-11-22T14:19:22Z

exonum-java-binding/core/src/main/java/com/exonum/binding/core/runtime/ServiceWrapper.java

+  /**
+   * Default interface comprised of transactions defined in the service implementation
+   * (intrinsic to this service).
+   * todo: @slowli What's the proper term for this interface (intrinsic, implicit, default)?


@slowli Is there one?

Pink @slowli

Sorry, didn't notice the ping. The core uses "default", however, I'd not consider this a 100% settled term. Do you think another term from the list is better? IMO, there's a tie between "default" and "intrinsic".

TIL default is not an adjective.

I agree that both make sense: intrinsic says that this interface belongs to, or is a part of a service artifact (with no extra definition elsewhere); default says that that interface is used when no interface is specified.

"implicit" will probably highlight that there is no explicit definition, but I don't think that is what must be communicated.

coveralls · 2019-11-22T16:53:42Z

Coverage increased (+0.07%) to 85.968% when pulling 6df6226 on dmitry-timofeev:support-configure-interface into 539450b on exonum:master.

vitvakatu · 2019-11-24T09:42:10Z

...m-java-binding/core/src/main/java/com/exonum/binding/core/runtime/ServiceRuntimeAdapter.java

   * @param arguments the transaction arguments
   * @param forkNativeHandle a handle to a native fork object
+   * @param callerServiceId the id of the service which invoked the transaction (in case of
+   *      inner transactions; or 0 when the caller is an external message


nit: missing closing brace

Also, I can't find any mention that 0 can't be a service ID. Doesn't -1 suit better in this case?

update: 0 is a supervisor service's ID, but supervisor service isn't strictly essential for blockchain operations (or it shouldn't be, at least).

0 here doesn't matter as it isn't supposed to be used in the case, same as hash code and public key in the other case (which are all zeroes). Whether a transaction is external is determined by the interface name currently. It could be set to something different to have something different from the supervisor id, but I hope this signature won't last long.

Whether a transaction is external is determined by the interface name currently.

The keyword is "currently." I'm not 100% sure that we'll keep the inability of external clients to call non-default service interface in 1.0, but this restriction will be lifted eventually. In this case the current implementation seemingly becomes dangerous: any transaction from the external client will be considered to be authorized by the supervisor! (I'm sure it'll be fixed by that time.)

The thing is that what is currently available is very limited and I don't expect this implementation to last (particularly, this monstrous parameter list). It just doesn't support (and explicitly forbids) anything but 'exonum.Configure'. Supporting anything else will require changes.

MakarovS · 2019-11-25T05:39:51Z

exonum-java-binding/core/src/main/java/com/exonum/binding/core/runtime/ServiceWrapper.java

+        break;
+      }
+      default: throw new IllegalArgumentException(
+          format("Unknown interface (name=%s, txId=%d)", interfaceName, txId));


Why static import for String.format? That's inconsistent with the rest of our codebase

To have it more consice and fit in a single expression. I wouldn't consider the consistency of static imports of a particular method as something to be given higher priority over readability and local consistency.

MakarovS · 2019-11-25T05:42:11Z

exonum-java-binding/core/src/main/java/com/exonum/binding/core/service/Configurable.java

+ * <p>The configuration update process includes the following steps: a proposal
+ * of a new configuration; verification of its correctness; approval of the proposal;
+ * and application of the new configuration. The protocol of the proposal and approval steps
+ * is determined by the installed supervisor service. The verification and application


are determined?

The protocol ... is determined

You are right, my mistake

MakarovS · 2019-11-25T05:52:21Z

exonum-java-binding/core/src/test/java/com/exonum/binding/core/runtime/ServiceWrapperTest.java

      ServiceArtifactId.newJavaId("com.acme:foo:1.2.3");
+  private static final String TEST_SERVICE_NAME = "test-service";
+  private static final int TEST_SERVICE_ID = 1;
+  private static final String DEFAULT_INTERFACE = "";


Why not ServiceWrapper.DEFAULT_INTERFACE_NAME?

…erface

dmitry-timofeev added 3 commits November 22, 2019 16:10

Add Configurable interface [ECR-3590]:

b9d5764

Add Configurable interface corresponding to `exonum.Configure` and its support to ServiceWrapper

Pass the required parameters from the native to the wrapper;

3a8f244

Also implement caller authorization

Clarify the config update process

1a3bb23

dmitry-timofeev commented Nov 22, 2019

View reviewed changes

dmitry-timofeev requested review from MakarovS, bullet-tooth, vitvakatu and slowli November 22, 2019 14:19

Fix spotbugs

89e7b88

vitvakatu reviewed Nov 24, 2019

View reviewed changes

MakarovS approved these changes Nov 25, 2019

View reviewed changes

dmitry-timofeev added 6 commits November 25, 2019 16:44

Fix parentheses

d0b2495

Use the constant from the SuT

627cf63

Merge branch 'master' into support-configure-interface

6df6226

Clarify the todo and hide it under comment.

ff3f2a3

Remove the todo for now.

00f6d37

Merge remote-tracking branch 'main/master' into support-configure-int…

5877a3e

…erface

dmitry-timofeev merged commit 92de342 into exonum:master Dec 2, 2019

dmitry-timofeev deleted the support-configure-interface branch December 2, 2019 17:54

Support configure interface [ECR-3590] #1234

Support configure interface [ECR-3590] #1234

Uh oh!

Conversation

dmitry-timofeev commented Nov 22, 2019

Overview

Definition of Done

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!

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

Uh oh!

coveralls commented Nov 22, 2019 •

edited

Loading