Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Draft: Resolves #80, Bumped fabric-proto-go to fabric-protos-go-apiv2 #84

Closed

Conversation

tobigiwa
Copy link

@tobigiwa tobigiwa commented Sep 17, 2023

Draft: This is a working change on bumping the fabric-chaincode-go to use fabric-protos-go-apiv2 and the new protobufs lib. This commits includes changes to fuction signatures, return values, interface definition and type definition to type "ChaincodeMessage" and "pb.Response".

It is a breaking change.

Here is a repository with a branch that house the state of the codebase after resolving the imports with fabric-protos-go-apiV2 lib, this does not breaks anything yet and it is intended to showcase to the maintainers the complaint of the go vet . The go vet ./... command should be run at the root of the project.

https://github.com/tobigiwa/fabric-chaincode-go/tree/fix

Resolves #80

…fabric-protos-go to fabric-protos-go-apiv2 and "github.com/golang/protobuf"(deprecated) to "google.golang.org/protobuf

Bumped google.golang.org/protobuf to v1.31.0 from v1.5.2

It can be noticed that the package "github.com/golang/protobuf" is still an indirect dependency used by google.golang.org/grpc/credentials v1.53.0, even the latest update v1.58.0 still does.

Made sure all tests passed.

CHANGES INCLUDE:

	SOURCE CODE
The "AssertProtoEqual" was borrowed from: // https://github.com/hyperledger/fabric-gateway/blob/cd1bc1f3fcf007bd97244120d9a8d112153322cd/pkg/internal/test/transaction.go#L20-L22
which is more apt to test the Protobufs of google.golang.org/protobuf/ implementation. It replaced assertEqual.

now := ptypes.TimestampNow() --> now := timestamp.Now()

IMPORTS
All Import aliases are kept.
"github.com/golang/protobuf/proto" --> "google.golang.org/protobuf/proto"

pb "github.com/hyperledger/fabric-protos-go/peer" --> pb "github.com/hyperledger/fabric-protos-go-apiv2/peer"

"github.com/golang/protobuf/ptypes/timestamp" --> timestamp "google.golang.org/protobuf/types/known/timestamppb"

"github.com/hyperledger/fabric-protos-go/ledger/queryresult" --> "github.com/hyperledger/fabric-protos-go-apiv2/ledger/queryresult"

"github.com/hyperledger/fabric-protos-go/ledger/msp" →
"github.com/hyperledger/fabric-protos-go-apiv2/ledger/msp"
"github.com/hyperledger/fabric-protos-go/common" --> "github.com/hyperledger/fabric-protos-go-apiv2/common"

Signed-off-by: tobigiwa <giwaoluwatobi@gmail.com>
@tobigiwa tobigiwa requested a review from a team as a code owner September 17, 2023 08:15
@tobigiwa
Copy link
Author

I'll be awaiting reply, discord username is @TobyTobias.

@ryjones
Copy link
Member

ryjones commented Sep 17, 2023

@denyeart take a look

@denyeart
Copy link
Contributor

Thank you for the contribution!

Could you check the "go vet" errors in the failed check?

Since the shift to fabric-protos-go-apiv2 is a significant change, we'll want to prove that everything works fine end-to-end. This would imply updating your fork of fabric-contract-api-go to use your updated fabric-chaincode-go, and then trying a fabric-sample with your updated fabric-contract-api-go and your updated fabric-chaincode-go. Here's the sample that I would recommend trying:
https://github.com/hyperledger/fabric-samples/blob/main/asset-transfer-basic/chaincode-go/go.mod

For doc tutorial about this chaincode, see:
https://hyperledger-fabric.readthedocs.io/en/latest/chaincode4ade.html
https://hyperledger-fabric.readthedocs.io/en/latest/deploy_chaincode.html

Once you demonstrate that it all works end-to-end, we can merge this PR, then merge the fabric-contract-api-go PR that uses the updated fabric-chaincode-go, and then merge the fabric-sample that uses them both.

… use fabric-protos-go-apiv2 and the new protobufs lib. This commits includes changes to fuction signatures, return values, interface definition and type definition to type "ChaincodeMessage" and "pb.Response"

. It is a breaking change.
Signed-off-by: tobigiwa <giwaoluwatobi@gmail.com>
@tobigiwa tobigiwa changed the title Resolves #80, Bumped fabric-proto-go to fabric-protos-go-apiv2 Draft: Resolves #80, Bumped fabric-proto-go to fabric-protos-go-apiv2 Sep 19, 2023
@jt-nti
Copy link
Member

jt-nti commented Jan 15, 2024

It would be great to get everything updated to use fabric-protos-go-apiv2 however since it is such a significant (breaking?) change, there are probably implications for module versioning.

fabric-chaincode-go is basically unversioned, with pseudo-version numbers, so technically it makes no guarantees about backward compatibility but making this change is likely to be disruptive.

fabric-contract-api-go is currently at v1.2.2, which will make things even more interesting! A breaking change should take it to v2.

@denyeart
Copy link
Contributor

denyeart commented Jan 15, 2024

I commented in the main fabric APIv2 issue.

I would be in favor of updating all fabric repositories to APIv2 before Fabric v3 gets released (including Go chaincode repositories), if somebody tests and successfully proves that having a mixed set of Fabric v2.x orderers and peers (old protobuf) and new v3.x orderers and peers (new APIv2) works seamlessly. Any volunteers?

@davidkhala
Copy link

@tobigiwa Are you still working on it?

@tobigiwa
Copy link
Author

Not again... buh am hoping to take another look at it again.

@tobigiwa
Copy link
Author

The sync.Mutex that was dragged into the apiV2 is the problem... function that takes it type and return it... are being flagged by go vet as copy of locked values

Signed-off-by: tobigiwa <giwaoluwatobi@gmail.com>
@bestbeforetoday
Copy link
Member

The change to fabric-protos-go-apiv2 was delivered in PR #108.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Update from fabric-protos-go to fabric-protos-go-apiv2
6 participants