Skip to content

Conversation

MakarovS
Copy link
Contributor

@MakarovS MakarovS commented Dec 18, 2019

Overview

Add protobuf type as a transaction method parameter.


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

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

checkArgument(transactionMethods.containsKey(transactionId),
"No method with transaction id (%s)", transactionId);
TransactionMethodObject transactionMethodObject = transactionMethods.get(transactionId);
Optional<Serializer> argumentsSerializer = transactionMethodObject.getArgumentsSerializer();
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

TransactionMethodObject just exposes all of its properties. I suggest moving
the serialization logic into that class to separate responsibilities (routing vs
parameter resolution, mapping and invocation).

* transaction, possibly modifying the blockchain state. The method should:
* <ul>
* <li>be public
* <li>have exactly two parameters - the
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This sentence has become too complex, I suggest rewriting as the following:
"<p>The method should be {@code public} and have the following parameters: [as a list] 1. ... 2. ..."

* <ul>
* <li>be public
* <li>have exactly two parameters - the
* {@linkplain TransactionMessage#getPayload() serialized transaction arguments} of type
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

just the 'transaction arguments', either as byte[] or as a protobuf message. Please also add that the protobuf messages will be deserialized using a public static {@code #parseFrom(byte[])} method


private static MethodHandle toMethodHandle(Method method, Lookup lookup) {
private static TransactionMethodObject toTransactionMethodObject(Method method, Lookup lookup) {
Serializer argumentsSerializer = null;
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I suggest specifying explicitly that the Serializer is unbounded <?> — in our case,
may produce any Object in its fromBytes (in TransactionMethodObject API and in the variables declarations)
so that the intent is clear and there are no warnings.


private static MethodHandle toMethodHandle(Method method, Lookup lookup) {
private static TransactionMethodObject toTransactionMethodObject(Method method, Lookup lookup) {
Serializer argumentsSerializer = null;
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

a no-op serializer would be better than null: StandardSerializers.bytes

Method transactionMethod =
ValidServiceProtobufArgument.class.getMethod("transactionMethod",
TestProtoMessages.Point.class, TransactionContext.class);
assertThat(singletonList(transactionMethod))
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The assertion is the other way around: assertthat(actualValue).matchesSomeCondition(conditionParams)

private final MethodHandle methodHandle;
private final Serializer argumentsSerializer;

TransactionMethodObject(MethodHandle methodHandle, @Nullable Serializer argumentsSerializer) {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Serializer<?> and not null — see above.

@coveralls
Copy link

coveralls commented Dec 19, 2019

Coverage Status

Coverage increased (+0.04%) to 86.883% when pulling cf7a094 on ECR-3991 into 856c177 on master.

@dmitry-timofeev dmitry-timofeev merged commit f235e16 into master Dec 20, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Development

Successfully merging this pull request may close these issues.

3 participants