-
Notifications
You must be signed in to change notification settings - Fork 56
Refactor CommonClient implementation and add SSL support for gRPC #37
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
base: develop
Are you sure you want to change the base?
Refactor CommonClient implementation and add SSL support for gRPC #37
Conversation
@Jeongseup would appreciate your thoughts on this change 🙏 |
Thank you It is wonderful. I thought also CVMS has to have a test mock client and db for new package implementation. But As you already know, This PR has HUGE code replacement. so.. I cannot check all about this. Please wait for it. I want to make a several new packages for Babylon and Axelar Amplifier. |
Sure no worries! After all this is quite a change. We run my forked CVMS and so far it works quite well. Only thing is it increased memory consumption a bit. Sometimes memory consumption spikes for a very short time before it's back to normal. I did not find the reason yet, but just something to be aware if you defined any memory usage quotas. Oh nice I also wanted to take a look at getting axelar amplifier support into CVMS, but had no time yet :( |
Ah I remembered it! When I developed this project a few months ago, There was memory leak in making a client (not requester). I think we should check memory leak before merging this PR. For that, I already implemented debug handle at https://github.com/cosmostation/cvms/blob/develop/internal/app/exporter/exporter.go#L56 |
Signed-off-by: Simon Lichtenauer <Simon.Lichtenauer@figment.io>
* started working on common client interface abstracting different concrete clients Signed-off-by: Simon Lichtenauer <Simon.Lichtenauer@figment.io> * migrated APIClient to use abstract client implementation instead of resty directly Signed-off-by: Simon Lichtenauer <Simon.Lichtenauer@figment.io> * refactored grpc client use to use common abstract client; Fixed common client to use global application logger Signed-off-by: Simon Lichtenauer <Simon.Lichtenauer@figment.io> * Added changes to GRPC client to support SSL and insecure connections Signed-off-by: Simon Lichtenauer <Simon.Lichtenauer@figment.io> * moved client implementation into own package to ensure its only used indirectly Signed-off-by: Simon Lichtenauer <Simon.Lichtenauer@figment.io> * moved grpchelper methods into the grpc client implementation. Removed no longer used grpc helper methods Signed-off-by: Simon Lichtenauer <Simon.Lichtenauer@figment.io> * add mock and demo it with simple happypath test for uptime module Signed-off-by: Simon Lichtenauer <Simon.Lichtenauer@figment.io> * add changes to be compatible with latest changes in cosmostation/develop branch Signed-off-by: Simon Lichtenauer <Simon.Lichtenauer@figment.io> --------- Signed-off-by: Simon Lichtenauer <Simon.Lichtenauer@figment.io>
Signed-off-by: Simon Lichtenauer <Simon.Lichtenauer@figment.io>
…LS check timesout (#11) Signed-off-by: Simon Lichtenauer <Simon.Lichtenauer@figment.io>
…ges from new client implementation Signed-off-by: Simon Lichtenauer <Simon.Lichtenauer@figment.io>
28a6494
to
370743a
Compare
Makes sense. The memory usage pattern looked like a memory leak to me. I'll see if I can find something. I just rebased to keep this draft compatible with your upstream branch |
Could you share test GRPC endpoint for TLS? I used my one but not working. I want to check your endpoint format. |
@Jeongseup It should be as simple as using: grpc: 'band-grpc.polkachu.com:22990' or grpc: 'axelar-grpc.publicnode.com:443' Also sorry for me not keeping this PR up to date. Was busy with other thing, but I plan to rebase as soon as I find some time |
Nob! I can understand your busy nowadays. Actullay I wanted to add golangci-lint for our project. for that, I got too many errors... caused by deprecated grpc client. and also your PR is very good implementation. and I'm making a branch for this. After creating new branch, Please check it. that branch is about up to date for this PR |
Actually I don't want to cosmos-sdk dependency if it is possible, that's why I used dynamic jsonpb, but this lib was deprecated now. So, I think we should remove cosmos-sdk.io depencies to make this PR. Expect that, I think that your PR for creating public client interface was REALLY useful and beautiful codes. Thanks! |
@Jeongseup yeah I understand the issue with depending on cosmos-sdk, However there's no real simple solution to actually deserialize the protobuf objects into usable go objects. The dependency I added is actually just the |
PR(Pull Request) Overview
Rework of the
CommonClient
implementation and adding support of SSL encrypted gRPC endpointsChanges
Description of Changes
This is a major change of the
CommonClient
type by instead of usingresty
directly the client implementations are hidden behind an abstractclient.Client
interface. This allows to implement various clients for different use-cases (e.g. grpc) and also simplifies testing massively by allowing to useclient.Mock
to test the API related methods. An example of this is the added unit test for thegetValidatorUptimeStatus
method fromuptime/api.go
(https://github.com/cosmostation/cvms/compare/develop...qwertzlbert:cvms:refacture-client-implementation?expand=1#diff-8cc62f3c8463e0096e5b2689cd1dbad8d56d2b79af139f1072eced745bd5ee24) .Additionally I reworked the gRPC implementation to now also support SSL connections and rely on the newer google gRPC packages instead of the deprecated packages used previously.
Using a gRPC connection is now much simpler as the client also implements the
client.Client
interface as you can see from the changes added to theeventnonce
package.Testing Method
client.Mock
Additional Information
Consider this PR as a work in progress for now, while it seems to work testing is ongoing. I just wanted to open the PR now already to get some feedback to this change