Add serviceName and serviceId to TransactionContext [ECR-3639] - #1181
Conversation
| Fork fork = viewFactory.createFork(forkNativeHandle, cleaner); | ||
| HashCode hash = HashCode.fromBytes(txMessageHash); | ||
| PublicKey authorPk = PublicKey.fromBytes(authorPublicKey); | ||
| String serviceName = serviceRuntime.getServiceNameById(serviceId); |
There was a problem hiding this comment.
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?
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. |
| import java.util.Objects; | ||
|
|
||
| /** | ||
| * Default implementation of the transaction context. |
There was a problem hiding this comment.
Would it make sense to use @AutoValue here?
There was a problem hiding this comment.
Sure, added @AutoValue.
| * @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, |
There was a problem hiding this comment.
| * @see ServiceRuntime#executeTransaction(Integer, int, byte[], TransactionContext, Fork, | |
| * @see ServiceRuntime#executeTransaction(Integer, int, byte[], Fork, |
| * | ||
| * @see TransactionMessage#getServiceId() | ||
| */ | ||
| Integer getServiceId(); |
There was a problem hiding this comment.
can it be null? If not why doesn't int?
There was a problem hiding this comment.
It can't (and having null Integer wouldn't be friendly), must be int, here and elsewhere.
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
But the builder checks (or shall check) that, doesn't it?
There was a problem hiding this comment.
The builder implementation nuances (that it uses Integer to distinguish between a set and unset value) must not affect the API.
| * | ||
| * @see TransactionMessage#getServiceId() | ||
| */ | ||
| Integer getServiceId(); |
There was a problem hiding this comment.
It can't (and having null Integer wouldn't be friendly), must be int, here and elsewhere.
|
|
||
| public static InternalTransactionContext newInstance(Fork fork, HashCode hash, | ||
| PublicKey authorPk, String serviceName, | ||
| Integer serviceId) { |
| public Fork getFork() { | ||
| return fork; | ||
| } | ||
| public abstract Fork getFork(); |
There was a problem hiding this comment.
They are already abstract and can be removed, AutoValue will infer them from the interface.
Overview
Add serviceName and serviceId to TransactionContext.
See: https://jira.bf.local/browse/ECR-3639
Definition of Done