FABGW-21: Realtime implementation of ChaincodeEvents service #2604

bestbeforetoday · 2021-05-25T17:42:43Z

Initial implementation of Fabric Gateway's ChaincodeEvents service, allowing only listening for realtime chaincode events.

Also add block number into CommitStatus response.

internal/pkg/gateway/commit/commitadapter.go

bestbeforetoday · 2021-05-26T12:41:45Z

/ci-run

github-actions · 2021-05-26T12:43:43Z

AZP build triggered!

bestbeforetoday · 2021-05-26T15:06:34Z

Refactored to break cyclic dependency and enable the use of Counterfeiter as the mocking framework again.

internal/pkg/gateway/commit/finder.go

internal/pkg/gateway/commit/commitadapter.go

integration/gateway/gateway_test.go

bestbeforetoday · 2021-05-27T14:43:02Z

@manish-sethi Moving back to draft to continue changes to support chaincode event listening, at least until there is a suitable mechanism to grab block number along with transaction status and a satisfactory interim PR can be delivered.

manish-sethi · 2021-06-01T16:16:13Z

@manish-sethi Moving back to draft to continue changes to support chaincode event listening, at least until there is a suitable mechanism to grab block number along with transaction status and a satisfactory interim PR can be delivered.

@bestbeforetoday - The PR for block number is merged now.

manish-sethi

@bestbeforetoday - I feel that the code in this area is already becoming complex enough. Primarily because of how dependencies are factored out and a number of notifiers. I have listed a few suggestions to simplify things, see if they makes sense to you.

manish-sethi · 2021-06-02T15:41:14Z

internal/pkg/gateway/api.go

+	request := &gp.ChaincodeEventsRequest{}
+	if err := proto.Unmarshal(signedRequest.Request, request); err != nil {
+		return status.Error(codes.InvalidArgument, "invalid chaincode events request")
+	}


Even if there is no error, this could still leave request with one or more empty field (e.g., signedRequest.Request could be an empty bytes). Better to explicitly check for this so it does not lead to nil-pointer panic in the downstream code. The downstream code may already be checking for this but it becomes hard to reason about at higher level so better to have independent check at the entry level.

I guess that this applies to existing function in this file as well.

How will an empty string or empty slice cause a nil-pointer panic? Could you give me an example of the change you are requiring here?

In downstream code this byte array is used to unmarshal and I am not certain that every place all the checks have been in place. So, it was just to fail early if this byte slice is nil - kind of basic input check.

You can unmarshall a zero-length []byte fine. I don't understand the benefit of adding code bloat for a special check for a zero-length slice rather than letting a meaningful error be raised, such as the requested channel doesn't exist or the supplied identity is not valid. The client APIs don't send empty protobufs so it's not a scenario that can occur by accident

internal/pkg/gateway/commit/blocknotifier.go

internal/pkg/gateway/commit/chaincodeeventnotifier.go

manish-sethi · 2021-06-02T21:26:51Z

internal/peer/node/start.go

 			peerAdapter := &commit.PeerAdapter{
 				Peer: peerInstance,
 			}
+			notifier := commit.NewNotifier(peerAdapter)
 			gatewayprotos.RegisterGatewayServer(
 				peerServer.Server(),
 				gateway.CreateServer(
 					&gateway.EndorserServerAdapter{Server: serverEndorser},
 					discoveryService,
 					&commit.Finder{
 						Query:    peerAdapter,
-						Notifier: commit.NewNotifier(peerAdapter),
+						Notifier: notifier,
+					},
+					&commit.Eventer{
+						Notifier: notifier,
 					},


Though not introduced in this PR but it got further flared - This pattern of linking the dependencies surfaces much of internal details of gateway module to this level. The only real dependency of gateway module is a few functions on the peerInstance. I feel that this will be clearer if you could define a limited interface for this and just pass peerInstance in gateway.CreateServer and construct PeerAdapter, notifier, Finder, and Eventer internally inside gateway module. This may also remove many interfaces in the gateway module.

I started with the narrower interface but was required to do it this way by another maintainer. The way the code is currently structured (at the request of another maintainer), we can't pass concrete implementations in to gateway.CreateServer since mocks are passed in there by unit tests.

I'll look at some changes back towards my original approach for the Finder and Eventer, and I propose we look at changes to other areas of the code in a different PR rather than shoe-horn a bunch of unrelated code refactor into this one.

I've made some changes to reduce the leakage of internal abstractions outside of the gateway package, which I think is along the lines of what you are asking.

Note that even within the gateway packages there are deliberately different layers of abstraction. The core Gateway code is intended to be agnostic to whether it is an embedded or standalone implementation, and so interacts with the embedded implementation-specific elements using interfaces, which could be replaced standalone implementations.

We can definitely do it in a separate PR. But I feel that there are way to many layers of abstraction for a simple enough functionality.

we can't pass concrete implementations in to gateway.CreateServer

contrary to this you were actually passing the peer instance to commit package. What my suggestion was that only external dependency of gateway package is to get an object on which it can call "RegisterForNotification" for a given channel and that should be easy enough to reflect in first-order via an interface.

What my suggestion was that only external dependency of gateway package is to get an object on which it can call "RegisterForNotification" for a given channel and that should be easy enough to reflect in first-order via an interface.

No.

RegisterForNotification() implies the peer embedded implementation, which is in the commit package. The commit package uses the NotificationSupplier interface to define CommitNotifications() which is the same as RegisterForNotification(), so it's already doing as you suggest.

The gateway server is not intended to be tied to either a peer embedded or standalone implementation, so it uses a CommitFinder interface to obtain transaction validation codes by some means that it doesn't need to know about, and which would be significantly different in the embedded and standalone implementations.

internal/pkg/gateway/commit/eventer.go

manish-sethi · 2021-06-02T22:43:25Z

internal/pkg/gateway/commit/finder.go

 type QueryProvider interface {
-	TransactionStatus(channelName string, transactionID string) (peer.TxValidationCode, error)
+	TransactionStatus(channelName string, transactionID string) (peer.TxValidationCode, uint64, error)
 }


To highlight the comment that I made in start.go in context - This is how it's easy for someone to get lost in the maze of layers of finer-grained dependencies as opposed having a simplified straight-forward dependency. In this case, this interface, it's sole implementation, and it's sole consumer all three are in the same package. Which makes it unnecessarily round-about to understand the true linkages.

This describes an interface for a subset of capability provided by the PeerAdapter struct, which allows me to use a mock implementation in unit tests. Trying to make the PeerAdapter work purely with interfaces (that I could then mock) would be even more convoluted because of the way to Go type system works

Decoupling code within a package for the sake of using mocks for UTs does not sound a good idea to me. I am not sure I understand what prevents from writing UTs as long as external dependencies are mocked.

QueryProvider decouples the PeerAdapter (i.e. the thin layer of code not easily untangled from the peer implementation, and so you could think of as being the external dependency) from the implementation in this package

manish-sethi · 2021-06-03T02:17:27Z

internal/pkg/gateway/commit/blocknotifier.go

+type blockListener interface {
+	receiveBlock(*ledger.CommitNotification)
+	close()
+}


Can we remove this interface and straight away use the two concrete types - to make it more explicit. The current code may have saved a few lines of code but lessen the readability.

This abstraction isn't there to save a few lines of code. It's to:

Keep the blockListener focused on the task of receiving and dispatching events from the ledger

Allow it to be extended with other listeners without requiring code changes

Avoid it needing dependencies on listener implementations

These objectives sounds good if they are between course level modules. Applying these to micro level of granularity has led to Fabric code at many places as hard to read IMO.

Feel free to make that change

manish-sethi · 2021-06-03T02:20:36Z

internal/pkg/gateway/commit/blocknotifier.go

+type blockNotifier struct {
+	commitChannel <-chan *ledger.CommitNotification


I feel that too many notifiers are making it hard to read code. Can we club the functionality in this into the main Notifier. That will make the main Notifier act as a central piece that glues together the lower level events received from ledger and the other two notifiers handle the event specific delivery.

There is a one-to-many relationship between Notifier and blockNotifier. Also Notifier needs to be able to throw away and recreate a blockNotifier. I'm not sure how you see merging Notifier and blockNotifier working

I was thinking in the lines of launching a gorouting for each channel directly into the Notifier and the notifier just keeps track of the two event specific notifiers. In the current structure of the code it sounded like all three notifiers are kind of siblings when they are actually not. The other approach could be to have the both event specific notifiers only inside this blocknotifier - so this becomes full controller of channel specific notifier and hierarchy is clearer.

Feel free to make that change

Initial implementation of Fabric Gateway's ChaincodeEvents service, allowing only listening for realtime chaincode events. Also add block number into CommitStatus response. Signed-off-by: Mark S. Lewis <[email protected]>

bestbeforetoday · 2021-06-03T14:26:43Z

/ci-run

github-actions · 2021-06-03T14:27:16Z

AZP build triggered!

Previously node.serve() accessed a lot of implementation detail within the gateway package to create structs of the appropriate abstraction for the gateway internals. Refactored to move the translation between external structs and internal abstractions to the gateway.CreateServer() function, and moved the old implementation to an newServer() function that is used by unit tests to create a Gateway server that is agnostic to whether it is an embedded or standalone implementation. Signed-off-by: Mark S. Lewis <[email protected]>

bestbeforetoday · 2021-06-03T18:26:29Z

@manish-sethi Your outstanding comments seem to be primarily objections to code structure that was already there prior to this PR, and was there at the request of other maintainers. I have addressed as many of these structural issues as I think I reasonably can in this PR, which is delivering chaincode event listening, not a refactor of existing code

manish-sethi · 2021-06-04T14:31:29Z

integration/gateway/gateway_test.go

-			badSigningIdentity := network.OrdererUserSigner(network.Orderer("orderer"), "Admin")
-			badIdentity, err := badSigningIdentity.Serialize()
+	Describe("ChaincodeEvents", func() {
+		It("should respond with emitted chaincode events", func() {


I guess that you still plan to add security related tests, like the one in CommitStatus api ? However, that can always be added later.

manish-sethi · 2021-06-04T14:32:25Z

@manish-sethi Your outstanding comments seem to be primarily objections to code structure that was already there prior to this PR, and was there at the request of other maintainers. I have addressed as many of these structural issues as I think I reasonably can in this PR, which is delivering chaincode event listening, not a refactor of existing code

Yes, As I mentioned in the previous comment, the refactoring can be taken up in a separate PR

bestbeforetoday · 2021-06-08T11:25:26Z

@manish You were not keen on having the blockNotifier in addition to the other notifiers. This is really there because you imposed the limitation that there could only be one listener to ledger notifications, so the blockNotifier provides a mechanism to workaround that limitation and allow multiple listeners. One approach to simplifying this code might be to remove your restriction on only allowing a single listener for ledger notifications, in which case the blockNotifier and the logic there could be completely removed.

bestbeforetoday requested a review from a team as a code owner May 25, 2021 17:42

bestbeforetoday force-pushed the FABGW-21 branch from e34c6df to 4d80b4a Compare May 26, 2021 08:51

C0rWin reviewed May 26, 2021

View reviewed changes

internal/pkg/gateway/commit/commitadapter.go Outdated Show resolved Hide resolved

bestbeforetoday force-pushed the FABGW-21 branch 2 times, most recently from 43d18f1 to 7f951ea Compare May 26, 2021 12:13

bestbeforetoday force-pushed the FABGW-21 branch 3 times, most recently from 1b323b6 to 16b05e6 Compare May 26, 2021 15:01

manish-sethi reviewed May 26, 2021

View reviewed changes

internal/pkg/gateway/commit/finder.go Outdated Show resolved Hide resolved

internal/pkg/gateway/commit/commitadapter.go Outdated Show resolved Hide resolved

integration/gateway/gateway_test.go Outdated Show resolved Hide resolved

bestbeforetoday marked this pull request as draft May 27, 2021 14:43

bestbeforetoday force-pushed the FABGW-21 branch 5 times, most recently from 358f48d to 8d64284 Compare May 28, 2021 14:24

bestbeforetoday changed the title ~~FABGW-21: Return block number in CommitStatusResponse~~ FABGW-21: Realtime implementation of ChaincodeEvents service May 28, 2021

bestbeforetoday force-pushed the FABGW-21 branch from 8d64284 to c552b51 Compare May 28, 2021 14:35

bestbeforetoday force-pushed the FABGW-21 branch 4 times, most recently from cd6dba4 to e4ee943 Compare June 2, 2021 13:17

bestbeforetoday marked this pull request as ready for review June 2, 2021 13:57

bestbeforetoday requested a review from manish-sethi June 2, 2021 18:04

manish-sethi reviewed Jun 3, 2021

View reviewed changes

FABGW-21: Realtime implementation of ChaincodeEvents service

4d83485

Initial implementation of Fabric Gateway's ChaincodeEvents service, allowing only listening for realtime chaincode events. Also add block number into CommitStatus response. Signed-off-by: Mark S. Lewis <[email protected]>

bestbeforetoday force-pushed the FABGW-21 branch from e4ee943 to 4d83485 Compare June 3, 2021 12:58

bestbeforetoday requested a review from manish-sethi June 3, 2021 17:16

manish-sethi reviewed Jun 4, 2021

View reviewed changes

manish-sethi approved these changes Jun 4, 2021

View reviewed changes

manish-sethi merged commit 1f89be9 into hyperledger:main Jun 4, 2021

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

FABGW-21: Realtime implementation of ChaincodeEvents service #2604

FABGW-21: Realtime implementation of ChaincodeEvents service #2604

bestbeforetoday commented May 25, 2021 •

edited

Loading

bestbeforetoday commented May 26, 2021

github-actions bot commented May 26, 2021

bestbeforetoday commented May 26, 2021

bestbeforetoday commented May 27, 2021

manish-sethi commented Jun 1, 2021

manish-sethi left a comment

manish-sethi Jun 2, 2021

bestbeforetoday Jun 3, 2021

manish-sethi Jun 3, 2021

bestbeforetoday Jun 3, 2021

manish-sethi Jun 2, 2021

bestbeforetoday Jun 3, 2021

bestbeforetoday Jun 3, 2021

manish-sethi Jun 3, 2021

manish-sethi Jun 3, 2021

bestbeforetoday Jun 3, 2021

manish-sethi Jun 2, 2021

bestbeforetoday Jun 3, 2021

manish-sethi Jun 3, 2021

bestbeforetoday Jun 3, 2021

manish-sethi Jun 3, 2021

bestbeforetoday Jun 3, 2021

manish-sethi Jun 3, 2021

bestbeforetoday Jun 3, 2021

manish-sethi Jun 3, 2021

bestbeforetoday Jun 3, 2021

manish-sethi Jun 3, 2021

bestbeforetoday Jun 3, 2021

bestbeforetoday commented Jun 3, 2021

github-actions bot commented Jun 3, 2021

bestbeforetoday commented Jun 3, 2021

manish-sethi Jun 4, 2021

manish-sethi commented Jun 4, 2021

bestbeforetoday commented Jun 8, 2021

		type blockNotifier struct {
		commitChannel <-chan *ledger.CommitNotification

FABGW-21: Realtime implementation of ChaincodeEvents service #2604

FABGW-21: Realtime implementation of ChaincodeEvents service #2604

Conversation

bestbeforetoday commented May 25, 2021 • edited Loading

bestbeforetoday commented May 26, 2021

github-actions bot commented May 26, 2021

bestbeforetoday commented May 26, 2021

bestbeforetoday commented May 27, 2021

manish-sethi commented Jun 1, 2021

manish-sethi left a 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

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

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

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

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

bestbeforetoday commented Jun 3, 2021

github-actions bot commented Jun 3, 2021

bestbeforetoday commented Jun 3, 2021

Choose a reason for hiding this comment

manish-sethi commented Jun 4, 2021

bestbeforetoday commented Jun 8, 2021

bestbeforetoday commented May 25, 2021 •

edited

Loading