use grpcAccess Client option in Gateway #45
Conversation
ianthpun
requested review from
Kay-Zee,
janezpodhostnik,
sideninja,
chasefleming,
nvdtf,
bluesign,
bjartek and
jribbink
as code owners
|
|
||
| grpc.WithTransportCredentials(insecure.NewCredentials()), |
There was a problem hiding this comment.
this is already defaulted
| func NewGrpcGateway(network config.Network, opts ...grpc.DialOption) (*GrpcGateway, error) { | ||
| options := []grpc.DialOption{ | ||
| func NewGrpcGateway(network config.Network, opts ...grpcAccess.ClientOption) (*GrpcGateway, error) { | ||
| opts = append( |
There was a problem hiding this comment.
This change isn't equivalent to the previous version: Before, the default was insecure, and the options passed to the function were appended, allowing to override this default. Now, the insecure option is appended to the given options, overriding options passed by the user.
Ideally also add a test that ensures this
There was a problem hiding this comment.
are you sure? Isn't it done where the defaults are set first, then any additonal options are appended to override the default?
https://github.com/onflow/flow-go-sdk/blob/master/access/grpc/client.go#L56-L58
Here the default is set as grpc.WithTransportCredentials(insecure.NewCredentials()), then any additional options are appended here:
https://github.com/onflow/flow-go-sdk/blob/master/access/grpc/client.go#L66-L70
meaning, shouldn't the final result be
dialOptions = []grpc.DialOptions{
grpc.WithTransportCredentials(insecure.NewCredentials()), // from the Default
grpc.WithDefaultCallOptions(grpc.MaxCallRecvMsgSize(maxGRPCMessageSize) // added from flowkit
}
There was a problem hiding this comment.
Oh sorry, i just understood what you meant 👍 let me fix this
This gives access to flowkit callers whatever option they want