Add new message format support [ECR-3473] - #1085
dmitry-timofeev merged 3 commits into
Conversation
| */ | ||
| final class ParsedTransactionMessage implements TransactionMessage { | ||
|
|
||
| private final Consensus.SignedMessage signedMessage; |
There was a problem hiding this comment.
It's little confusing that we need references to Consensus.SignedMessage and SignedMessage.
| * {@link Consensus.SignedMessage}; or if the payload of the message is not | ||
| * {@link Consensus.ExonumMessage} | ||
| */ | ||
| public static SignedMessage parseFrom(byte[] messageBytes) throws InvalidProtocolBufferException { |
There was a problem hiding this comment.
this class has package private level but methods are public
| import com.google.protobuf.InvalidProtocolBufferException; | ||
|
|
||
| /** | ||
| * A wrapper around {@link Consensus.SignedMessage} protobuf message containing |
There was a problem hiding this comment.
It's not clear what is the purpose of this class and where it can be helpful. As I can see in PassedMessage we can get everything we need from the protobuf message.
There was a problem hiding this comment.
🤷♂ The line below says " which converts protobuf types into internal types." That is its main purpose.
You do need such a class, unless you are fine with such client code:
PublicKey authorPk = PublicKey.fromBytes(message.getAuthor()
.toByteArray());
Can you spot a mistake in this snippet — it compiles, will not throw in runtime, but is incorrect?
There was a problem hiding this comment.
On top of that — it stores ExonumMessage as ByteString, which is even less intuitive to use.
|
|
||
| @Override | ||
| public byte[] getPayload() { | ||
| // todo: Return as ByteString? Add getPayloadAs(MessageType)? |
There was a problem hiding this comment.
Why'd we need to return ByteString here?
There was a problem hiding this comment.
I think that if we encourage the use of protobuf then the payload is likely a serialized protobuf message. A message type can be created both from a byte array and a ByteString. The latter will not require two extra copies. The downside is that we use a protobuf type in a TransactionMessage interface — but as all the messages are in protobuf now, we can probably live with that.
| exonum.Signature signature = 3; | ||
| } | ||
|
|
||
| // List of consensus messages |
There was a problem hiding this comment.
Isn't there a way to reuse same proto messages from core?
There was a problem hiding this comment.
There was a problem hiding this comment.
I added them (and a patch atop of them) as separate commits to be able to re-apply easily (hopefully, to a separate repo and not to the re-imported definitions 🙃 ).
| /** | ||
| * A funnel for ByteStrings which puts their bytes to the sink without copying. | ||
| */ | ||
| enum ByteStringFunnel implements Funnel<ByteString> { |
There was a problem hiding this comment.
Should we create a task for using ByteStringFunnel in places where we currently use
Hashing.defaultHashFunction().newHasher()
.putBytes(v.toByteArray())
.hash();
There was a problem hiding this comment.
I've found only a single such usage, and decided to inline the #getAsReadonlyByteBuffer instead of making the funnel public.
Import protobuf definitions of Exonum messages required to create transaction messages. We need to copy these definitions until a separate repo with them is created: ECR-3472 The definitions are as of exonum/84ead6191173138f96feabadd3033b3f9aa42368 and will be subsequently patched to work well with Java codegen.
Specify the Java package in which the compiler shall place the generated classes. This patch shall be re-applied when messages are updated (copied over again) or when they finally find their home (ECR-3472).
Add support for new protocol buffers-based message format. SignedMessage supports any Exonum message; whilst TransactionMessage (including ParsedTransactionMessage) represent a transaction message and allow to access any properties of the transaction. As transaction payloads (arguments) are often protocol buffer messages, some methods now have protobuf-specific overloads for higher convenience (e.g., TransactionMessage.Builder#payload).
e7a3dde to
d85deb7
Compare
Overview
Support the new, protobuf-based, message format.
There are a couple of questions for discussion marked as todos.
The definitions are as of exonum/exonum@84ead61
and will be subsequently patched to work well with Java codegen. The corresponding commit
may be skipped (b0448c7 )
See: https://jira.bf.local/browse/ECR-3473
Definition of Done