FABN-1722: export BlockDecoder for typescript definition #477

jscode017 · 2021-07-27T05:30:15Z

Signed-off-by: jsjs026 [email protected]

I refer to https://hyperledger.github.io/fabric-sdk-node/release-2.2/BlockDecoder.html
I am not sure I did it right, there are some questions/discussions.

For parameters end with suffix proto, I used type object here
Do I need to do additional things besides modifying the index.d.ts file?

petermetz · 2021-07-27T16:25:29Z

fabric-common/types/index.d.ts

@@ -245,6 +245,15 @@ export class Proposal extends ServiceAction {
 	public compareProposalResponseResults(proposalResponses: any[]): boolean;
 }

+export class BlockDecoder {
+	constructor();
+	public decode(blockBuf: Buffer): fabproto6.common.Block;


@jscode017 These methods are static according to the documentation, please update the type definitions accordingly. https://hyperledger.github.io/fabric-sdk-node/release-2.2/BlockDecoder.html

OK, fix it.

Awesome, cheers!

Signed-off-by: jsjs026 <[email protected]>

petermetz · 2021-07-27T23:57:39Z

Do I need to do additional things besides modifying the index.d.ts file?

@jscode017 I'd also ask you to test if it actually works as expected, here's how you can do that manually as a one-off:

You locate the fabric-common package directory inside the node_modules folder,
then inside there you locate the same index.d.ts file your PR here changes.
You update that index.d.ts manually to have the changes from this PR as well.
You go to the the place in your code where you want to import the BlockDecoder and import it via the module syntax e.g. import { BlockDecoder } from "fabric-common";
Then you see if your code still compiles without issues. If yes, then that's a very successful smoke test, if not then you can just figure out what's wrong based on the compiler errors, fix it in the index.d.ts and finally update your PR here to match that.

jscode017 · 2021-07-28T06:49:10Z

@petermetz
It works when I change the index.d.ts in node-modules!
Or at least the decode() method works.(Since I do not have test codes for other methods).

petermetz · 2021-07-29T20:20:58Z

@jscode017 Good enough for me. Of course the Fabric maintainers will provide the actual review but I wanted to help out with the preliminary as much as possible.

bestbeforetoday

This is a tricky one. If you dig down into the protobuf definitions you will find that there are several points where properties are just Uint8Array. Often this byte data is actually a serialized protobuf structure. What the block decoder is doing is traversing down through the protobuf structure unpacking everything (including the nested serialized protobufs) into plain JavaScript objects, so the return values don't map exactly to any of the protobuf TypeScript definitions.

So, short of hand-crafting TypeScript definitions for the complete unpacked object structures, what can we do? Well the protobuf TypeScript definitions include both implementation classes (e.g. common.Block) and accompanying interfaces (e.g. common.IBlock). The class definitions include lots of protobuf-specific methods that definitely don't exist on the unpacked JavaScript objects returned from the block decoder, so they are the wrong thing to use. The interface definitions only define the protobuf properties so they are pretty close to what you get from the block decoder, with the caveat that they define some properties (like block.data.data) as raw bytes.

Even the protobuf interface definitions are a lot better than no definitions so, unless you are really motivated to hand-craft type definitions for the entire object structure, I would suggest just using those instead of the protobuf implementation class definitions.

bestbeforetoday · 2021-08-12T15:32:58Z

fabric-common/types/index.d.ts

+export class BlockDecoder {
+	constructor();
+	public static decode(blockBuf: Buffer): fabproto6.common.Block;
+	public static decodeBlock(blockProto: object): fabproto6.common.Block;


This will take a fabproto6.common.IBlock as its argument and version of that completely unpacked to a plain JavaScript object... or (a little confusingly) just return an fabproto6.common.IBlock too

bestbeforetoday · 2021-08-12T15:34:04Z

fabric-common/types/index.d.ts

+	constructor();
+	public static decode(blockBuf: Buffer): fabproto6.common.Block;
+	public static decodeBlock(blockProto: object): fabproto6.common.Block;
+	public static decodeFilteredBlock(filteredBlockProto: object): fabproto6.protos.FilteredBlock;


Argument should be protos.IFilteredBlock

bestbeforetoday · 2021-08-12T15:34:40Z

fabric-common/types/index.d.ts

+	public static decode(blockBuf: Buffer): fabproto6.common.Block;
+	public static decodeBlock(blockProto: object): fabproto6.common.Block;
+	public static decodeFilteredBlock(filteredBlockProto: object): fabproto6.protos.FilteredBlock;
+	public static decodeBlockWithPrivateData(BlockAndPrivateData: object): fabproto6.protos.BlockAndPrivateData;


Argument should be protos.IBlockAndPrivateData

petermetz · 2021-08-16T19:16:32Z

@bestbeforetoday If you are interested I could recommend the thesayyn/protoc-gen-ts for generating the Typescript sources from the .proto files. I used it in a recent PR of mine to add gRPC support in the Cactus API server and was very happy with the way it solved my issues.

PR: hyperledger-cacti/cacti#1190

An example of a proto message in TS (generated): https://github.com/hyperledger/cactus/blob/58a3a1c8c836f9b4a0fa09346cb9e42145cdb437/packages/cactus-cmd-api-server/src/main/typescript/generated/proto/protoc-gen-ts/models/health_check_response_pb.ts

bestbeforetoday · 2021-08-27T11:43:11Z

fabric-common/types/index.d.ts

+export class BlockDecoder {
+	constructor();


The BlockDecoder only exposes static methods so it's not really intended to be used as class and instances created with new BlockDecoder(). It might be worth defining it in TypeScript just as a namespace instead of a class with a constructor, and the static methods declared as functions within that namespace. Something like:

declare namespace BlockDecoder { export function decode(blockBuf: Buffer): fabproto6.common.IBlock; }

bestbeforetoday · 2021-09-15T11:09:16Z

@petermetz Thanks for the pointer to thesayyn/protoc-gen-ts. I am currently using improbable-eng/ts-protoc-gen to extend protoc generated output to include TypeScript definitions in the new fabric-gateway client API, which is similar and seems to work fine.

The fabric-protos package, which is published as part of this Node SDK, also includes TypeScript definitions for the generated protobuf implementations. These are generated using protobuf.js and are a little different in structure to the implementations generated by protoc, but functionally equivalent. It would be a big change to move away from protobuf.js.

There are a few other options for gRPC/protobuf and TypeScript generation listed at badsyntax/grpc-js-typescript.

The problem addressed by this PR is not the lack of TypeScript definitions for the protobufs, it's that the objects returned by BlockDecoder methods are not the generated protobufs, and it is the definitions for the returned objects that are missing. BlockDecoder takes actual Fabric protobuf objects as input and deserializes all the serialized protobufs nested within those protobuf structures to make working with them easier. The structure of those output objects is similar to the generated protobuf interface definitions already in fabric-protos, but not the same as many of the fields defined in the protobufs as bytes are replaced by actual objects. Ideally we would have correct TypeScript definitions for what is really returned by the BlockDecoder but, since they were not written, we currently have any type used instead.

bestbeforetoday · 2021-10-19T08:47:35Z

This has been inactive for a long time and is not yet in a state to merge so I'll close it. Feel free to reopen if progress in made on the implementation.

jscode017 requested a review from a team as a code owner July 27, 2021 05:30

petermetz suggested changes Jul 27, 2021

View reviewed changes

FABN-1722: export BlockDecoder for typescript definition

c4a57e4

Signed-off-by: jsjs026 <[email protected]>

jscode017 force-pushed the FABN-1722 branch from 053d17b to c4a57e4 Compare July 27, 2021 17:05

bestbeforetoday requested changes Aug 12, 2021

View reviewed changes

bestbeforetoday reviewed Aug 27, 2021

View reviewed changes

bestbeforetoday closed this Oct 19, 2021

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

FABN-1722: export BlockDecoder for typescript definition #477

FABN-1722: export BlockDecoder for typescript definition #477

jscode017 commented Jul 27, 2021

petermetz Jul 27, 2021

jscode017 Jul 27, 2021

petermetz Jul 27, 2021

petermetz commented Jul 27, 2021 •

edited

Loading

jscode017 commented Jul 28, 2021

petermetz commented Jul 29, 2021

bestbeforetoday left a comment •

edited

Loading

bestbeforetoday Aug 12, 2021

bestbeforetoday Aug 12, 2021

bestbeforetoday Aug 12, 2021

petermetz commented Aug 16, 2021

bestbeforetoday Aug 27, 2021

bestbeforetoday commented Sep 15, 2021

bestbeforetoday commented Oct 19, 2021

FABN-1722: export BlockDecoder for typescript definition #477

FABN-1722: export BlockDecoder for typescript definition #477

Conversation

jscode017 commented Jul 27, 2021

petermetz Jul 27, 2021

Choose a reason for hiding this comment

jscode017 Jul 27, 2021

Choose a reason for hiding this comment

petermetz Jul 27, 2021

Choose a reason for hiding this comment

petermetz commented Jul 27, 2021 • edited Loading

jscode017 commented Jul 28, 2021

petermetz commented Jul 29, 2021

bestbeforetoday left a comment • edited Loading

Choose a reason for hiding this comment

bestbeforetoday Aug 12, 2021

Choose a reason for hiding this comment

bestbeforetoday Aug 12, 2021

Choose a reason for hiding this comment

bestbeforetoday Aug 12, 2021

Choose a reason for hiding this comment

petermetz commented Aug 16, 2021

bestbeforetoday Aug 27, 2021

Choose a reason for hiding this comment

bestbeforetoday commented Sep 15, 2021

bestbeforetoday commented Oct 19, 2021

petermetz commented Jul 27, 2021 •

edited

Loading

bestbeforetoday left a comment •

edited

Loading