feat: add balance checker #17
Conversation
Summary of ChangesHello @chatton, I'm Gemini Code Assist1! I'm currently reviewing this pull request and will post my feedback shortly. In the meantime, here's a summary to help you and other reviewers quickly get up to speed! This pull request enhances the monitoring tool by integrating a balance checker for Celestia addresses. It allows users to track the native token holdings of specified accounts and provides visibility into the health and reliability of the consensus RPC endpoints used for these queries, all exposed via Prometheus metrics. This adds a critical layer of operational insight for blockchain infrastructure. Highlights
Using Gemini Code AssistThe full guide for Gemini Code Assist can be found on our documentation page, here are some quick tips. Invoking Gemini You can request assistance from Gemini at any point by creating a comment using either
Customization To customize Gemini Code Assist for GitHub experience, repository maintainers can create a configuration file and/or provide a custom code review style guide (such as PEP-8 for Python) by creating and adding files to a Limitations & Feedback Gemini Code Assist may make mistakes. Please leave feedback on any instances where its feedback is incorrect or counter productive. You can react with 👍 and 👎 on @gemini-code-assist comments. If you're interested in giving your feedback about your experience with Gemini Code Assist for Github and other Google products, sign up here. You can also get AI-powered code generation, chat, as well as code reviews directly in the IDE at no cost with the Gemini Code Assist IDE Extension. Footnotes
|
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request introduces a balance monitoring feature, which is a great addition. The implementation includes a new Cosmos client, a balance exporter, and corresponding Prometheus metrics. My review focuses on improving security, efficiency, and consistency. I've identified a critical security issue with insecure gRPC connections, a high-severity performance issue due to inefficient connection handling, and a minor documentation inconsistency. The proposed changes will make the new feature more robust and secure.
| grpcEndpoint := stripScheme(endpoint) | ||
|
|
||
| conn, err := grpc.NewClient(grpcEndpoint, grpc.WithTransportCredentials(insecure.NewCredentials())) |
There was a problem hiding this comment.
Using insecure.NewCredentials() establishes a non-TLS gRPC connection, which is a significant security risk when connecting to public RPC endpoints as shown in the README examples (https://...). This exposes traffic to potential eavesdropping and man-in-the-middle attacks. The client should dynamically use TLS when the endpoint URL has an https:// prefix.
Note: This change requires adding the google.golang.org/grpc/credentials import.
| grpcEndpoint := stripScheme(endpoint) | |
| conn, err := grpc.NewClient(grpcEndpoint, grpc.WithTransportCredentials(insecure.NewCredentials())) | |
| useTLS := strings.HasPrefix(endpoint, "https://") | |
| grpcEndpoint := stripScheme(endpoint) | |
| var transportCreds credentials.TransportCredentials | |
| if useTLS { | |
| transportCreds = credentials.NewTLS(nil) | |
| } else { | |
| transportCreds = insecure.NewCredentials() | |
| } | |
| conn, err := grpc.NewClient(grpcEndpoint, grpc.WithTransportCredentials(transportCreds)) |
| for _, address := range e.addresses { | ||
| e.checkBalance(ctx, m, address) | ||
| } |
There was a problem hiding this comment.
This loop calls checkBalance for each address, and checkBalance in turn loops through endpoints, creating a new gRPC connection each time. This is inefficient as it can result in many connections per scrape interval. It's better to fetch all balances using a single connection per available endpoint. I'm suggesting changing this to a single function call. Please see my other comment on the checkBalance function for the implementation of checkBalances.
e.checkBalances(ctx, m)| // checkBalance queries balance with fallback across endpoints | ||
| func (e *exporter) checkBalance(ctx context.Context, m *metrics.Metrics, address string) { | ||
| // try each endpoint until one succeeds | ||
| for _, endpoint := range e.endpoints { | ||
| // create client | ||
| client, err := cosmos.NewClient(endpoint, e.logger) | ||
| if err != nil { | ||
| e.logger.Warn(). | ||
| Err(err). | ||
| Str("endpoint", endpoint). | ||
| Str("address", address). | ||
| Msg("failed to create client, trying next endpoint") | ||
| m.RecordConsensusRpcEndpointAvailability(e.chainID, endpoint, false) | ||
| m.RecordConsensusRpcEndpointError(e.chainID, endpoint, utils.CategorizeError(err)) | ||
| continue | ||
| } | ||
|
|
||
| // query balance | ||
| balances, err := client.GetBalance(ctx, address) | ||
|
|
||
| // close client | ||
| _ = client.Close() | ||
|
|
||
| if err != nil { | ||
| e.logger.Warn(). | ||
| Err(err). | ||
| Str("endpoint", endpoint). | ||
| Str("address", address). | ||
| Msg("failed to query balance, trying next endpoint") | ||
| m.RecordConsensusRpcEndpointAvailability(e.chainID, endpoint, false) | ||
| m.RecordConsensusRpcEndpointError(e.chainID, endpoint, utils.CategorizeError(err)) | ||
| continue | ||
| } | ||
|
|
||
| // success - record availability | ||
| m.RecordConsensusRpcEndpointAvailability(e.chainID, endpoint, true) | ||
|
|
||
| // record balances for each denom | ||
| for _, coin := range balances { | ||
| // convert cosmos math.Int to string then to big.Float | ||
| amountStr := coin.Amount.String() | ||
| amount, ok := new(big.Float).SetString(amountStr) | ||
| if !ok { | ||
| e.logger.Warn(). | ||
| Str("amount", amountStr). | ||
| Str("denom", coin.Denom). | ||
| Msg("failed to parse balance amount") | ||
| continue | ||
| } | ||
|
|
||
| balanceFloat, _ := amount.Float64() | ||
| m.RecordAccountBalance(e.chainID, address, coin.Denom, balanceFloat) | ||
|
|
||
| e.logger.Info(). | ||
| Str("address", address). | ||
| Str("denom", coin.Denom). | ||
| Float64("balance", balanceFloat). | ||
| Str("endpoint", endpoint). | ||
| Msg("recorded account balance") | ||
| } | ||
|
|
||
| // successfully queried from this endpoint | ||
| return | ||
| } | ||
|
|
||
| // all endpoints failed | ||
| e.logger.Error(). | ||
| Str("address", address). | ||
| Msg("failed to query balance from all endpoints") | ||
| } |
There was a problem hiding this comment.
To improve efficiency, this function should be replaced with a new function checkBalances that handles all addresses in one go, avoiding the creation of multiple gRPC connections. This new function will iterate through endpoints, and for each successful connection, it will query balances for all addresses. This also includes logging the error from client.Close() instead of ignoring it. This change is related to my other comment in ExportMetrics.
// checkBalances queries balance for all addresses with fallback across endpoints
func (e *exporter) checkBalances(ctx context.Context, m *metrics.Metrics) {
// try each endpoint until one succeeds for all addresses
for _, endpoint := range e.endpoints {
client, err := cosmos.NewClient(endpoint, e.logger)
if err != nil {
e.logger.Warn().
Err(err).
Str("endpoint", endpoint).
Msg("failed to create client, trying next endpoint")
m.RecordConsensusRpcEndpointAvailability(e.chainID, endpoint, false)
m.RecordConsensusRpcEndpointError(e.chainID, endpoint, utils.CategorizeError(err))
continue
}
allSucceeded := true
for _, address := range e.addresses {
balances, err := client.GetBalance(ctx, address)
if err != nil {
e.logger.Warn().
Err(err).
Str("endpoint", endpoint).
Str("address", address).
Msg("failed to query balance, trying next endpoint")
m.RecordConsensusRpcEndpointAvailability(e.chainID, endpoint, false)
m.RecordConsensusRpcEndpointError(e.chainID, endpoint, utils.CategorizeError(err))
allSucceeded = false
break // This endpoint is faulty, try the next one
}
// success for this address - record availability for endpoint
m.RecordConsensusRpcEndpointAvailability(e.chainID, endpoint, true)
// record balances for each denom
for _, coin := range balances {
amountStr := coin.Amount.String()
amount, ok := new(big.Float).SetString(amountStr)
if !ok {
e.logger.Warn().
Str("amount", amountStr).
Str("denom", coin.Denom).
Msg("failed to parse balance amount")
continue
}
balanceFloat, _ := amount.Float64()
m.RecordAccountBalance(e.chainID, address, coin.Denom, balanceFloat)
e.logger.Info().
Str("address", address).
Str("denom", coin.Denom).
Float64("balance", balanceFloat).
Str("endpoint", endpoint).
Msg("recorded account balance")
}
}
if err := client.Close(); err != nil {
e.logger.Warn().Err(err).Str("endpoint", endpoint).Msg("failed to close client connection")
}
if allSucceeded {
return // All done for this scrape interval
}
}
// all endpoints failed for at least one address
e.logger.Error().
Msg("failed to query balances from all endpoints")
}| --header-namespace collect_testnet_header \ | ||
| --data-namespace collect_testnet_data \ |
There was a problem hiding this comment.
The namespaces in this example (collect_testnet_header, collect_testnet_data) are inconsistent with the changes made elsewhere in this README, where they were updated to testnet_header and testnet_data. For consistency, this example should also use the updated namespace names.
| --header-namespace collect_testnet_header \ | |
| --data-namespace collect_testnet_data \ | |
| --header-namespace testnet_header \ | |
| --data-namespace testnet_data \ |
| addressList := strings.Split(flags.balanceAddresses, ",") | ||
| rpcUrls := strings.Split(flags.balanceRpcUrls, ",") |
There was a problem hiding this comment.
While splitting comma-separated strings is a common pattern, it can be brittle if users accidentally add extra spaces (e.g., "addr1, addr2"). Using strings.FieldsFunc with a custom splitter can make parsing more robust by trimming whitespace automatically.
| addressList := strings.Split(flags.balanceAddresses, ",") | |
| rpcUrls := strings.Split(flags.balanceRpcUrls, ",") | |
| addressList := strings.FieldsFunc(flags.balanceAddresses, func(c rune) bool { return c == ',' }) | |
| rpcUrls := strings.FieldsFunc(flags.balanceRpcUrls, func(c rune) bool { return c == ',' }) |
Overview
Adds a new balance monitor which monitors specified addresses and provides their balance as metrics.
closes #15