FABN-1544 Update gRPC to pure js version #247
Conversation
|
/azp run |
|
Azure Pipelines successfully started running 1 pipeline(s). |
There was a problem hiding this comment.
Lots of comments. Many of them I think are worth tweaking. The only showstopper is the change to the types on Checkpointer methods. Since we know certain properties will exist at runtime, you might just need to assert that properties exist in the internal event handling code when they are calling to the checkpointer to make this work if the event typings allow things to be undefined, e.g.:
checkpointer.addTransactionId(txId!);| return filteredTransactions.map((tx) => newFilteredTransactionEvent(blockEvent, tx)); | ||
| } | ||
|
|
||
| function newFilteredTransactionEvent(blockEvent: BlockEvent, filteredTransaction: FilteredTransaction): TransactionEvent { | ||
| function newFilteredTransactionEvent(blockEvent: BlockEvent, filteredTransaction: fabproto6.protos.IFilteredTransaction): TransactionEvent { | ||
| const status = TransactionStatus.getStatusForCode(filteredTransaction.tx_validation_code); |
There was a problem hiding this comment.
We know there will be a transaction validation code at runtime, even if the generated types allow them not to be. We could make our life much easier in TransactionStatus.getStatusForCode() by telling the TypeScript compiler there will be a value:
const status = TransactionStatus.getStatusForCode(filteredTransaction.tx_validation_code!);| const status = codeToStatusMap[code]; | ||
| if (!status) { | ||
| throw new Error('Unexpected transaction status code: ' + code); | ||
| export function getStatusForCode(code: fabproto6.protos.TxValidationCode | null | undefined): string { |
There was a problem hiding this comment.
I think we should fix the typing issue differently to keep the code simpler and more maintainable, since we have better information about what is really there at runtime. This code works fine, even if the code turns out to be null or undefined at runtime.
export function getStatusForCode(code: fabproto6.protos.TxValidationCode): string {
const status = fabproto6.protos.TxValidationCode[code];
if (!status) {
throw new Error('Unexpected transaction status code: ' + code);
}
return status;
}| @@ -242,8 +242,8 @@ export interface EventInfo { | |||
| status?: string; | |||
| endBlockReceived?: boolean; | |||
| chaincodeEvents?: ChaincodeEvent[]; | |||
| block?: Block; | |||
| filteredBlock?: FilteredBlock; | |||
| block?: fabproto6.common.IBlock; | |||
There was a problem hiding this comment.
Need to be careful here as the deserialized objects passed back are not the same as the protobuf TypeScript deifnitions
There was a problem hiding this comment.
yes, a little miss leading
| @@ -5,12 +5,12 @@ | |||
| */ | |||
|
|
|||
| import sinon = require('sinon'); | |||
| import chai = require('chai'); | |||
| const expect = chai.expect; | |||
| import 'mocha'; | |||
There was a problem hiding this comment.
This doesn't look correct
There was a problem hiding this comment.
removed, it was used to allow for debug under vscode
| channelHeader.tx_id = transactionId; | ||
|
|
||
| const payload = new protos.common.Payload(); | ||
| payload.header = new protos.common.Header(); | ||
| const payload: any = {}; |
There was a problem hiding this comment.
Might be safer to stick with the protobuf definitions rather than any and just cast values we're assigning to properties to any type at assignment where the object representation doesn't match the protobuf TypeScript. At least the typing will ensure we get the correct property names that way
| @@ -11,7 +11,7 @@ const DiscoveryService = require('./DiscoveryService.js'); | |||
| const Endorsement = require('./Endorsement.js'); | |||
| const Commit = require('./Commit.js'); | |||
| const Query = require('./Query.js'); | |||
| const fabprotos = require('fabric-protos'); | |||
| const fabproto6 = require('fabric-protos'); | |||
There was a problem hiding this comment.
What is the special reason for the '6' suffix?
There was a problem hiding this comment.
the conversion is based on protobufjs v6
| block_header.previous_hash = proto_block_header.getPreviousHash().toBuffer().toString('hex'); | ||
| block_header.data_hash = proto_block_header.getDataHash().toBuffer().toString('hex'); | ||
| block_header.number = blockHeaderProto.number; | ||
| block_header.previous_hash = blockHeaderProto.previous_hash; |
There was a problem hiding this comment.
Does it mean we transfer the responsibility to developer about how to visualize this?
There was a problem hiding this comment.
yes, we should not be making that decision, this way they get the field as defined in the protobuf
| } else { | ||
| throw new Error('unknown signature policy type'); |
There was a problem hiding this comment.
This deletion inspire me that, we suffer a lot like this.
There are some error handler which actually should be assertion, with cases rarely happen.
There are many error catcher simply return a message wrapping original error message, without taking original error stack into concern
For both case it is better to expose those fatal error to developer. instead of handle them as take it for granted
| function decodeChaincodeID(proto_chaincode_id) { | ||
| function decodeResponse(responseProto) { | ||
| if (responseProto) { | ||
| const response = {}; |
There was a problem hiding this comment.
const {status,message,payload } = responseProto
return {status,message,payload }
There was a problem hiding this comment.
Yes we could do that, we should do that with an overall clean up of this class
| chaincode_id.path = proto_chaincode_id.getPath(); | ||
| chaincode_id.name = proto_chaincode_id.getName(); | ||
| chaincode_id.version = proto_chaincode_id.getVersion(); | ||
| chaincode_id.path = chaincodeIDProto.path; |
There was a problem hiding this comment.
same as above, or could we return the chaincodeIDProto directly? What else is inside that proto than these properties?
There was a problem hiding this comment.
Yes we could do that, we should do that with an overall clean up of this class
| } | ||
|
|
||
| return range_query_info; | ||
| } | ||
|
|
||
| function decodeKVWrite(proto_kv_write) { | ||
| function decodeKVWrite(kVWriteProto) { |
There was a problem hiding this comment.
Yes we could do that, we should do that with an overall clean up of this class
Replace the grpc package with a pure javascript implementation. The change involves changing the grpc message building and decoding. Include and use the generated typescript for the protobufs. Signed-off-by: Bret Harrison <beharrison@nc.rr.com>
Replace the grpc package with a pure javascript implementation.
The change involves changing the grpc message building and decoding.
Include and use the generated typescript for the protobufs.
Signed-off-by: Bret Harrison beharrison@nc.rr.com