-
Notifications
You must be signed in to change notification settings - Fork 109
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
feat: add common rpc package #2788
Conversation
Important Review skippedAuto incremental reviews are disabled on this repository. Please check the settings in the CodeRabbit UI or the You can disable this status message by setting the WalkthroughWalkthroughThe changes introduce a new Changes
Sequence Diagram(s)sequenceDiagram
participant Client
participant RPC
participant CometBFT
participant gRPC
Client->>RPC: Request to initialize Clients
RPC->>CometBFT: Initialize CometBFT Client
CometBFT-->>RPC: Return CometBFT Client
RPC->>gRPC: Initialize gRPC Client
gRPC-->>RPC: Return gRPC Client
RPC-->>Client: Return Clients struct
Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
Documentation and Community
|
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## develop #2788 +/- ##
===========================================
- Coverage 66.80% 66.79% -0.02%
===========================================
Files 368 370 +2
Lines 20661 20677 +16
===========================================
+ Hits 13803 13811 +8
- Misses 6223 6230 +7
- Partials 635 636 +1
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Actionable comments posted: 4
Outside diff range, codebase verification and nitpick comments (1)
pkg/rpc/rpc.go (1)
19-28
: Add documentation for each field in theClients
struct.While the struct is well-defined, adding documentation for each field will improve readability and maintainability.
Consider adding comments like this:
// Clients contains RPC client interfaces to interact with zetacored type Clients struct { // AuthorityClient is used to interact with the authority module. AuthorityClient authoritytypes.QueryClient // CctxClient is used to interact with the crosschain module. CctxClient crosschaintypes.QueryClient // FungibleClient is used to interact with the fungible module. FungibleClient fungibletypes.QueryClient // AuthClient is used to interact with the auth module. AuthClient authtypes.QueryClient // BankClient is used to interact with the bank module. BankClient banktypes.QueryClient // ObserverClient is used to interact with the observer module. ObserverClient observertypes.QueryClient // LightClient is used to interact with the lightclient module. LightClient lightclienttypes.QueryClient }
Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Files selected for processing (1)
- pkg/rpc/rpc.go (1 hunks)
Additional context used
Path-based instructions (1)
pkg/rpc/rpc.go (1)
Pattern
**/*.go
: Review the Go code, point out issues relative to principles of clean code, expressiveness, and performance.
GitHub Check: codecov/patch
pkg/rpc/rpc.go
[warning] 30-39: pkg/rpc/rpc.go#L30-L39
Added lines #L30 - L39 were not covered by tests
[warning] 43-46: pkg/rpc/rpc.go#L43-L46
Added lines #L43 - L46 were not covered by tests
[warning] 48-48: pkg/rpc/rpc.go#L48
Added line #L48 was not covered by tests
[warning] 50-50: pkg/rpc/rpc.go#L50
Added line #L50 was not covered by tests
[warning] 54-57: pkg/rpc/rpc.go#L54-L57
Added lines #L54 - L57 were not covered by tests
[warning] 59-60: pkg/rpc/rpc.go#L59-L60
Added lines #L59 - L60 were not covered by tests
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think this is a direction that makes sense
Using grpc was more a shortcut and we should indeed remove it
8e29b2b
to
aaadc78
Compare
We currently have logic to create zetacored rpc clients in both e2e and zetaclient/zetacore. We should deduplicate this logic and export it publicly so that we can easily make type safe RPCs in other codebases.
Also add a cometbft client so that it's easier to run e2e tests against anyone who exposes a cometbft/tendermint rpc interface. gRPC is not always exposed publicly.
Closes #2786
External imports degraded by #959. Fixed by #2796
TODO:
zetaclient/zetacore.clients
struct. Update: embed therpc.Clients
struct rather than replacing as it has some other items.rpc.Clients
is currently limited to read only operations.cmd/zetace2/config.zetaChainClients
structzetacore_rpc
rather thanzetacore_grpc
ifzetacore_grpc
is unset in e2e configSummary by CodeRabbit
Clients
interface for easier interaction with various services within the ZetaChain ecosystem.