flow-cli icon indicating copy to clipboard operation
flow-cli copied to clipboard

Allow custom message limits in flowkit client

Open peterargue opened this issue 2 years ago • 6 comments

Issue To Be Solved

Currently, the flowkit client uses a hardcoded max message size: https://github.com/onflow/flow-cli/blob/6aa41a27add2d95066fc96072cc33c498f515deb/flowkit/gateway/grpc.go#L39

However, Access nodes can configure custom node specific limits to handle cases where data sizes exceed the default. This has come up several times in the last few months as data sizes have increased. Public ANs currently use a limit of 60MB.

(Optional): Suggest A Solution

We should make this limit configurable. A few potential backwards compatible solutions:

  1. Add a new constructor NewCustomGrpcGateway which supports passing in a client and the other fields. This could allow callers to build their own SDK client with the grpc settings they want, as well as pass their own context.
  2. Add optional Option callbacks to allow customizing the grpc client options. This would be less flexible and require some more complex logic in the library to properly manage and pass the options into the SDK instantiation.
  3. Add an optional Option callback to allow specifying just the max message size. This would be simple to implement and add minimal changes to the code, but provides the least flexibility. However, it would solve the immediate problem in a simple way.

(Optional): Context

This limit is too low for GetTransactionResultByBlockID for some recent blocks, resulting the ResourceExhausted errors.

peterargue avatar Jul 17 '23 18:07 peterargue

What about making the variables of the struct public, then we can create a gateway any way we like?

bjartek avatar Jul 17 '23 18:07 bjartek

Also not sure i really approve of sending in a context that is stored for the duration of the gateway. Context should be sent in with every request IIRC so that it can be used to cancel a request and all tasks depending on it.

For flow-cli pure usage this is not so important but for flowkit where this is something that might live on for a while it is.

bjartek avatar Jul 17 '23 18:07 bjartek

What about making the variables of the struct public, then we can create a gateway any way we like?

My concern with this is that they should never be updated after instantiation, and creates room for inconsistent state like a secure client with secureClient set to false. Having a constructor that accepted the required fields at least makes it clear exactly which settings are applied.

Also not sure i really approve of sending in a context that is stored for the duration of the gateway. Context should be sent in with every request IIRC so that it can be used to cancel a request and all tasks depending on it.

For flow-cli pure usage this is not so important but for flowkit where this is something that might live on for a while it is.

Yea that's fair. Storing contexts on structs isn't a best practice. However, the flowkit API already hides the context so it would at least allow a caller to have some control over the client/request lifecycle. I'm not attached either way.

peterargue avatar Jul 18 '23 18:07 peterargue

This might be naive, but can we just increase the hardcoded max size to a larger value to maximize support?

nvdtf avatar Jul 18 '23 21:07 nvdtf

I did so here https://github.com/onflow/flow-cli/pull/1118 @nvdtf

bjartek avatar Jul 18 '23 21:07 bjartek

This might be naive, but can we just increase the hardcoded max size to a larger value to maximize support?

yea, that would work for now. we've had to update the value multiple times already, so just thinking about options that avoid having to update multiple repos. I'm totally fine if you'd prefer to keep it simple.

peterargue avatar Jul 19 '23 16:07 peterargue