Implement query committed chaincodes in channel #88

samuelvenzi · 2023-02-09T13:01:15Z

Implement query committed function
Implement query committed unit test
Add query committed to e2e test

Signed-off-by: Samuel Venzi samuel.venzi@me.com

Signed-off-by: Samuel Venzi <samuel.venzi@me.com>

samuelvenzi · 2023-02-09T13:24:34Z

I let slip a small type error in the cc sequence verification. It's corrected and e2e test should be successful now.

SamYuan1990 · 2023-02-09T14:34:52Z

pkg/chaincode/querycommitted.go

+
+// QueryCommitted chaincode on a specific peer.
+func QueryCommitted(ctx context.Context, connection grpc.ClientConnInterface, signingID identity.SigningIdentity, channelID string) (*lifecycle.QueryChaincodeDefinitionsResult, error) {
+	queryArgs := &lifecycle.QueryChaincodeDefinitionsArgs{}


https://github.com/hyperledger/fabric/blob/main/internal/peer/lifecycle/chaincode/querycommitted.go#L223-L227
if I am correct, we can add chaincode name a parameter? ref to fabric code as example.

wait, @bestbeforetoday , we removed name as parameter at https://github.com/hyperledger/fabric-protos-go-apiv2/blob/main/peer/lifecycle/lifecycle.pb.go#L1290-L1328 ?
are we able to pass QueryChaincodeDefinitionsArgs with name in fabric-protos-go-apiv2?

There are currently two options for querying committed chaincodes in Fabric. One of them queries all committed in the channel and the other filters it by chaincode name.

One uses QueryChaincodeDefinitions tx name and the other uses QueryChaincodeDefinition.

Should I proceed to add both options then? In separate functions or the same?

I suppose if possible, we should support both query all and query by name.

are we able to pass QueryChaincodeDefinitionsArgs with name in fabric-protos-go-apiv2?

We can use QueryChaincodeDefinitionArgs with name. I can do this after this PR.

type QueryChaincodeDefinitionArgs struct { state protoimpl.MessageState sizeCache protoimpl.SizeCache unknownFields protoimpl.UnknownFields Name string `protobuf:"bytes,1,opt,name=name,proto3" json:"name,omitempty"` }

by default, I will merge this PR before Monday, will keep this PR open for other reviewers. :-)
it's 11 pm my local time, I am going to have a rest.

@samuelvenzi , you are able to have another PR for query by name after this PR been merged.

When querying installed chaincode, there are two options: query all installed chaincodes, or query a specific named chaincode. Both options return exactly the same information about the installed chaincode(s). The difference is whether you get information about all or only one of the installed chaincodes. I can't see much point in the versions where you specify the name since you can just pick the name you are interested in from the all installed chaincodes result.

When querying committed chaincode, there are the same two options: query all committed, or query a specific named chaincode. I have no idea why, but for this query they actually return slightly different information. Querying for a specific named chaincode returns all the information returned from querying all, and also includes the approvals for that chaincode.

So for query installed, I am personally happy with just querying for all installed chaincodes. Query for a specific installed chaincode just means more code to maintain for no value.

For query committed, I think we will need to prove API calls for both options. I would prefer to have two different API calls. Any preferences on the naming? It could be QueryAllCommitted and QueryCommitted, or QueryCommitted and QueryCommittedWithName, or something else...? That can happen in a later PR.

I personally prefer QueryCommitted and QueryCommittedWithName. I didn't realize the information was slightly different, but you're right.

SamYuan1990

LGTM, as comments with nit enhancement maybe blocked by proto definitions and I don't want that block this PR.

bestbeforetoday

Nice work. Thank you for the contribution! I've added my thoughts to the discussions in the comments but generally happy with this PR as-is. Just one change to the e2e test code that I think would be worth making - see in-line comments.

bestbeforetoday · 2023-02-09T17:50:38Z

pkg/chaincode/querycommitted.go

+
+// QueryCommitted chaincode on a specific peer.
+func QueryCommitted(ctx context.Context, connection grpc.ClientConnInterface, signingID identity.SigningIdentity, channelID string) (*lifecycle.QueryChaincodeDefinitionsResult, error) {
+	queryArgs := &lifecycle.QueryChaincodeDefinitionsArgs{}


When querying installed chaincode, there are two options: query all installed chaincodes, or query a specific named chaincode. Both options return exactly the same information about the installed chaincode(s). The difference is whether you get information about all or only one of the installed chaincodes. I can't see much point in the versions where you specify the name since you can just pick the name you are interested in from the all installed chaincodes result.

When querying committed chaincode, there are the same two options: query all committed, or query a specific named chaincode. I have no idea why, but for this query they actually return slightly different information. Querying for a specific named chaincode returns all the information returned from querying all, and also includes the approvals for that chaincode.

So for query installed, I am personally happy with just querying for all installed chaincodes. Query for a specific installed chaincode just means more code to maintain for no value.

For query committed, I think we will need to prove API calls for both options. I would prefer to have two different API calls. Any preferences on the naming? It could be QueryAllCommitted and QueryCommitted, or QueryCommitted and QueryCommittedWithName, or something else...? That can happen in a later PR.

bestbeforetoday · 2023-02-09T18:03:32Z

test/e2e_test.go

@@ -273,6 +273,14 @@ var _ = Describe("e2e", func() {
 			printGrpcError(err)
 			Expect(err).NotTo(HaveOccurred(), "commit chaincode")

+			result, err := chaincode.QueryCommitted(specCtx, peerConnections[0].connection, peerConnections[0].id, channelName)


The commit above should have caused the chaincode to be committed on all network peers where it was installed. We could confirm this by querying installed chaincode on both org peers.

However, we only know for sure that the commit has completed on the (Gateway) peer where we invoked the commit since that is where we observe the successfully committed transaction in the ledger. The other peers may not yet have processed the transaction. So I think this approach of querying just the peer used for the commit is reasonable.

One thing I would change though... using peerConnections[0] happens to be the same peer that was used for the commit but that could change if somebody changed the code above later. I think it would be better to be more explicit about the peer being used with n_conn1 as the connection and org1MSP as the identity.

Thanks for the pointer, @bestbeforetoday. Changing it now.

Signed-off-by: Samuel Venzi <samuel.venzi@me.com>

Implement query committed chaincodes in channel

aa07010

Signed-off-by: Samuel Venzi <samuel.venzi@me.com>

samuelvenzi marked this pull request as ready for review February 9, 2023 13:02

samuelvenzi mentioned this pull request Feb 9, 2023

golang features implementation #15

Closed

22 tasks

SamYuan1990 requested review from bestbeforetoday, SamYuan1990 and channingduan February 9, 2023 13:08

Fix e2e test sequence verification

e9b0c6e

Signed-off-by: Samuel Venzi <samuel.venzi@me.com>

SamYuan1990 reviewed Feb 9, 2023

View reviewed changes

SamYuan1990 previously approved these changes Feb 9, 2023

View reviewed changes

bestbeforetoday reviewed Feb 9, 2023

View reviewed changes

Make connection and id explicit in e2e test QueryCommitted call

63f798d

Signed-off-by: Samuel Venzi <samuel.venzi@me.com>

samuelvenzi dismissed SamYuan1990’s stale review via 63f798d February 9, 2023 19:37

samuelvenzi requested review from bestbeforetoday and SamYuan1990 and removed request for channingduan, bestbeforetoday and SamYuan1990 February 9, 2023 19:38

bestbeforetoday approved these changes Feb 9, 2023

View reviewed changes

bestbeforetoday merged commit a37ccb5 into hyperledger:main Feb 9, 2023

samuelvenzi deleted the feature/querycommitted branch February 10, 2023 13:13

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Implement query committed chaincodes in channel #88

Implement query committed chaincodes in channel #88

samuelvenzi commented Feb 9, 2023

samuelvenzi commented Feb 9, 2023

SamYuan1990 Feb 9, 2023

SamYuan1990 Feb 9, 2023

SamYuan1990 Feb 9, 2023

samuelvenzi Feb 9, 2023

SamYuan1990 Feb 9, 2023

samuelvenzi Feb 9, 2023 •

edited

Loading

SamYuan1990 Feb 9, 2023

SamYuan1990 Feb 9, 2023

bestbeforetoday Feb 9, 2023 •

edited

Loading

samuelvenzi Feb 9, 2023

SamYuan1990 left a comment

bestbeforetoday left a comment

bestbeforetoday Feb 9, 2023 •

edited

Loading

bestbeforetoday Feb 9, 2023

samuelvenzi Feb 9, 2023

Implement query committed chaincodes in channel #88

Implement query committed chaincodes in channel #88

Conversation

samuelvenzi commented Feb 9, 2023

samuelvenzi commented Feb 9, 2023

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

samuelvenzi Feb 9, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

bestbeforetoday Feb 9, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

SamYuan1990 left a comment

Choose a reason for hiding this comment

bestbeforetoday left a comment

Choose a reason for hiding this comment

bestbeforetoday Feb 9, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

samuelvenzi Feb 9, 2023 •

edited

Loading

bestbeforetoday Feb 9, 2023 •

edited

Loading

bestbeforetoday Feb 9, 2023 •

edited

Loading