Skip to content

Conversation

qwertzlbert
Copy link
Contributor

PR(Pull Request) Overview

Rework of the CommonClient implementation and adding support of SSL encrypted gRPC endpoints

Changes

  • Major feature addition or modification
  • Bug fix
  • Code improvement
  • Documentation update

Description of Changes

This is a major change of the CommonClient type by instead of using resty directly the client implementations are hidden behind an abstract client.Client interface. This allows to implement various clients for different use-cases (e.g. grpc) and also simplifies testing massively by allowing to use client.Mock to test the API related methods. An example of this is the added unit test for the getValidatorUptimeStatus method from uptime/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 the eventnonce package.

Testing Method

  • Added simple unit test to demonstrate the use of client.Mock
  • Manual testing with various networks (ongoing)

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

@qwertzlbert
Copy link
Contributor Author

@Jeongseup would appreciate your thoughts on this change 🙏
I know it's quite a huge change, but it should help making CVMS easier to change and to test.

@Jeongseup
Copy link
Contributor

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.

@qwertzlbert
Copy link
Contributor Author

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 :(

@Jeongseup
Copy link
Contributor

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

qwertzlbert and others added 5 commits February 7, 2025 15:32
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>
@qwertzlbert qwertzlbert force-pushed the refacture-client-implementation branch from 28a6494 to 370743a Compare February 7, 2025 14:33
@qwertzlbert
Copy link
Contributor Author

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

@Jeongseup
Copy link
Contributor

Could you share test GRPC endpoint for TLS? I used my one but not working. I want to check your endpoint format.

@qwertzlbert
Copy link
Contributor Author

@Jeongseup It should be as simple as using: grpc: 'band-grpc.polkachu.com:22990' or grpc: 'axelar-grpc.publicnode.com:443'
The client implementation should automatically detect if TLS is supported or not.

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

@Jeongseup
Copy link
Contributor

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

@Jeongseup
Copy link
Contributor

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!

@qwertzlbert
Copy link
Contributor Author

@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 api package, so not the full fledged cosmos-sdk.
The blank imports in https://github.com/cosmostation/cvms/pull/37/files#diff-b2155c0479756324081c9093a7222373280327747f79b8641f7f4ecb499b87e5R26 are just to register the decoders and make them available globally so the protobuf resolver can find it.

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.

2 participants