Support configure interface [ECR-3590] - #1234
Conversation
Add Configurable interface corresponding to `exonum.Configure` and its support to ServiceWrapper
Also implement caller authorization
| /** | ||
| * 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)? |
There was a problem hiding this comment.
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".
There was a problem hiding this comment.
"implicit" will probably highlight that there is no explicit definition, but I don't think that is what must be communicated.
| * @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 |
There was a problem hiding this comment.
nit: missing closing brace
There was a problem hiding this comment.
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).
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.)
There was a problem hiding this comment.
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.
| break; | ||
| } | ||
| default: throw new IllegalArgumentException( | ||
| format("Unknown interface (name=%s, txId=%d)", interfaceName, txId)); |
There was a problem hiding this comment.
Why static import for String.format? That's inconsistent with the rest of our codebase
There was a problem hiding this comment.
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.
| * <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 |
There was a problem hiding this comment.
The protocol ... is determined
There was a problem hiding this comment.
You are right, my mistake
| 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 = ""; |
There was a problem hiding this comment.
Why not ServiceWrapper.DEFAULT_INTERFACE_NAME?
Overview
Add Configurable interface corresponding to
exonum.Configureand its support to ServiceRuntime.
See: https://jira.bf.local/browse/ECR-3590
Definition of Done