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

Categorise chaincode operations by peer and gateway #194

Open
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

bestbeforetoday
Copy link
Member

Previous implementation was standalone functions, with all requiring both a gRPC connection and a client signing identity. This required:

  • An underisably large number of prameters.
  • Unwritten conventions on parameter ordering.
  • Documentation comments to highlight when the connection should be to a specific peer or to an organization gateway.
  • Duplication of arguments across multiple functions for calling code driving the chaincode lifecycle.

This implementation defines a Peer and Gateway struct that hold a gRPC connection and a client identity. Each of these types defines methods appropriate to their logical role to:

  • Avoid the need to provide gRPC connection and client identity arguments on every call.
  • Provide calling code with logical context for what they are interacting with.
  • Allow IDE code completion to suggest appropriate methods for the connection type.

For example, this pseudo-code flow:

chaincode.Install(ctx, peerConnection, id, chaincodePackage)
chaincode.Approve(ctx, gatewayConnection, id, chaincodeDefinition)
chaincode.Commit(ctx, gatewayConnection, id, chaincodeDefinition)

becomes:

peer.Install(ctx, chaincodePackage)
gateway.Approve(ctx, chaincodeDefinition)
gateway.Commit(ctx, chaincodeDefinition)

@bestbeforetoday
Copy link
Member Author

bestbeforetoday commented Apr 19, 2024

@SamYuan1990 @samuelvenzi What do you think of this as an approach? It's been bugging me for a long time that so many parameters are duplicated across chaincode functions. This is an alternative approach. Both work so I'm interested in your perspective on whether it's worth making a change from what we have. Any change needs to happen before we hit a proper v1 release.

I actually included the service discovery PeerMembershipQuery on Gateway. We could go further down that route and associate all functionality (not just chaincode lifecycle) with appropriate node connections (Gateway, Peer, Orderer) and hide all the implementation code/packages, or we could keep each category of functionality separate, like we do currently with a different package for each concern.

@SamYuan1990
Copy link
Contributor

I am good to see we simply the logic, but I have a question as chaincode.Install mapping to the same commend as peer chaincode install do we have any plan/idea to simply the peer CLI?

@bestbeforetoday
Copy link
Member Author

bestbeforetoday commented Apr 20, 2024

There are definitely some areas where a replacement admin CLI can be simplified compared to the existing peer CLI. The key one is that, for all existing peer CLI commands that submit a transaction to the ledger (which means all the ones on Gateway in this PR), the user needs to specify sufficient endorsing peers and the orderer(s) to which the endorsed transaction will be submitted. The admin SDK implementation makes use of the peer’s Gateway service to handle these interactions and so only ever requires a connection to a single (Gateway) peer for these interactions. This remains true for a BFT ordering service since the Gateway service handles the different ordering requirements for the consensus implementation.

Chaincode install talks directly to a specific peer so doesn’t use the Gateway service. It might not be possible to simplify that one. You could argue for a chaincode install command that allows multiple target peers to be specified, but I think that is going to make it harder to handle failure scenarios compared to just having the user script (or code) their own flow to drive install on each of the desired peers.

@SamYuan1990
Copy link
Contributor

Chaincode install talks directly to a specific peer so doesn’t use the Gateway service. It might not be possible to simplify that one. You could argue for a chaincode install command that allows multiple target peers to be specified, but I think that is going to make it harder to handle failure scenarios compared to just having the user script (or code) their own flow to drive install on each of the desired peers.

let's make it step by step, also we need to considering with existing user habits, this PR LGTM.

SamYuan1990
SamYuan1990 previously approved these changes Apr 20, 2024
Copy link
Contributor

@SamYuan1990 SamYuan1990 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

@samuelvenzi
Copy link
Contributor

@bestbeforetoday Sorry it took me this long to respond. I def like this approach. I see that this is now a draft, but I'll keep an eye to check it again once it's ready.

Previous implementation was standalone functions, with all requiring both a gRPC
connection and a client signing identity. This required:

- An underisably large number of prameters.
- Unwritten conventions on parameter ordering.
- Documentation comments to highlight when the connection should be to a
  specific peer or to an organization gateway.
- Duplication of arguments across multiple functions for calling code driving
  the chaincode lifecycle.

This implementation defines a Peer and Gateway struct that hold a gRPC
connection and a client identity. Each of these types defines methods
appropriate to their logical role to:

- Avoid the need to provide gRPC connection and client identity arguments on
  every call.
- Provide calling code with logical context for what they are interacting with.
- Allow IDE code completion to suggest appropriate methods for the connection type.

For example, this pseudo-code flow:

    chaincode.Install(ctx, peerConnection, id, chaincodePackage)
    chaincode.Approve(ctx, gatewayConnection, id, chaincodeDefinition)
    chaincode.Commit(ctx, gatewayConnection, id, chaincodeDefinition)

becomes:

    peer.Install(ctx, chaincodePackage)
    gateway.Approve(ctx, chaincodeDefinition)
    gateway.Commit(ctx, chaincodeDefinition)

Signed-off-by: Mark S. Lewis <Mark.S.Lewis@outlook.com>
@bestbeforetoday bestbeforetoday marked this pull request as ready for review June 2, 2024 14:14
@bestbeforetoday bestbeforetoday enabled auto-merge (squash) June 2, 2024 14:16
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.

None yet

3 participants