Add serviceName and serviceId to TransactionContext [ECR-3639] #1181

MakarovS · 2019-10-23T23:07:53Z

Overview

Add serviceName and serviceId to TransactionContext.

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

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-10-23T23:43:29Z

Coverage increased (+0.2%) to 86.177% when pulling 89e85c3 on ECR-3639 into 892596a on dynamic-services.

dmitry-timofeev · 2019-10-24T12:25:32Z

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

@@ -157,10 +157,13 @@ void executeTransaction(int serviceId, int txId, byte[] arguments,
      Fork fork = viewFactory.createFork(forkNativeHandle, cleaner);
      HashCode hash = HashCode.fromBytes(txMessageHash);
      PublicKey authorPk = PublicKey.fromBytes(authorPublicKey);
+      String serviceName = serviceRuntime.getServiceNameById(serviceId);


I remember we discussed to push the instantiation of TransactionContext inside the runtime because it already has this knowledge, the adapter will not have to request if from the runtime (and to be appropriately tested), and the runtime — provide an accessor for that. Is there a reason for the other approach?

dmitry-timofeev · 2019-10-24T12:32:20Z

Q: Do we actually need to include service id? Shouldn't service name be sufficient for our purposes, as it's always known to the user (unlike service id)?

The present TransactionConverter does not include the service id (com.exonum.binding.core.service.TransactionConverter#toTransaction), so if the transaction execution logic does need the id in some operations, the converter will have to be supplied with that id, or the tx execution logic will have to access it elsewhere.

dmitry-timofeev · 2019-10-25T09:44:52Z

...nding/core/src/main/java/com/exonum/binding/core/transaction/InternalTransactionContext.java

@@ -21,6 +21,7 @@
 import com.exonum.binding.common.crypto.PublicKey;
 import com.exonum.binding.common.hash.HashCode;
 import com.exonum.binding.core.storage.database.Fork;
+import java.util.Objects;

 /**
 * Default implementation of the transaction context.


Would it make sense to use @AutoValue here?

Sure, added @AutoValue.

...nding/core/src/main/java/com/exonum/binding/core/transaction/InternalTransactionContext.java

bullet-tooth · 2019-10-28T08:08:51Z

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

@@ -146,7 +146,8 @@ void stopService(int id) {
   * @param txMessageHash the hash of the transaction message
   * @param authorPublicKey the public key of the transaction author
   * @throws TransactionExecutionException if the transaction execution failed
-   * @see ServiceRuntime#executeTransaction(Integer, int, byte[], TransactionContext)
+   * @see ServiceRuntime#executeTransaction(Integer, int, byte[], TransactionContext, Fork,


Suggested change

* @see ServiceRuntime#executeTransaction(Integer, int, byte[], TransactionContext, Fork,

* @see ServiceRuntime#executeTransaction(Integer, int, byte[], Fork,

bullet-tooth · 2019-10-28T08:12:02Z

...-java-binding/core/src/main/java/com/exonum/binding/core/transaction/TransactionContext.java

+   *
+   * @see TransactionMessage#getServiceId()
+   */
+  Integer getServiceId();


can it be null? If not why doesn't int?

It can't (and having null Integer wouldn't be friendly), must be int, here and elsewhere.

I intentionally used Integer here, so that if serviceId isn't specified, NPE would be thrown by AutoValue when creating an instance. Opposed to int, which would just set this value to 0 by default, which is error-prone. Should I keep the Integer or change it to int and validate that it was set or change it to int and not bother with that?

But the builder checks (or shall check) that, doesn't it?

The builder implementation nuances (that it uses Integer to distinguish between a set and unset value) must not affect the API.

dmitry-timofeev · 2019-10-28T11:48:58Z

...-java-binding/core/src/main/java/com/exonum/binding/core/transaction/TransactionContext.java

+   *
+   * @see TransactionMessage#getServiceId()
+   */
+  Integer getServiceId();


It can't (and having null Integer wouldn't be friendly), must be int, here and elsewhere.

dmitry-timofeev · 2019-10-28T11:49:14Z

...nding/core/src/main/java/com/exonum/binding/core/transaction/InternalTransactionContext.java

+
+  public static InternalTransactionContext newInstance(Fork fork, HashCode hash,
+                                                       PublicKey authorPk, String serviceName,
+                                                       Integer serviceId) {


Also, int here.

dmitry-timofeev · 2019-10-28T11:49:30Z

...nding/core/src/main/java/com/exonum/binding/core/transaction/InternalTransactionContext.java

-  public Fork getFork() {
-    return fork;
-  }
+  public abstract Fork getFork();


They are already abstract and can be removed, AutoValue will infer them from the interface.

ECR-3639 Add serviceName and serviceId to TransactionContext

2e73a01

MakarovS requested review from bullet-tooth and dmitry-timofeev October 23, 2019 23:07

ECR-3639 Minor fixes

7a4d2a4

dmitry-timofeev reviewed Oct 24, 2019

View reviewed changes

MakarovS added 2 commits October 24, 2019 17:35

ECR-3639 Move TransactionContext instantiation to ServiceRuntime

4bdbc23

ECR-3639 Checkstyle fix

9751fb4

dmitry-timofeev approved these changes Oct 25, 2019

View reviewed changes

MakarovS added 2 commits October 25, 2019 23:53

ECR-3639 Use @autovalue

2b352e0

ECR-3639 Minor fix

105a144

bullet-tooth approved these changes Oct 28, 2019

View reviewed changes

dmitry-timofeev reviewed Oct 28, 2019

View reviewed changes

MakarovS added 3 commits October 28, 2019 20:22

ECR-3639 Review fixes

8e6f123

ECR-3639 Change serviceId type to int

7e14650

ECR-3639 Import fix

67b2808

dmitry-timofeev approved these changes Oct 29, 2019

View reviewed changes

Merge branch 'dynamic-services' into ECR-3639

89e85c3

MakarovS merged commit d627798 into dynamic-services Oct 30, 2019

dmitry-timofeev deleted the ECR-3639 branch October 30, 2019 19:02

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add serviceName and serviceId to TransactionContext [ECR-3639] #1181

Add serviceName and serviceId to TransactionContext [ECR-3639] #1181

MakarovS commented Oct 23, 2019 •

edited

Loading

coveralls commented Oct 23, 2019 •

edited

Loading

dmitry-timofeev Oct 24, 2019

dmitry-timofeev commented Oct 24, 2019

dmitry-timofeev Oct 25, 2019

MakarovS Oct 26, 2019

bullet-tooth Oct 28, 2019

bullet-tooth Oct 28, 2019

dmitry-timofeev Oct 28, 2019

MakarovS Oct 29, 2019

dmitry-timofeev Oct 29, 2019

dmitry-timofeev Oct 29, 2019

dmitry-timofeev Oct 28, 2019

dmitry-timofeev Oct 28, 2019

dmitry-timofeev Oct 28, 2019

	* @see ServiceRuntime#executeTransaction(Integer, int, byte[], TransactionContext, Fork,
	* @see ServiceRuntime#executeTransaction(Integer, int, byte[], Fork,

Add serviceName and serviceId to TransactionContext [ECR-3639] #1181

Add serviceName and serviceId to TransactionContext [ECR-3639] #1181

Conversation

MakarovS commented Oct 23, 2019 • edited Loading

Overview

Definition of Done

coveralls commented Oct 23, 2019 • edited Loading

Choose a reason for hiding this comment

dmitry-timofeev commented Oct 24, 2019

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

MakarovS commented Oct 23, 2019 •

edited

Loading

coveralls commented Oct 23, 2019 •

edited

Loading