search

hyperledger / fabric-admin-sdk

Fabric SDK for Admin Capability services
Apache License 2.0
31 stars 19 forks source link

Dev: try to replace proto #57

Closed SamYuan1990 closed 1 year ago

SamYuan1990 commented 1 year ago

by this pr, I try to fix #45 and fix #46, there still some code as copy paste or logging issue left. to do:

Signed-off-by: Sam Yuan yy19902439@126.com

SamYuan1990 commented 1 year ago

hi @bestbeforetoday , I try to use latest proto v2 api and meanwhile I found I have to decouple fabric dependencies... but seems there some error during implementation please advise.

bestbeforetoday commented 1 year ago

This looks like both fabric-protos and fabric-protos-apiv2 are both being included in the dependency tree. Only one implementation of the Go protobuf bindings for a given type can exist in the Go implementation:

https://developers.google.com/protocol-buffers/docs/reference/go/faq#namespace-conflict

Somehow we need to make sure that this module and any module dependencies are not using the older fabric-protos package. The best approach is probably just to avoid any dependency on other Fabric packages that use the older protobuf bindings, and implement the required logic in this module. In time all of the Fabric packages will migrate to the newer protobuf bindings but this will take time. As that happens we can look for common functionality and extract that out to shared packages but for now just implementing things locally is probably going to be easier.

SamYuan1990 commented 1 year ago

This looks like both fabric-protos and fabric-protos-apiv2 are both being included in the dependency tree. Only one implementation of the Go protobuf bindings for a given type can exist in the Go implementation:

https://developers.google.com/protocol-buffers/docs/reference/go/faq#namespace-conflict

Somehow we need to make sure that this module and any module dependencies are not using the older fabric-protos package. The best approach is probably just to avoid any dependency on other Fabric packages that use the older protobuf bindings, and implement the required logic in this module. In time all of the Fabric packages will migrate to the newer protobuf bindings but this will take time. As that happens we can look for common functionality and extract that out to shared packages but for now just implementing things locally is probably going to be easier.

do you think can we upgrade the function(from old proto to new) one by one or we have to upgrade all the proto from old to new in once?

bestbeforetoday commented 1 year ago

Sorry, I don't know. When moving from fabric-protos-go to fabric-protos-go-apiv2 previously, I switched the whole module from one to the other in one step. Give it a try and see what happens. There are quite a few dependencies between different protobuf messages though, so it might be difficult to use only a certain subset of messages from each of fabric-protos-go and fabric-protos-go-apiv2.

SamYuan1990 commented 1 year ago

@bestbeforetoday , please help review this PR.

SamYuan1990 commented 1 year ago

@bestbeforetoday, I found error as "failed to send transaction EOF", related with network connection issues and I make a quick fix. https://github.com/Hyperledger-TWGC/fabric-admin-sdk/actions/runs/3592767059/jobs/6048879630#step:7:150

SamYuan1990 commented 1 year ago

@bestbeforetoday , ready for review.