v0.6.0#14
Merged
Merged
Conversation
…to transaction general to get tx hashes.
…ctures. Updated descriptions for height range validation and modified response types to use pointers for block data. Introduced helper methods in transaction handler for cleaner code and improved error handling.
… privilege validation. Introduce AllTableNames function to streamline privilege assignment for users.
…management database parameters
…gregate views and adjust continuous aggregate SQL generation with chunk interval.
…ethods for getting data from the view tables.
…thods for transaction and block data retrieval. Update address, block, and transaction handlers to use the new interfaces.
…action counts, refactor route registration, and enhance input validation for address and transaction types.
…amps instead of dates for improved precision in different timezones and consistency in block and transaction count queries.
…ogic. Replace log.Fatalf with structured logging and simplify address management functions for better clarity and performance.
… and performance.
|
Warning Rate limit exceeded
⌛ How to resolve this issue?After the wait time has elapsed, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans have higher rate limits than the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout. Please see our FAQ for further information. 📝 WalkthroughWalkthroughAdds API key management, valkey-based rate limiting and keystore, new analytics endpoints and continuous-aggregate support, many DB read/query additions, handler/interface refactors, route wiring, CLI/key tooling, server bootstrap changes, and large indexer/materialize and SQL datatype additions. Changes
Sequence Diagram(s)sequenceDiagram
participant Client
participant RL as RateLimiter
participant KS as KeyStore
participant VK as Valkey
participant DB as KeyDB
Client->>RL: HTTP request (may include X-API-Key)
RL->>KS: GetKeyLimit(hash) / lookup
KS-->>RL: (limit, found?)
alt API key provided & valid
RL->>VK: Increment(identifier, ctx)
VK-->>RL: (count, err)
alt first increment
RL->>VK: Expirer(identifier, window)
VK-->>RL: (ok, err)
end
alt count > limit
RL-->>Client: 429 + Retry-After
else within limit
RL-->>Client: forward request + X-RateLimit-* headers
end
else API key invalid
RL-->>Client: 401 Unauthorized
end
Note over KS,DB: KS periodically Refresh() -> DB.GetAllApiKeysWithLimits()
sequenceDiagram
participant Client
participant Handler
participant DB
participant MV as MaterializedView
Client->>Handler: Request with date-range
Handler->>Handler: validate input (dates/ranges)
alt invalid
Handler-->>Client: 400 Bad Request
else valid
Handler->>DB: GetBlockCountByDate(start,end)
DB->>MV: read continuous aggregate
MV-->>DB: aggregated rows
DB-->>Handler: []*BlockCountByDate
alt empty
Handler-->>Client: 404 Not Found
else
Handler-->>Client: 200 OK + data
end
end
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Possibly related PRs
Poem
✨ Finishing Touches🧪 Generate unit tests (beta)
|
Review Summary by Qodov0.6.0: Add rate limiting, API key management, and transaction analytics with comprehensive refactoring
WalkthroughsDescription• **API Rate Limiting**: Added comprehensive rate limiting middleware using Valkey with support for both IP-based and API key-based limiting, including proper HTTP headers and distributed request counting • **API Key Management**: Implemented complete API key management system with CRUD operations, CLI commands, key generation, and periodic refresh mechanism with database synchronization • **Transaction & Block Statistics**: Added new analytics endpoints for transaction counts, volumes, and block statistics with time-range queries and proper validation • **Database Optimization**: Refactored concurrent processing in data processor with helper functions, optimized block queries with parallel data fetching, and added continuous aggregate SQL generation utilities • **Code Refactoring**: Extracted large functions into smaller focused helpers across multiple files (operator.go, setup.go, main.go, decoder.go), improved error handling with structured logging, and split monolithic DatabaseHandler interface into specialized interfaces • **TimescaleDB Improvements**: Added materialized view data types, continuous aggregate query methods, updated compression configuration to newer API, and implemented aggregate refresh functionality • **Testing**: Added comprehensive test coverage for address cache, rate limiter middleware, and keystore functionality • **Bug Fixes**: Fixed multiple typos (TrasnactionsData → TransactionsData, specifly → specify), updated reflection API usage for Go 1.22+ compatibility, and corrected privilege parameter naming • **Configuration & Dependencies**: Added Valkey service to Docker Compose, updated Go dependencies (added decimal, valkey-glide, removed gozstd), and enhanced environment configuration for rate limiting Diagramflowchart LR
A["API Requests"] -->|"Rate Limiting"| B["Valkey Client"]
A -->|"API Key Auth"| C["KeyStore"]
C -->|"Periodic Sync"| D["API Key DB"]
E["Indexer"] -->|"Process Blocks"| F["Refactored Operator"]
F -->|"Concurrent Processing"| G["Database"]
H["Analytics Queries"] -->|"Aggregates"| I["Materialized Views"]
I -->|"Query Methods"| J["TimescaleDB"]
K["CLI Commands"] -->|"Manage Keys"| D
File Changes1. indexer/data_processor/operator.go
|
Code Review by Qodo
1.
|
![qodo-code-review[bot]](https://avatars.githubusercontent.com/in/484649?s=60&v=4){kind=small}
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 7
Note
Due to the large number of review comments, Critical severity comments were prioritized as inline comments.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (3)
indexer/decoder/decoder.go (1)
224-235:⚠️ Potential issue | 🔴 CriticalBug:
extractCoinsproduces duplicate entries due to pre-allocation + append.The slice is pre-allocated with
len(amount)zero-valued elements, thenappendadds more elements. This results in a slice with2*len(amount)entries: the first half are zero-valuedCoin{}structs.🐛 Fix: Use indexed assignment or zero-length slice
Option 1: Use indexed assignment (preferred)
func extractCoins(amount std.Coins) ([]Coin, error) { coins := make([]Coin, len(amount)) - for _, coin := range amount { - coins = append(coins, Coin{ + for i, coin := range amount { + coins[i] = Coin{ Amount: coin.Amount, Denom: coin.Denom, - }) + } } return coins, nil }Option 2: Pre-allocate capacity only
func extractCoins(amount std.Coins) ([]Coin, error) { - coins := make([]Coin, len(amount)) + coins := make([]Coin, 0, len(amount)) for _, coin := range amount {🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@indexer/decoder/decoder.go` around lines 224 - 235, The extractCoins function pre-allocates a slice with make([]Coin, len(amount)) then uses append, causing duplicate entries; fix by either replacing append with indexed assignment into the pre-sized slice (coins[i] = Coin{...}) inside the loop over amount, or change the allocation to a zero-length slice with capacity (make([]Coin, 0, len(amount))) and keep append; update the loop in extractCoins and ensure Coin and std.Coins usage remains consistent.compression/train/process.go (1)
99-132:⚠️ Potential issue | 🟡 MinorprocessEvents silently skips marshal errors but never returns an error.
The function signature returns
errorbut always returnsnil. Marshal failures are logged and skipped. Consider either:
- Removing the error return if errors are intentionally non-fatal, or
- Returning an error (or aggregated errors) if any marshaling fails.
Current behavior may hide data loss issues silently.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@compression/train/process.go` around lines 99 - 132, The function processEvents currently swallows proto.Marshal errors and always returns nil error; update it so marshal failures are propagated: either remove the error return from processEvents if you decide failures are non-fatal, or (recommended) keep the signature and return an error when proto.Marshal fails (e.g., capture the marshal error from proto.Marshal and return it immediately or aggregate multiple errors and return a combined error after the loop). Locate processEvents and the proto.Marshal call and modify the control flow to propagate the error (include the underlying err in the returned error) instead of only logging and continuing.indexer/db_init/tables.go (1)
574-594:⚠️ Potential issue | 🟠 MajorQuote or validate identifiers before concatenating these
GRANTstatements.
userNameand eachtableNameare inserted verbatim into SQL. Names with punctuation or quotes will break setup, and untrusted values can inject additional SQL.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@indexer/db_init/tables.go` around lines 574 - 594, The AppointPrivileges function builds GRANT statements by concatenating userName and tableName directly into the SQL, which risks broken identifiers and SQL injection; update AppointPrivileges to validate the privilege value as before, but instead of inserting raw identifiers use a proper identifier-quoting helper (e.g., QuoteIdentifier from your SQL driver or a small utility that wraps identifiers in double quotes and escapes embedded quotes) for both userName and each tableName when appending to the strings.Builder, and avoid interpolating untrusted input into the SQL; ensure the "keymgr" branch also quotes "api_keys" consistently and keep the same GRANT verbs.
🟠 Major comments (15)
indexer/address_cache/address.go-108-110 (1)
108-110:⚠️ Potential issue | 🟠 MajorMissing error logging and potential silent failure.
When
FindExistingAccountsfails, the error is silently swallowed andnilis returned. This causesinsertWithRetryandfetchAndCacheInsertedto no-op, effectively losing track of addresses that should be inserted. Consider logging the error and returning the originaladdressesslice to allow the insert attempt to proceed.🐛 Proposed fix
existing, err := a.db.FindExistingAccounts(ctx, addresses, chainName, insertValidators) if err != nil { - return nil + l.Error().Caller().Stack().Err(err).Msg("error finding existing accounts, proceeding with insert attempt") + return addresses }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@indexer/address_cache/address.go` around lines 108 - 110, The call to FindExistingAccounts currently swallows errors and returns nil; change this so errors are logged and the original addresses slice is returned to allow downstream insertWithRetry and fetchAndCacheInserted to proceed. Locate the block that checks "if err != nil" after calling FindExistingAccounts in the function in address.go, replace the silent return with a process (or logger) error call that includes the error and then return the original addresses slice (not nil) so inserts are attempted; ensure the log message references FindExistingAccounts and the surrounding context (e.g., function handling addresses) for easier debugging.indexer/data_processor/operator.go-194-229 (1)
194-229:⚠️ Potential issue | 🟠 MajorFailed blocks leave zero-valued entries in the result slice.
Unlike
processTransactionwhich uses avalidflag to filter failed entries,processBlockreturns early on errors but leaves zero-valuedsqlDataTypes.Blocksstructs inblocksData. These will be inserted into the database, potentially causing integrity issues (e.g., height=0, empty hash).🔧 Suggested fix: Add validity tracking like processTransaction
func (d *DataProcessor) ProcessBlocks(blocks []*rpcClient.BlockResponse, fromHeight uint64, toHeight uint64) { blockAmount := len(blocks) blocksData := make([]sqlDataTypes.Blocks, blockAmount) + valid := make([]bool, blockAmount) wg := sync.WaitGroup{} wg.Add(blockAmount) for idx, block := range blocks { - go d.processBlock(idx, block, &wg, blocksData) + go d.processBlock(idx, block, &wg, &valid[idx], blocksData) } wg.Wait() + // Collect only successfully processed blocks + result := make([]sqlDataTypes.Blocks, 0, blockAmount) + for idx, ok := range valid { + if ok { + result = append(result, blocksData[idx]) + } + } + timeout := 10*time.Second + (time.Duration(blockAmount) * time.Second / 5) ctx, cancel := context.WithTimeout(context.Background(), timeout) defer cancel() - err := d.dbPool.InsertBlocks(ctx, blocksData) + err := d.dbPool.InsertBlocks(ctx, result)🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@indexer/data_processor/operator.go` around lines 194 - 229, processBlock currently returns early on decode/parse errors and leaves zero-valued entries in blocksData; fix by adding validity tracking like processTransaction: extend processBlock signature to accept a valid []bool (or a pointer to a bool slice) and set valid[idx] = true only after successfully populating blocksData[idx]; on any early return leave valid[idx] false so the caller can filter out invalid entries before database insertion. Ensure you update the caller to pass the same valid slice and to filter blocksData by valid flags like processTransaction does.docker-compose-dev.yml-70-72 (1)
70-72:⚠️ Potential issue | 🟠 MajorUse health-based
depends_onto wait for service readiness.The
apiservice at lines 70-72 uses simpledepends_onformat that only ensures startup order. Sincetimescaledbandvalkeyhave healthchecks defined, update to health-based conditions to ensure the API doesn't start until both services are healthy:depends_on: timescaledb: condition: service_healthy valkey: condition: service_healthyThe
indexerservice (lines 42-43) should similarly be updated to reference thetimescaledbhealthcheck condition.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@docker-compose-dev.yml` around lines 70 - 72, Update the docker-compose depends_on for the api and indexer services to use health-based conditions so they wait for dependent services to be healthy: replace the simple list-style depends_on under the api service (currently referencing timescaledb and valkey) with a map that sets timescaledb: condition: service_healthy and valkey: condition: service_healthy, and similarly change the indexer service's depends_on (currently referencing timescaledb) to a map with timescaledb: condition: service_healthy so both services will wait for the defined healthchecks to pass before starting.docker-compose.yml-56-58 (1)
56-58:⚠️ Potential issue | 🟠 MajorUse health-gated
depends_onfor API startup reliability.The current
depends_onconfiguration (simple list format) only controls container start order—it does not wait for services to be healthy. Both timescaledb and valkey define healthchecks, so the API can start and attempt connections before these services are ready, causing race conditions and failures.Convert to health-gated conditions:
🛠️ Proposed fix
api: depends_on: - - timescaledb - - valkey + timescaledb: + condition: service_healthy + valkey: + condition: service_healthyApply this change to both
docker-compose.yml(lines 56-58) anddocker-compose-dev.yml(lines 70-72).🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@docker-compose.yml` around lines 56 - 58, Replace the simple list-style depends_on with health-gated depends_on for the API service so it waits for the services' healthchecks; specifically change the depends_on block that currently lists "timescaledb" and "valkey" to use objects with health conditions (e.g., depends_on: timescaledb: condition: service_healthy and valkey: condition: service_healthy) and apply the same replacement in both docker-compose.yml and docker-compose-dev.yml so the API will only start after timescaledb and valkey report healthy.api/valkey/client.go-37-43 (1)
37-43: 🛠️ Refactor suggestion | 🟠 MajorContext should be the first parameter per Go conventions.
Go idiom places
context.Contextas the first parameter. The current signature(key string, ctx context.Context)is non-idiomatic and inconsistent with standard library patterns.♻️ Suggested fix
-func (c *ValkeyClient) Increment(key string, ctx context.Context) (int64, error) { +func (c *ValkeyClient) Increment(ctx context.Context, key string) (int64, error) { return c.client.Incr(ctx, key) } -func (c *ValkeyClient) Expirer(key string, ctx context.Context, expiration time.Duration) (bool, error) { +func (c *ValkeyClient) Expirer(ctx context.Context, key string, expiration time.Duration) (bool, error) { return c.client.Expire(ctx, key, expiration) }Note: This will require updating callers in
api/ratelimit/ratelimit.goand any other consumers.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@api/valkey/client.go` around lines 37 - 43, The method parameter order is non-idiomatic: move context.Context to be the first parameter on ValkeyClient methods by changing Increment(key string, ctx context.Context) to Increment(ctx context.Context, key string) and Expirer(key string, ctx context.Context, expiration time.Duration) to Expirer(ctx context.Context, key string, expiration time.Duration); update all callers (e.g., usages in api/ratelimit/ratelimit.go and any other consumers) to pass the context first and run a build to catch remaining call sites.go.mod-65-73 (1)
65-73:⚠️ Potential issue | 🟠 MajorUpdate OpenTelemetry exporter versions to v1.42.0 to match the SDK.
OpenTelemetry-Go requires all stable (v1.x) modules to share the exact same version number. The current configuration mixes
otel/sdk v1.42.0with exporters atv1.40.0, which is an unsupported combination per the project's versioning policy. Update all exporter packages tov1.42.0:
go.opentelemetry.io/otel/exporters/otlp/otlpmetric/otlpmetricgrpcgo.opentelemetry.io/otel/exporters/otlp/otlpmetric/otlpmetrichttpgo.opentelemetry.io/otel/exporters/otlp/otlptracego.opentelemetry.io/otel/exporters/otlp/otlptrace/otlptracehttp🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@go.mod` around lines 65 - 73, The go.mod currently mixes OpenTelemetry modules at v1.40.0 with SDK modules at v1.42.0; update the exporter module versions to match the SDK by changing the versions for go.opentelemetry.io/otel/exporters/otlp/otlpmetric/otlpmetricgrpc, go.opentelemetry.io/otel/exporters/otlp/otlpmetric/otlpmetrichttp, go.opentelemetry.io/otel/exporters/otlp/otlptrace, and go.opentelemetry.io/otel/exporters/otlp/otlptrace/otlptracehttp to v1.42.0 so all stable v1.x otel modules use the identical version, then run the appropriate go tooling (e.g., go get or go mod tidy) to update the lockfile.api/handlers/address.go-32-35 (1)
32-35:⚠️ Potential issue | 🟠 MajorReturn 5xx for non-not-found failures.
Any DB timeout or connection error from
GetDailyActiveAccountis currently rewritten to 404, which makes outages indistinguishable from missing data. Reserve 404 for an explicit no-data condition and surface the rest as 5xx.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@api/handlers/address.go` around lines 32 - 35, The handler currently maps any error from h.db.GetDailyActiveAccount to a 404; change this to only return huma.Error404NotFound when the DB error explicitly indicates no-data (e.g., compare err to sql.ErrNoRows or your repository's NotFound sentinel/error type), and for all other errors return a 5xx error (e.g., huma.Error500InternalServerError) including the original err; update the error branch around the GetDailyActiveAccount call to perform this conditional check and return the appropriate huma error while preserving the original error details.api/keystore/keystore.go-12-23 (1)
12-23:⚠️ Potential issue | 🟠 MajorHide the backing store behind a one-method interface.
With
dbfixed to*database.TimescaleDb, the fake inapi/keystore/keystore_test.gocannot be injected, so the new tests have to copyRefreshbehavior by hand instead of exercisingKeyStore.Refresh/KeyStore.StartPeriodicRefresh. That leaves regressions in the real refresh path effectively untested.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@api/keystore/keystore.go` around lines 12 - 23, The KeyStore currently hardcodes db as *database.TimescaleDb which prevents injecting a fake in tests; define a minimal one-method interface (e.g., KeyStoreDB with the single method used by KeyStore.Refresh), change KeyStore.db to that interface type and update NewKeyStore signature to accept the interface, implement an adapter that wraps *database.TimescaleDb for production, and update api/keystore/keystore_test.go to inject a fake implementing that interface so tests exercise KeyStore.Refresh and KeyStore.StartPeriodicRefresh directly.api/key_cmd.go-88-91 (1)
88-91:⚠️ Potential issue | 🟠 MajorDon't silently fall back to
5432whenKEY_DB_PORTis malformed.If the env var is set to a non-number, this path ignores the parse failure and later uses the default port. That can point key-management commands at the wrong database instead of surfacing the config error.
Proposed fix
+import "strconv" + ... if params.port == 0 { if v := os.Getenv("KEY_DB_PORT"); v != "" { - fmt.Sscanf(v, "%d", ¶ms.port) + port, err := strconv.Atoi(v) + if err != nil { + return nil, fmt.Errorf("invalid KEY_DB_PORT %q: %w", v, err) + } + params.port = port } }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@api/key_cmd.go` around lines 88 - 91, The code currently ignores parse errors when reading KEY_DB_PORT into params.port, causing a silent fallback to the default 5432; change the parsing so a malformed KEY_DB_PORT surfaces an error (don’t let params.port silently remain 0). Replace the fmt.Sscanf call with strconv.Atoi (or use fmt.Sscan with error handling) to capture parse errors, and if parsing fails return or propagate an explicit error from the command initialization (or log and exit) so callers know the env var was invalid; reference the params.port variable and the KEY_DB_PORT env var when implementing the check.pkgs/database/key_mgmt.go-155-168 (1)
155-168:⚠️ Potential issue | 🟠 MajorInconsistent hash parameter format.
EnableKeypasseshash(the[32]bytearray) directly to the query at line 160, whileDisableKey(line 145) andAdjustRpmLimit(line 177) passhash[:](a slice). This inconsistency may cause unexpected behavior depending on how pgx handles the parameter.🐛 Proposed fix
func (t *TimescaleDb) EnableKey(ctx context.Context, hash [32]byte) error { result, err := t.pool.Exec(ctx, ` UPDATE api_keys SET is_active = true WHERE hash = $1 - `, hash) + `, hash[:]) if err != nil { return err }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@pkgs/database/key_mgmt.go` around lines 155 - 168, EnableKey currently passes the [32]byte array value directly to t.pool.Exec while DisableKey and AdjustRpmLimit use hash[:] (a byte slice), causing inconsistent parameter types; change the Exec call in EnableKey (function EnableKey) to pass hash[:] instead of hash so the query parameter type matches the other functions and avoids pgx type-mismatch behavior, keep the rest of the update/rows-affected handling unchanged.pkgs/database/query_block.go-138-145 (1)
138-145:⚠️ Potential issue | 🟠 MajorTransaction query does not align with the blocks being fetched.
query2fetches an arbitraryLIMIT $2of transactions without filtering by the block heights returned inquery1. This means transactions may be fetched for blocks outside the "last X blocks" range.Consider using a subquery or filtering by
block_height >= (SELECT MIN(height) FROM ...)to ensure transactions correspond to the returned blocks.🐛 Proposed fix
query2 := ` SELECT encode(tx_hash, 'base64'), block_height FROM transaction_general WHERE chain_name = $1 - LIMIT $2 + AND block_height IN ( + SELECT height FROM blocks + WHERE chain_name = $1 + ORDER BY height DESC + LIMIT $2 + ) `🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@pkgs/database/query_block.go` around lines 138 - 145, query2 currently selects an arbitrary LIMIT $2 of transactions and can return txs from blocks outside the "last X blocks" returned by query1; update the transaction query (query2) to restrict results to the block heights produced by query1 by adding a filter on transaction_general.block_height (for example using a subquery or IN clause that selects heights from the same source used in query1 or by requiring block_height >= (SELECT MIN(height) FROM ... ) ), ensure you keep the chain_name/$1 filter and the LIMIT/$2 parameter, and reference the transaction_general table and columns tx_hash and block_height when making this change.pkgs/database/query_view.go-55-64 (1)
55-64:⚠️ Potential issue | 🟠 MajorCoalesce empty aggregates to zero.
SUM(...)returnsNULLwhen the window has no rows. Scanning that intoint64turns “no data in this window” into an error/404 for these count endpoints, when they should return0instead.Possible fix
- SUM(block_count) as block_count + COALESCE(SUM(block_count), 0) as block_count - SUM(transaction_count) as total_tx_count + COALESCE(SUM(transaction_count), 0) as total_tx_count - SUM(transaction_count) as tx_count_24h + COALESCE(SUM(transaction_count), 0) as tx_count_24hAlso applies to: 111-120, 131-141
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@pkgs/database/query_view.go` around lines 55 - 64, The SUM(...) aggregate in the SQL query can return NULL for empty windows which causes row.Scan into int64 (variable blockCount) to fail; update the SQL to wrap the aggregate with COALESCE, e.g. COALESCE(SUM(block_count), 0) in the query string used before the call to t.pool.QueryRow(ctx, query, chainName) so row.Scan(&blockCount) always receives 0 instead of NULL; apply the same COALESCE fix to the other aggregate queries referenced (the blocks around lines 111-120 and 131-141) so their scanned int64/count variables never get NULL.api/handlers/transactions.go-215-217 (1)
215-217:⚠️ Potential issue | 🟠 MajorDon't surface storage failures as 400s.
At this point the request has already been validated and the transaction has been found. If one of these follow-up fetches fails, that's a missing-data/backend problem, not a bad request. Returning 400 here will misclassify valid requests and break retry/monitoring behavior. Please reserve 400 for malformed inputs, use 404 only when the message payload is genuinely absent, and return a 5xx for query failures.
Also applies to: 246-248, 277-279, 308-310
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@api/handlers/transactions.go` around lines 215 - 217, The handler is returning huma.Error400BadRequest for storage/query failures (e.g., the fmt.Sprintf call that mentions "vm_msg_call" and txHash and the similar blocks at the other locations), but these are backend/missing-data issues; change the logic so that when the follow-up fetch fails you distinguish between "not found" (return a 404 Not Found) and other storage/query errors (return a 5xx, e.g., huma.Error500InternalServerError) and include the original error in the log/response context; update the three similar sites referenced (the block with "vm_msg_call" and the other blocks at the cited ranges) to use this pattern.indexer/cmd/setup.go-108-113 (1)
108-113:⚠️ Potential issue | 🟠 MajorMake
setup create-dbresumable.If anything after
CreateDatabasefails, rerunning againstpostgreswill fail on the existing DB, while rerunning against the target DB takes thecurrentDb == newDbNamebranch and skips the remaining setup entirely. Please split “create DB if missing” from “initialize schema” so the command can safely resume after a partial failure.Also applies to: 130-164
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@indexer/cmd/setup.go` around lines 108 - 113, The current logic returns early when currentDb == newDbName which skips schema initialization on retry; change it to split DB creation from schema initialization by first ensuring the database exists (call or extract CreateDatabase into a helper like ensureDatabaseExists or reuse the existing CreateDatabase call against the postgres connection when currentDb != newDbName) and then always call initializeNewDatabase (or the schema init function) against the target DB regardless of whether creation was needed; update the branch that uses currentDb/newDbName so it only skips CreateDatabase but still invokes initializeNewDatabase (apply the same split to the other similar block referenced in lines 130-164).pkgs/database/query_view.go-325-339 (1)
325-339:⚠️ Potential issue | 🟠 MajorThis 24h rollup drops buckets where the validator missed every block.
Starting from
validator_signing_countermeans any hour with zero signed blocks has novscrow, so it never participates in the join. That undercountstotal_blocks, underreports missed blocks, and inflatessigning_rate_pct. Drive this aggregation fromblock_counterand left join the validator counts instead.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@pkgs/database/query_view.go` around lines 325 - 339, The current SQL (assigned to query2) incorrectly drives the 24h rollup from validator_signing_counter (vsc), which omits hours with zero signed blocks; rewrite the query to aggregate from block_counter (bc) as the primary table and LEFT JOIN validator_signing_counter vsc ON bc.time_bucket = vsc.time_bucket AND bc.chain_name = vsc.chain_name AND vsc.validator_id = $2, then filter by bc.chain_name = $1 and bc.time_bucket in the 24-hour window; recompute blocks_signed using coalesce(sum(vsc.blocks_signed),0), blocks_not_signed as coalesce(sum(bc.block_count),0) - coalesce(sum(vsc.blocks_signed),0), total_blocks from bc, and signing_rate_pct using the same nullif-safe division so missing vsc rows do not drop buckets.
🟡 Minor comments (5)
api/handlers/address.go-25-26 (1)
25-26:⚠️ Potential issue | 🟡 MinorAllow a single-day window.
This check rejects
start_date == end_date, so clients can't request exactly one day's daily-active data.Proposed fix
- if !input.StartDate.Before(input.EndDate) { - return nil, huma.Error400BadRequest("start_date must be before end_date", nil) + if input.StartDate.After(input.EndDate) { + return nil, huma.Error400BadRequest("start_date must be on or before end_date", nil) }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@api/handlers/address.go` around lines 25 - 26, The current validation rejects equal start and end dates by using if !input.StartDate.Before(input.EndDate) — allow single-day windows by changing the check to only fail when start is after end (use input.StartDate.After(input.EndDate)); update the error path that returns huma.Error400BadRequest("start_date must be before end_date", nil) to remain but ensure it only triggers on After, preserving the existing message or adjust to "start_date must be on or before end_date" if you prefer clearer wording; locate and modify the conditional that references input.StartDate and input.EndDate in the handler function where this validation occurs.pkgs/database/types_api.go-172-173 (1)
172-173:⚠️ Potential issue | 🟡 Minor
omitemptyneeds to live inside thejsontag.
encoding/jsononly recognizes options attached to thejsonkey in comma-separated format, so the current tag still emits"time":nullfor a nilTimeinstead of omitting it.Proposed fix
type ValidatorSigning struct { - Time *time.Time `json:"time" doc:"Time" omitempty:"true"` + Time *time.Time `json:"time,omitempty" doc:"Time"` BlocksSigned int64 `json:"blocks_signed" doc:"Blocks signed"` BlocksMissed int64 `json:"blocks_missed" doc:"Blocks missed"` TotalBlocks int64 `json:"blocks_total" doc:"Total blocks"`🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@pkgs/database/types_api.go` around lines 172 - 173, The struct field ValidatorSigning.Time uses omitempty as a separate tag key which encoding/json ignores; update the struct tag on ValidatorSigning.Time so the json key includes the omitempty option (e.g., change the json tag to include ",omitempty") and remove the standalone omitempty:"true" tag while preserving the doc tag; locate ValidatorSigning and update the Time field's struct tag accordingly.api/serve.go-135-137 (1)
135-137:⚠️ Potential issue | 🟡 MinorMinor typo in API documentation.
"APi" should be "API" in the security scheme description.
📝 Proposed fix
Description: `API key for authenticated access with a higher rate limit. Pass your key in the X-API-Key header. - You can query the APi without the API key but the stricter rate limit will be applied.`, + You can query the API without the API key but the stricter rate limit will be applied.`,🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@api/serve.go` around lines 135 - 137, The Description string for the security scheme in api/serve.go contains a typo ("APi") — update the Description value (the Description field inside the security scheme definition) to use "API" instead of "APi" so the text reads "You can query the API without the API key but the stricter rate limit will be applied."; keep the rest of the string unchanged.api/serve.go-171-179 (1)
171-179:⚠️ Potential issue | 🟡 MinorError handling after partial write is ineffective.
If
w.Write(favicon)partially succeeds (writes some bytes but returns an error), callinghttp.Errorwill fail silently because headers have already been sent. The error is logged, which is good, buthttp.Errorwon't change the response status at that point.Consider restructuring to avoid misleading error handling:
🛡️ Proposed fix
router.Get("/favicon.ico", func(w http.ResponseWriter, r *http.Request) { w.Header().Set("Content-Type", "image/x-icon") _, err := w.Write(favicon) if err != nil { log.Printf("failed to write favicon: %v", err) - http.Error(w, "Internal Server Error", http.StatusInternalServerError) - return + // Headers already sent; cannot change response status } })🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@api/serve.go` around lines 171 - 179, The handler for "/favicon.ico" currently calls http.Error after a write that may have partially succeeded, which is ineffective once headers/body have been sent; change the handler in the router.Get("/favicon.ico", ...) block to first write the favicon into an in-memory buffer (e.g., bytes.Buffer or similar), set the Content-Length header from the buffer size, then write the buffer to w; if w.Write returns an error, only log the error (do not call http.Error) because the response may already be partially sent. Ensure you update the handler code paths around the Write of favicon to follow this buffering-and-log-only approach.api/handlers/blocks.go-47-49 (1)
47-49:⚠️ Potential issue | 🟡 MinorInconsistent error message.
The condition
input.ToHeight-input.FromHeight > 100allows up to 101 blocks (from=1, to=101), but the error message says "difference must be less than 100". Consider updating the message to "must be at most 100" or changing the condition to>= 100if you want a maximum of 100 blocks.📝 Proposed fix (if 100 blocks max is intended)
- if input.ToHeight-input.FromHeight > 100 { - return nil, huma.Error400BadRequest("From height and to height difference must be less than 100", nil) + if input.ToHeight-input.FromHeight >= 100 { + return nil, huma.Error400BadRequest("Range must not exceed 100 blocks", nil) }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@api/handlers/blocks.go` around lines 47 - 49, The check on input.ToHeight and input.FromHeight is inconsistent with its message: change the condition from `input.ToHeight-input.FromHeight > 100` to `input.ToHeight-input.FromHeight >= 100` and update the huma.Error400BadRequest message to "From height and to height difference must be at most 100" (adjust the string passed to huma.Error400BadRequest where the current check lives).
🧹 Nitpick comments (15)
indexer/data_processor/operator.go (2)
498-503: Consider moving slice assignment outside the critical section.The
decodedMsgs[idx]assignment doesn't require mutex protection since each goroutine writes to its own pre-allocated index. Moving it outside the lock reduces contention:♻️ Suggested optimization
mu.Lock() for _, address := range addresses { (*addressesMap)[address] = struct{}{} } - decodedMsgs[idx] = decodedMsg mu.Unlock() + decodedMsgs[idx] = decodedMsg🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@indexer/data_processor/operator.go` around lines 498 - 503, The critical section currently holds mu while adding addresses to addressesMap and also while writing decodedMsgs[idx]; move only the addressesMap updates inside the mu.Lock()/mu.Unlock() and perform the decodedMsgs[idx] = decodedMsg assignment outside the lock since each goroutine writes to its own pre-allocated index (use the existing variables mu, addressesMap, addresses, decodedMsg, and idx to identify the code to change).
406-413: Redundant bounds check -idxfrom range will never exceed slice length.The guard
if idx >= transactionAmountis unreachable sinceidxcomes from iteratingrange transactionsandtransactionAmount = len(transactions). This check can be removed to reduce noise.♻️ Simplified loop
for idx, transaction := range transactions { - // Guard against index out of bounds - if idx >= transactionAmount { - wg.Done() - continue - } go d.processMessageGroup(idx, transaction, allDecodedMsgs[idx], &wg, &msgResults) }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@indexer/data_processor/operator.go` around lines 406 - 413, The bounds check "if idx >= transactionAmount { wg.Done(); continue }" is redundant because idx comes from ranging over transactions and transactionAmount == len(transactions); remove that entire if-block so the loop simply ranges and launches goroutines with go d.processMessageGroup(idx, transaction, allDecodedMsgs[idx], &wg, &msgResults). Ensure wg.Add was called appropriately before this loop and that processMessageGroup is responsible for calling wg.Done() so the waitgroup accounting remains correct.api/config/loader.go (1)
90-102: Consider extracting shared env-loading helper.
LoadValkeyEnvironmentrepeats the same read+parse flow used byLoadEnvironment. A shared helper would reduce duplication and keep error behavior consistent.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@api/config/loader.go` around lines 90 - 102, Extract the duplicated read+parse logic into a shared helper (e.g., a function like loadEnvInto(reader EnvFileReader, path string, out interface{}) error) and use it from both LoadValkeyEnvironment and LoadEnvironment; the helper should call reader.ReadFile(path) and then env.Parse(out), returning wrapped errors consistently, and the existing functions should simply create their specific struct (ValkeyEnv or the other env struct), call the helper to populate it, and return the pointer or error.api/key/generate.go (1)
10-17: Extract key-format constants to avoid drift.Lines 10 and 17 use magic values (
32,10) tied to key format. A small constants block would make future format changes safer.♻️ Proposed refactor
func GenerateApiKey() (string, string, [32]byte, error) { - rawKey := make([]byte, 32) + const ( + apiKeyByteLen = 32 + apiKeyPrefix = "api_" + keyPrefixLen = 10 + ) + rawKey := make([]byte, apiKeyByteLen) _, err := rand.Read(rawKey) if err != nil { return "", "", [32]byte{}, err } - rawKeyHex := "api_" + hex.EncodeToString(rawKey) + rawKeyHex := apiKeyPrefix + hex.EncodeToString(rawKey) keyHash := sha256.Sum256([]byte(rawKeyHex)) - keyPrefix := rawKeyHex[:10] + keyPrefix := rawKeyHex[:keyPrefixLen] return rawKeyHex, keyPrefix, keyHash, nil }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@api/key/generate.go` around lines 10 - 17, Replace the magic numbers used when generating the key by introducing named constants (e.g., apiKeyRandomBytes = 32, apiKeyPrefix = "api_", apiKeyPrefixLen = 10) and use them when creating rawKey, building rawKeyHex and slicing keyPrefix; update the uses in the rawKey, rawKeyHex and keyPrefix expressions (and keep keyHash calculation as-is) so future format changes only require updating the constants.CHANGELOG.md (2)
29-29: Section header should be "Changed" for consistency.The Keep a Changelog format uses "Changed" (see line 45, 60, 97). This section uses "Changes" which is inconsistent.
📝 Suggested fix
-### Changes +### Changed🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@CHANGELOG.md` at line 29, Replace the inconsistent section header "### Changes" with the canonical Keep a Changelog header "### Changed" so it matches other sections (e.g., lines showing "### Changed"); update the header string used in the CHANGELOG.md to "### Changed" wherever this instance appears.
12-16: Minor grammar improvements recommended.A few grammar/style refinements per static analysis:
📝 Suggested fixes
-The zstd compression has been added. This it is not production ready and still in development, however +The zstd compression has been added. This is not production-ready and still in development, however initial testing has been done and it seems to work, about 30% less storage is required. -The API has been improved with some new routes and minor improvements. The cursor based pagination has been added +The API has been improved with some new routes and minor improvements. The cursor-based pagination has been added🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@CHANGELOG.md` around lines 12 - 16, Fix small grammar and punctuation issues in the changelog: in the zstd sentence change "This it is not production ready and still in development, however" to "It is not production-ready and is still in development" and rephrase "about 30% less storage is required." to "it requires about 30% less storage."; in the API paragraph hyphenate "cursor-based pagination" and change "The API now also has POST utilities to convert between Base64 and Base64URL." to "The API now includes POST utilities for converting between Base64 and Base64URL." Update these exact phrases in CHANGELOG.md so wording, hyphenation, and sentence flow are corrected.api/valkey/client.go (1)
15-31: Consider making timeout configurable.The 5-second timeout is hardcoded. For production flexibility, consider accepting timeout as a parameter or via configuration.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@api/valkey/client.go` around lines 15 - 31, The NewValkeyClient function currently hardcodes timeout := 5 * time.Second; make the timeout configurable by adding a timeout parameter (or reading from a config) to NewValkeyClient and use that value when calling cfg.WithConnectionTimeout and cfg.WithRequestTimeout; update the function signature (e.g., NewValkeyClient(host string, port int, timeout time.Duration) or accept an options/config struct) and ensure ValkeyClient initialization and any callers are adjusted to pass the desired timeout rather than the fixed 5s.compression/train/process.go (1)
77-84: Consider making debug output configurable.
DebugOut: os.Stdoutwill write debug information to stdout unconditionally. For production use, consider making this configurable or using a proper logger.♻️ Suggested approach
-func BuildZstdDict(events [][]byte) ([]byte, error) { +func BuildZstdDict(events [][]byte, debugWriter io.Writer) ([]byte, error) { const maxHistorySize = 112 << 10 // ... dict, err := zstd.BuildDict(zstd.BuildDictOptions{ ID: 1, Contents: events, History: history, Level: lvl, - DebugOut: os.Stdout, + DebugOut: debugWriter, // pass nil to disable, or os.Stdout for CLI usage })🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@compression/train/process.go` around lines 77 - 84, The BuildDictOptions currently sets DebugOut: os.Stdout which always writes debug info; change it to be configurable by exposing a logger/output option (or reading an env/config flag) and pass that value into zstd.BuildDictOptions.DebugOut instead of os.Stdout, or set DebugOut to nil when debug is disabled; update the code around zstd.BuildDict and the variable (e.g., lvl, dict, err in process.go) to obtain the output stream from the new config (or a logger wrapper) and use it in BuildDictOptions so debug output can be toggled or redirected in production.api/ratelimit/ratelimit_test.go (1)
123-136: Simplify hex encoding using standard library.The inline hex encoding function can be replaced with
encoding/hex:♻️ Suggested simplification
+import "encoding/hex" + // ensure key-based identifier used (first 8 bytes of hash) short := (h[:8]) -id := "key:" + (func() string { return (func(b []byte) string { - const hextable = "0123456789abcdef" - out := make([]byte, len(b)*2) - for i, v := range b { - out[i*2] = hextable[v>>4] - out[i*2+1] = hextable[v&0x0f] - } - return string(out) -})(short) })() +id := "key:" + hex.EncodeToString(short)🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@api/ratelimit/ratelimit_test.go` around lines 123 - 136, Replace the custom inline hex encoder used to build id from short (the first 8 bytes of h) with the standard library: call encoding/hex's EncodeToString on short to produce the hex string when constructing id (variable names: h, short, id) so the test becomes simpler and more readable while keeping the subsequent check against vk.expireCalls["rl:"+id] unchanged.go.mod (1)
35-35: Dependency changed from indirect to direct without being in main require block.
github.com/cosmos/gogoproto v1.7.2was changed from indirect to direct (missing// indirectcomment) but appears in the secondary require block rather than the main one. If this is intentionally a direct dependency, consider moving it to the main require block (lines 5-25) for clarity.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@go.mod` at line 35, The go.mod entry for github.com/cosmos/gogoproto v1.7.2 was promoted from indirect to direct but was left in the secondary require block; move the dependency line "github.com/cosmos/gogoproto v1.7.2" into the primary require block (the top require {...} block) so it is clearly a direct dependency, or if it should remain indirect add the "// indirect" comment after the version; update the require placement and comment accordingly to keep go.mod semantics consistent.api/huma-types/address.go (1)
28-31: Consider aligning field names with query parameter names for clarity.The struct field names (
StartDate,EndDate) don't match the query parameter names (start_timestamp,end_timestamp). While functional, this inconsistency may cause confusion during maintenance.💡 Suggested alignment
Either rename fields to match the query params:
type DailyActiveAccountGetInput struct { - StartDate time.Time `query:"start_timestamp" doc:"Start date (inclusive)" format:"date-time" required:"true"` - EndDate time.Time `query:"end_timestamp" doc:"End date (inclusive)" format:"date-time" required:"true"` + StartTimestamp time.Time `query:"start_timestamp" doc:"Start timestamp (inclusive)" format:"date-time" required:"true"` + EndTimestamp time.Time `query:"end_timestamp" doc:"End timestamp (inclusive)" format:"date-time" required:"true"` }Or update the query params to match the field semantics (if the intent is date-only):
type DailyActiveAccountGetInput struct { - StartDate time.Time `query:"start_timestamp" doc:"Start date (inclusive)" format:"date-time" required:"true"` - EndDate time.Time `query:"end_timestamp" doc:"End date (inclusive)" format:"date-time" required:"true"` + StartDate time.Time `query:"start_date" doc:"Start date (inclusive)" format:"date-time" required:"true"` + EndDate time.Time `query:"end_date" doc:"End date (inclusive)" format:"date-time" required:"true"` }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@api/huma-types/address.go` around lines 28 - 31, The struct DailyActiveAccountGetInput uses field names StartDate and EndDate that don't match the query tag names start_timestamp and end_timestamp; update one side for clarity and maintainability—either rename the fields to StartTimestamp and EndTimestamp to mirror the `query:"start_timestamp"`/`query:"end_timestamp"` tags, or change the query tags to `query:"start_date"`/`query:"end_date"` (and adjust any consumers) so the field names and query param names are consistent; locate the struct in api/huma-types/address.go and update both the field identifiers and any related docs or handlers referencing DailyActiveAccountGetInput to keep behavior intact.pkgs/sql_data_types/hypertables.go (1)
39-47: Consider extracting a shared helper forTableColumns()to reduce duplication.The
TableColumns()method is duplicated across all 8 structs with identical reflection logic. This violates DRY and increases maintenance burden.♻️ Proposed refactor: Extract a generic helper
Add a package-level helper function:
// GetTableColumnsFromStruct extracts column names from struct tags func GetTableColumnsFromStruct(v any) []string { columns := make([]string, 0) fields := reflect.TypeOf(v) for i := 0; i < fields.NumField(); i++ { field := fields.Field(i) columns = append(columns, field.Tag.Get("db")) } return columns }Then each struct's method becomes a one-liner:
func (b Blocks) TableColumns() []string { - columns := make([]string, 0) - fields := reflect.TypeOf(b) - for i := 0; i < fields.NumField(); i++ { - field := fields.Field(i) - columns = append(columns, field.Tag.Get("db")) - } - return columns + return GetTableColumnsFromStruct(b) }Also applies to: 76-85, 113-122, 164-173, 225-233, 295-304, 363-372, 422-432
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@pkgs/sql_data_types/hypertables.go` around lines 39 - 47, The TableColumns() reflection logic is duplicated across multiple structs (e.g., Blocks.TableColumns()), so extract a single package-level helper GetTableColumnsFromStruct(v any) that contains the reflect.TypeOf loop and tag extraction, then replace each struct method (like Blocks.TableColumns(), and the other seven identical TableColumns methods) with a one-line wrapper that calls GetTableColumnsFromStruct(b) to return the columns; this centralizes the logic and removes duplication.api/huma-types/validator.go (1)
9-11: Inconsistent validation constraints for ValidatorAddress.
ValidatorSigning24hGetInput.ValidatorAddresslacksminLengthandmaxLengthconstraints, whileValidatorSigningByHourGetInput.ValidatorAddress(line 18) hasminLength:"40" maxLength:"40". Consider adding the same constraints for consistent API validation.♻️ Proposed fix
type ValidatorSigning24hGetInput struct { - ValidatorAddress string `path:"validator_address" doc:"Validator consensus address" required:"true"` + ValidatorAddress string `path:"validator_address" doc:"Validator consensus address" required:"true" minLength:"40" maxLength:"40"` }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@api/huma-types/validator.go` around lines 9 - 11, The ValidatorSigning24hGetInput struct's ValidatorAddress field is missing the minLength and maxLength tags; update ValidatorSigning24hGetInput.ValidatorAddress to include the same validation tags used by ValidatorSigningByHourGetInput (minLength:"40" maxLength:"40") so both inputs enforce identical 40-character address constraints in their struct field tags.api/handlers/validators.go (1)
30-32: Consider distinguishing "not found" from other database errors.Both handlers return 404 for any database error. While this is a common pattern to avoid leaking internal details, it could make debugging difficult. A database connection failure would appear as "not found" to clients.
Consider logging the actual error internally (you're already passing it to
huma.Error404NotFound) and potentially returning 500 for non-"not found" errors if the database layer can distinguish them.Also applies to: 54-57
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@api/handlers/validators.go` around lines 30 - 32, The current handler always returns huma.Error404NotFound for any DB error when looking up input.ValidatorAddress; change the error handling so only a true "not found" sentinel (e.g., sql.ErrNoRows or your DB layer's NotFound error) maps to huma.Error404NotFound while all other errors are logged internally and return a 500-style error (e.g., huma.Error500InternalServerError); preserve the existing log of the actual error, use the specific error check before calling huma.Error404NotFound, and apply the same pattern to the other block referenced at lines 54-57.api/ratelimit/ratelimit.go (1)
99-104: Edge case in X-Forwarded-For parsing.If
X-Forwarded-Forstarts with a comma (malformed header like, 1.2.3.4),strings.IndexBytereturns 0, which fails the> 0check, causing the full malformed string to be returned. Consider using>= 0or handling the edge case:♻️ Proposed fix
func realIP(r *http.Request) string { if xff := r.Header.Get("X-Forwarded-For"); xff != "" { - if i := strings.IndexByte(xff, ','); i > 0 { + if i := strings.IndexByte(xff, ','); i >= 0 { + if i == 0 { + // Malformed: starts with comma, try next segment + parts := strings.SplitN(xff, ",", 3) + if len(parts) > 1 { + return strings.TrimSpace(parts[1]) + } + } return strings.TrimSpace(xff[:i]) } return strings.TrimSpace(xff) }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@api/ratelimit/ratelimit.go` around lines 99 - 104, The X-Forwarded-For parsing block using r.Header.Get("X-Forwarded-For") and strings.IndexByte(xff, ',') doesn't handle headers that start with a comma (e.g., ", 1.2.3.4") because the check i > 0 will fail and return the malformed string; change the logic to robustly extract the first non-empty IP token by splitting the xff value on ',' (or scanning for the first token) and returning the first token after trimming whitespace and leading commas; update the code around the strings.IndexByte(xff, ',') usage in this function to skip empty tokens and return a trimmed, non-empty IP string.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 630a5e19-3387-4b45-8d6c-82a01a8a3725
⛔ Files ignored due to path filters (1)
go.sumis excluded by!**/*.sum
📒 Files selected for processing (58)
.env.exampleCHANGELOG.mdMakefileapi/config/loader.goapi/config/types.goapi/handlers/address.goapi/handlers/blocks.goapi/handlers/blocks_test.goapi/handlers/database_test.goapi/handlers/interface.goapi/handlers/transactions.goapi/handlers/validators.goapi/huma-types/address.goapi/huma-types/block.goapi/huma-types/transaction.goapi/huma-types/validator.goapi/key/generate.goapi/key_cmd.goapi/keystore/keystore.goapi/keystore/keystore_test.goapi/main.goapi/ratelimit/ratelimit.goapi/ratelimit/ratelimit_test.goapi/routes/addresses.goapi/routes/blocks.goapi/routes/transactions.goapi/routes/utils.goapi/routes/validators.goapi/serve.goapi/valkey/client.gocompression/train/process.goconfig-api.yml.exampledocker-compose-dev.ymldocker-compose.ymlgo.modindexer/address_cache/address.goindexer/address_cache/address_test.goindexer/cmd/live.goindexer/cmd/setup.goindexer/data_processor/operator.goindexer/data_processor/types.goindexer/db_init/hypertable.goindexer/db_init/materialize.goindexer/db_init/tables.goindexer/decoder/decoder.goindexer/decoder/product.goindexer/decoder/types.goindexer/orchestrator/operator.goindexer/orchestrator/operator_test.goindexer/orchestrator/types.gopkgs/database/insert.gopkgs/database/key_mgmt.gopkgs/database/query_block.gopkgs/database/query_view.gopkgs/database/types_api.gopkgs/sql_data_types/hypertables.gopkgs/sql_data_types/materialize.gopkgs/sql_data_types/table.go
💤 Files with no reviewable changes (2)
- pkgs/database/insert.go
- indexer/cmd/live.go
There was a problem hiding this comment.
Actionable comments posted: 14
♻️ Duplicate comments (2)
indexer/db_init/hypertable.go (1)
73-80:⚠️ Potential issue | 🔴 Critical
timescaledb.segmentbyis still emitted with invalid SQL value formatting.
timescaledb.segmentby = %sinjects raw text instead of a string literal, which can breakALTER TABLE ... SET (...)parsing at runtime. This was already flagged earlier and appears unresolved.🐛 Proposed fix
- columnsString := strings.Join(columns, ", ") + columnsString := strings.Join(columns, ",") sql := fmt.Sprintf( ` ALTER TABLE %s SET ( timescaledb.enable_columnstore, - timescaledb.segmentby = %s, + timescaledb.segmentby = '%s', timescaledb.orderby = 'timestamp DESC' ); `, tableName, columnsString)#!/bin/bash # Verify unresolved raw segmentby interpolation still exists. rg -nP "timescaledb\.segmentby\s*=\s*%s" indexer/db_init/hypertable.go # Expected after fix: no matches.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@indexer/db_init/hypertable.go` around lines 73 - 80, The ALTER TABLE SQL in hypertable.go builds a string via fmt.Sprintf that injects columnsString into the timescaledb.segmentby clause without quoting, causing invalid SQL; update the fmt.Sprintf call that constructs sql (the ALTER TABLE ... SET (...) block) to emit a properly quoted string literal for timescaledb.segmentby (e.g., wrap the %s in single quotes or otherwise escape/quote columnsString) so the generated ALTER TABLE uses timescaledb.segmentby = '<columns>' instead of unquoted raw text.indexer/address_cache/address.go (1)
148-155:⚠️ Potential issue | 🔴 CriticalContext is cancelled before use, causing all inserts to fail.
Same issue as
insertWithRetry- the context is cancelled immediately after creation (line 150) before being used inInsertAddresses(line 152).🐛 Proposed fix: call cancel() after the operation
for _, addr := range addresses { ctx, cancel := context.WithTimeout(context.Background(), 5*time.Second) - cancel() if err := a.db.InsertAddresses(ctx, []string{addr}, chainName, insertValidators); err != nil { l.Error().Caller().Stack().Err(err).Msgf("error inserting address: %s", addr) } + cancel() }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@indexer/address_cache/address.go` around lines 148 - 155, The context is being cancelled immediately after creation which makes every call to a.db.InsertAddresses fail; move the cancel() call so it runs after the InsertAddresses call (and its error handling) instead of directly after context.WithTimeout — create ctx, cancel := context.WithTimeout(...), call a.db.InsertAddresses(ctx, ...), handle any error, then call cancel() (or ensure cancel is invoked in a defer scoped to just that iteration) to properly release the timeout context.
🧹 Nitpick comments (4)
indexer/data_processor/operator.go (3)
478-486: Move slice assignment outside the critical section.
decodedMsgs[idx] = decodedMsgdoesn't need mutex protection since each goroutine writes to its own index slot. Including it inside the lock unnecessarily extends the critical section and increases contention.♻️ Suggested fix
// Collect addresses from this transaction and store in map[string]struct{} addresses := decodedMsg.CollectAllAddresses() mu.Lock() for _, address := range addresses { (*addressesMap)[address] = struct{}{} } - decodedMsgs[idx] = decodedMsg mu.Unlock() + decodedMsgs[idx] = decodedMsg🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@indexer/data_processor/operator.go` around lines 478 - 486, The critical section around mu.Lock()/mu.Unlock() is larger than necessary: after collecting addresses via decodedMsg.CollectAllAddresses() you should only protect writes to the shared addressesMap; move the per-goroutine assignment decodedMsgs[idx] = decodedMsg to execute outside the locked region so the lock only wraps the loop that writes (*addressesMap)[address] = struct{}{} (i.e., call mu.Lock(), write into addressesMap, mu.Unlock(), then set decodedMsgs[idx] = decodedMsg).
704-711: Consider pre-estimating slice capacity.The slice is created with zero capacity and will grow through multiple
appendoperations. Since you already have the message counts, you could estimate a capacity to reduce allocations.♻️ Suggested improvement
func createAddressTx(msgGroups *decoder.DbMessageGroups) []sqlDataTypes.AddressTx { msgSendCount := len(msgGroups.MsgSend) msgCallCount := len(msgGroups.MsgCall) msgAddPkgCount := len(msgGroups.MsgAddPkg) msgRunCount := len(msgGroups.MsgRun) - addresses := make([]sqlDataTypes.AddressTx, 0) + // Estimate capacity (each message produces at least one AddressTx) + estimatedCap := msgSendCount + msgCallCount + msgAddPkgCount + msgRunCount + addresses := make([]sqlDataTypes.AddressTx, 0, estimatedCap)🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@indexer/data_processor/operator.go` around lines 704 - 711, createAddressTx currently builds addresses with make([]sqlDataTypes.AddressTx, 0) and repeatedly appends, which causes extra allocations; compute totalCount := len(msgGroups.MsgSend)+len(msgGroups.MsgCall)+len(msgGroups.MsgAddPkg)+len(msgGroups.MsgRun) (or use the existing msgSendCount/msgCallCount/msgAddPkgCount/msgRunCount variables) and initialize addresses with make([]sqlDataTypes.AddressTx, 0, totalCount) before appending so the slice has pre-allocated capacity and avoids growth overhead.
106-129: Consider batching mutex operations to reduce lock contention.The current implementation acquires and releases the mutex for each precommit individually, which increases contention when processing many blocks concurrently. Collecting all addresses locally first and then performing a single locked write would be more efficient.
♻️ Suggested improvement
func processPrecommits( mu *sync.Mutex, addressesMap *map[string]struct{}, wg *sync.WaitGroup, block *rpcClient.BlockResponse, ) { defer wg.Done() + // Collect addresses locally first + localAddrs := make([]string, 0) precommits := block.Result.Block.LastCommit.Precommits for _, precommit := range precommits { if precommit != nil { - mu.Lock() - (*addressesMap)[precommit.ValidatorAddress] = struct{}{} - mu.Unlock() + localAddrs = append(localAddrs, precommit.ValidatorAddress) } } + localAddrs = append(localAddrs, block.Result.Block.Header.ProposerAddress) - proposer := block.Result.Block.Header.ProposerAddress + // Single locked write mu.Lock() - (*addressesMap)[proposer] = struct{}{} + for _, addr := range localAddrs { + (*addressesMap)[addr] = struct{}{} + } mu.Unlock() }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@indexer/data_processor/operator.go` around lines 106 - 129, In processPrecommits, avoid locking per precommit by collecting validator addresses into a local temporary map or slice (e.g., localAddrs) while iterating precommits and adding the proposer, then acquire mu once and merge localAddrs into the shared addressesMap before unlocking; use the existing symbols precommits, proposer, addressesMap, and mu to locate the code and ensure wg.Done() remains deferred.pkgs/database/query_view.go (1)
311-323: Extract validator-ID resolution into a shared helper.The address→ID lookup query is duplicated in two methods. A small helper will keep error handling and query behavior consistent.
♻️ Suggested refactor sketch
+func (t *TimescaleDb) resolveValidatorID(ctx context.Context, validatorAddress, chainName string) (int32, error) { + query := ` + SELECT id + FROM gno_validators + WHERE address = $1 + AND chain_name = $2 + ` + var validatorID int32 + if err := t.pool.QueryRow(ctx, query, validatorAddress, chainName).Scan(&validatorID); err != nil { + return 0, fmt.Errorf("validator seems to not exist: %w", err) + } + return validatorID, nil +}Also applies to: 363-376
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@pkgs/database/query_view.go` around lines 311 - 323, Extract the duplicated address→ID lookup into a new helper (e.g., getValidatorIDByAddress or resolveValidatorID) that accepts context, validatorAddress and chainName and returns (int32, error); move the SQL, QueryRow, Scan and error-wrapping logic from the blocks around the id lookup (the code using query1/row1/validatorId) into this helper and replace both occurrences (the block at the current snippet and the similar one at lines ~363-376) with calls to the helper, ensuring you preserve the same error messages and behavior.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@api/key_cmd.go`:
- Around line 257-289: The list command (keyListCmd) opens a DB via connectKeyDb
but never closes it, causing a connection leak; after successfully obtaining db
(the variable returned from connectKeyDb) add a deferred close call (defer
db.Close()) immediately before creating the context so the database is always
closed when RunE returns, ensuring resources used by db.ListApiKeys are
released; place the defer right after the err check that follows connectKeyDb to
cover all return paths in RunE.
- Around line 205-255: The disable and enable CLI handlers (keyDisableCmd and
keyEnableCmd) open a DB via connectKeyDb but never close it; after successfully
obtaining db (the value returned by connectKeyDb in both RunE functions) add a
deferred close (e.g., defer db.Close()) immediately after the error check so the
connection is always released before function exit; apply the same fix in both
keyDisableCmd and keyEnableCmd to mirror keyCreateCmd's behavior.
- Around line 145-155: connectKeyDb returns a *database.TimescaleDb (wrapping a
connection pool) but the four key command handlers (keyCreateCmd, keyDisableCmd,
keyEnableCmd, keyListCmd) never close the pool, leaking connections; after
calling connectKeyDb in each handler, immediately defer closing the pool via
db.GetPool().Close() (or call Close() at the end of the handler if defer isn't
appropriate), ensuring you handle nil/db error cases (only defer when db != nil)
and keep the rest of the handler logic unchanged.
In `@indexer/address_cache/address.go`:
- Around line 130-142: The ctx is cancelled immediately after creation (ctx,
cancel := context.WithTimeout(...); cancel()), so InsertAddresses always
receives a cancelled context; move the cancel call to after the database call
(or use defer cancel() right after creating ctx if you change loop structure) so
that a.db.InsertAddresses(ctx, addresses, chainName, insertValidators) runs with
a valid context, then call cancel() (or let defer run) before the next
iteration; adjust the block around InsertAddresses and insertOneByOne to ensure
cancel() is invoked after the operation completes.
- Around line 108-110: The call to FindExistingAccounts in address.go currently
swallows errors by returning nil which prevents insertWithRetry from attempting
any inserts; update the error path in the block that checks "if err != nil" to
log the error (use the existing logger/context) and return the original
addresses slice (or otherwise propagate the error) so insertWithRetry still
processes addresses; reference the FindExistingAccounts call and the
insertWithRetry flow to ensure you preserve diagnostics and allow insertion
attempts instead of silently skipping them.
In `@indexer/data_processor/operator.go`:
- Around line 388-395: Remove the dead bounds-check inside the loop: the `if idx
>= transactionAmount { wg.Done(); continue }` is always false because
`transactionAmount = len(transactions)` and `idx` comes from `range
transactions`; delete that `if` block and its `wg.Done()` call and simply launch
the goroutine `go d.processMessageGroup(idx, transaction, allDecodedMsgs[idx],
&wg, &msgResults)` for each iteration. Ensure the `wg` counter (Add/Done pairs)
remains correct elsewhere after removing the skipped-branch.
- Around line 176-211: ProcessBlocks is currently vulnerable because
processBlock returns on error leaving zero-value entries in blocksData; modify
processBlock to accept a parallel valid []bool (or *bool pointer per index) and
set valid[idx]=true only on successful parsing (after decoding hash and parsing
height), and ensure the caller ProcessBlocks allocates and passes that valid
slice and filters blocksData to only include entries where valid is true before
DB insert (mirror the valid[] pattern used in ProcessTransactions); update
function signatures: processBlock(..., blocksData []sqlDataTypes.Blocks, valid
[]bool) and set valid[idx]=true at the end on success.
In `@indexer/db_init/hypertable.go`:
- Around line 93-104: The comment for the exported method is stale and has a
typo: replace the incorrect header "AddCompressionPolicy" and fix "TThis" so the
doc matches the actual method name AddColumnstorePolicy on type DBInitializer;
update the comment text to correctly describe that AddColumnstorePolicy adds the
columnstore policy for given tables and remove the duplicate/incorrect
characters so generated docs and linters reflect the correct symbol name and
wording.
In `@pkgs/database/key_mgmt.go`:
- Around line 37-61: GetAllApiKeys is missing a rows.Err() check after iterating
rows; after the for rows.Next() loop (and before returning apiKeys) call if err
:= rows.Err(); err != nil { return nil, err } so iteration errors (e.g.,
network) are surfaced—mirror the pattern used in
GetAllApiKeysWithLimits/ListApiKeys and keep the existing defer rows.Close().
In `@pkgs/database/query_block.go`:
- Around line 152-154: The SQL in GetLastXBlocks uses "block_height >= (SELECT
MAX(height) ...) - $2" which yields x+1 rows; change the lower bound to compute
MAX - $2 + 1 so the range is inclusive and returns exactly x rows (i.e. use
"block_height >= (SELECT MAX(height) FROM blocks WHERE chain_name = $1) - $2 +
1"). Update the query in the GetLastXBlocks code path accordingly so it matches
the intended x-sized window (and remains ordered DESC).
- Around line 83-90: GetLatestBlock (and similarly GetLastXBlocks) currently
runs query1 (ORDER BY height DESC LIMIT 1) and query2 (which uses SELECT
MAX(height) in its WHERE) concurrently which can return mismatched heights if a
new block is inserted mid-run; change the flow to first fetch the block height
from query1 (use the returned blocks[0].Height) and then run the transaction
query (query2) using that concrete height as a parameter instead of the MAX()
subquery so both queries see the same height; apply the same pattern in
GetLastXBlocks by computing the exact height(s) up front and passing them as
parameters to the tx query rather than embedding MAX() twice so you don’t get
off-by-one or extra tx rows.
In `@pkgs/database/query_view.go`:
- Around line 53-60: Queries that SUM() over empty result sets can return NULL
and break scanning into int64s; update the SQL in GetBlockCount24h,
GetBlockCountByDate, GetTotalTxCount, and GetTotalTxCount24h to wrap the
aggregate with COALESCE so it returns 0 instead of NULL (e.g. change
SUM(block_count) to COALESCE(SUM(block_count), 0) and SUM(transaction_count) to
COALESCE(SUM(transaction_count), 0)) so the scanned values into the int64
variables succeed.
- Around line 331-332: The signing_rate_pct expression can be NULL when total
blocks is zero and will break scanning into the non-nullable float64 field
ValidatorSigning.SigningRate; change the SQL to return 0 instead of NULL by
wrapping the rounded expression with COALESCE (e.g. COALESCE(round(...), 0) AS
signing_rate_pct) (or coerce to 0.0) and apply the same change in both
validator-signing queries so the scan into ValidatorSigning.SigningRate never
receives NULL.
- Around line 380-393: The query in GetValidatorSigningByHour uses unqualified
time_bucket references causing ambiguous column errors when joining
validator_signing_counter and block_counter; qualify the time_bucket argument in
time_bucket_gapfill('1 hour', time_bucket) and in the GROUP BY time_bucket('1
hour', time_bucket) to reference the intended table (e.g., vsc.time_bucket) so
the expressions become time_bucket_gapfill('1 hour', vsc.time_bucket) and GROUP
BY time_bucket('1 hour', vsc.time_bucket), ensuring all time_bucket uses
consistently reference validator_signing_counter (vsc).
---
Duplicate comments:
In `@indexer/address_cache/address.go`:
- Around line 148-155: The context is being cancelled immediately after creation
which makes every call to a.db.InsertAddresses fail; move the cancel() call so
it runs after the InsertAddresses call (and its error handling) instead of
directly after context.WithTimeout — create ctx, cancel :=
context.WithTimeout(...), call a.db.InsertAddresses(ctx, ...), handle any error,
then call cancel() (or ensure cancel is invoked in a defer scoped to just that
iteration) to properly release the timeout context.
In `@indexer/db_init/hypertable.go`:
- Around line 73-80: The ALTER TABLE SQL in hypertable.go builds a string via
fmt.Sprintf that injects columnsString into the timescaledb.segmentby clause
without quoting, causing invalid SQL; update the fmt.Sprintf call that
constructs sql (the ALTER TABLE ... SET (...) block) to emit a properly quoted
string literal for timescaledb.segmentby (e.g., wrap the %s in single quotes or
otherwise escape/quote columnsString) so the generated ALTER TABLE uses
timescaledb.segmentby = '<columns>' instead of unquoted raw text.
---
Nitpick comments:
In `@indexer/data_processor/operator.go`:
- Around line 478-486: The critical section around mu.Lock()/mu.Unlock() is
larger than necessary: after collecting addresses via
decodedMsg.CollectAllAddresses() you should only protect writes to the shared
addressesMap; move the per-goroutine assignment decodedMsgs[idx] = decodedMsg to
execute outside the locked region so the lock only wraps the loop that writes
(*addressesMap)[address] = struct{}{} (i.e., call mu.Lock(), write into
addressesMap, mu.Unlock(), then set decodedMsgs[idx] = decodedMsg).
- Around line 704-711: createAddressTx currently builds addresses with
make([]sqlDataTypes.AddressTx, 0) and repeatedly appends, which causes extra
allocations; compute totalCount :=
len(msgGroups.MsgSend)+len(msgGroups.MsgCall)+len(msgGroups.MsgAddPkg)+len(msgGroups.MsgRun)
(or use the existing msgSendCount/msgCallCount/msgAddPkgCount/msgRunCount
variables) and initialize addresses with make([]sqlDataTypes.AddressTx, 0,
totalCount) before appending so the slice has pre-allocated capacity and avoids
growth overhead.
- Around line 106-129: In processPrecommits, avoid locking per precommit by
collecting validator addresses into a local temporary map or slice (e.g.,
localAddrs) while iterating precommits and adding the proposer, then acquire mu
once and merge localAddrs into the shared addressesMap before unlocking; use the
existing symbols precommits, proposer, addressesMap, and mu to locate the code
and ensure wg.Done() remains deferred.
In `@pkgs/database/query_view.go`:
- Around line 311-323: Extract the duplicated address→ID lookup into a new
helper (e.g., getValidatorIDByAddress or resolveValidatorID) that accepts
context, validatorAddress and chainName and returns (int32, error); move the
SQL, QueryRow, Scan and error-wrapping logic from the blocks around the id
lookup (the code using query1/row1/validatorId) into this helper and replace
both occurrences (the block at the current snippet and the similar one at lines
~363-376) with calls to the helper, ensuring you preserve the same error
messages and behavior.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 562c30b0-a38d-4ec1-b70f-d4bec1b10a4e
📒 Files selected for processing (12)
api/config/types.goapi/key_cmd.goapi/ratelimit/ratelimit.goapi/ratelimit/ratelimit_test.goapi/serve.goconfig-api.yml.exampleindexer/address_cache/address.goindexer/data_processor/operator.goindexer/db_init/hypertable.gopkgs/database/key_mgmt.gopkgs/database/query_block.gopkgs/database/query_view.go
🚧 Files skipped from review as they are similar to previous changes (4)
- config-api.yml.example
- api/config/types.go
- api/ratelimit/ratelimit_test.go
- api/serve.go
Comment on lines
+145
to
+155
| func connectKeyDb(cmd *cobra.Command) (*database.TimescaleDb, error) { | ||
| params, err := parseKeyDbFlags(cmd) | ||
| if err != nil { | ||
| return nil, err | ||
| } | ||
| params.password, err = keyPromptPassword() | ||
| if err != nil { | ||
| return nil, err | ||
| } | ||
| return database.NewTimescaleDbSetup(params.createDatabaseConfig()), nil | ||
| } |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Search for Close method on TimescaleDb
ast-grep --pattern $'func ($_ *TimescaleDb) Close() $_ {
$$$
}'Repository: Cogwheel-Validator/spectra-gnoland-indexer
Length of output: 68
🏁 Script executed:
# Find the database.TimescaleDb type definition
find . -name "*.go" -type f | xargs grep -l "type TimescaleDb" | head -5Repository: Cogwheel-Validator/spectra-gnoland-indexer
Length of output: 110
🏁 Script executed:
# Also search for the database package structure
find . -path "*/database/*.go" -type fRepository: Cogwheel-Validator/spectra-gnoland-indexer
Length of output: 407
🏁 Script executed:
# Search for Close methods in the database package
find . -path "*/database/*.go" -type f -exec grep -l "Close" {} \;Repository: Cogwheel-Validator/spectra-gnoland-indexer
Length of output: 327
🏁 Script executed:
# Read TimescaleDb type definition
cat -n ./pkgs/database/types.go | head -100Repository: Cogwheel-Validator/spectra-gnoland-indexer
Length of output: 941
🏁 Script executed:
# Search for Close method in database files, showing context
rg "func.*Close" ./pkgs/database/ -A 3Repository: Cogwheel-Validator/spectra-gnoland-indexer
Length of output: 68
🏁 Script executed:
# Check the key_mgmt.go file to see how connections are handled
cat -n ./pkgs/database/key_mgmt.goRepository: Cogwheel-Validator/spectra-gnoland-indexer
Length of output: 5158
🏁 Script executed:
# Look at pool.go to see connection management
cat -n ./pkgs/database/pool.goRepository: Cogwheel-Validator/spectra-gnoland-indexer
Length of output: 7062
🏁 Script executed:
# Search for actual Close calls in the codebase, not just definitions
rg "\.Close\(\)" --type go -B 2Repository: Cogwheel-Validator/spectra-gnoland-indexer
Length of output: 4560
🏁 Script executed:
# Read the actual key_cmd.go file to see the command handlers
cat -n ./api/key_cmd.goRepository: Cogwheel-Validator/spectra-gnoland-indexer
Length of output: 9580
Add missing database connection cleanup to all four key command handlers.
connectKeyDb returns a *database.TimescaleDb (wrapping a connection pool), but none of the command handlers close the underlying pool after use. This leaks database connection resources. All four subcommands (create, disable, enable, list) are affected.
🔧 Corrected fix for keyCreateCmd (apply similar pattern to other commands)
db, err := connectKeyDb(cmd)
if err != nil {
return err
}
+ defer db.GetPool().Close()
ctx, cancel := context.WithTimeout(context.Background(), 15*time.Second)
defer cancel()Note: TimescaleDb does not have a Close() method; use db.GetPool().Close() instead.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@api/key_cmd.go` around lines 145 - 155, connectKeyDb returns a
*database.TimescaleDb (wrapping a connection pool) but the four key command
handlers (keyCreateCmd, keyDisableCmd, keyEnableCmd, keyListCmd) never close the
pool, leaking connections; after calling connectKeyDb in each handler,
immediately defer closing the pool via db.GetPool().Close() (or call Close() at
the end of the handler if defer isn't appropriate), ensuring you handle nil/db
error cases (only defer when db != nil) and keep the rest of the handler logic
unchanged.
Comment on lines
+205
to
+255
| var keyDisableCmd = &cobra.Command{ | ||
| Use: "disable", | ||
| Short: "Disable an API key by name", | ||
| RunE: func(cmd *cobra.Command, args []string) error { | ||
| name, _ := cmd.Flags().GetString("name") | ||
| if name == "" { | ||
| return fmt.Errorf("--name is required") | ||
| } | ||
|
|
||
| db, err := connectKeyDb(cmd) | ||
| if err != nil { | ||
| return err | ||
| } | ||
|
|
||
| ctx, cancel := context.WithTimeout(context.Background(), 15*time.Second) | ||
| defer cancel() | ||
|
|
||
| if err := db.DisableKeyByName(ctx, name); err != nil { | ||
| return fmt.Errorf("failed to disable key: %w", err) | ||
| } | ||
|
|
||
| log.Printf("API key %q disabled", name) | ||
| return nil | ||
| }, | ||
| } | ||
|
|
||
| var keyEnableCmd = &cobra.Command{ | ||
| Use: "enable", | ||
| Short: "Enable an API key by name", | ||
| RunE: func(cmd *cobra.Command, args []string) error { | ||
| name, _ := cmd.Flags().GetString("name") | ||
| if name == "" { | ||
| return fmt.Errorf("--name is required") | ||
| } | ||
|
|
||
| db, err := connectKeyDb(cmd) | ||
| if err != nil { | ||
| return err | ||
| } | ||
|
|
||
| ctx, cancel := context.WithTimeout(context.Background(), 15*time.Second) | ||
| defer cancel() | ||
|
|
||
| if err := db.EnableKeyByName(ctx, name); err != nil { | ||
| return fmt.Errorf("failed to enable key: %w", err) | ||
| } | ||
|
|
||
| log.Printf("API key %q enabled", name) | ||
| return nil | ||
| }, | ||
| } |
There was a problem hiding this comment.
Database connection leak in disable/enable commands.
Same issue as keyCreateCmd — the database connection should be closed after use.
🔧 Proposed fix for keyDisableCmd and keyEnableCmd
db, err := connectKeyDb(cmd)
if err != nil {
return err
}
+ defer db.Close()📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| var keyDisableCmd = &cobra.Command{ | |
| Use: "disable", | |
| Short: "Disable an API key by name", | |
| RunE: func(cmd *cobra.Command, args []string) error { | |
| name, _ := cmd.Flags().GetString("name") | |
| if name == "" { | |
| return fmt.Errorf("--name is required") | |
| } | |
| db, err := connectKeyDb(cmd) | |
| if err != nil { | |
| return err | |
| } | |
| ctx, cancel := context.WithTimeout(context.Background(), 15*time.Second) | |
| defer cancel() | |
| if err := db.DisableKeyByName(ctx, name); err != nil { | |
| return fmt.Errorf("failed to disable key: %w", err) | |
| } | |
| log.Printf("API key %q disabled", name) | |
| return nil | |
| }, | |
| } | |
| var keyEnableCmd = &cobra.Command{ | |
| Use: "enable", | |
| Short: "Enable an API key by name", | |
| RunE: func(cmd *cobra.Command, args []string) error { | |
| name, _ := cmd.Flags().GetString("name") | |
| if name == "" { | |
| return fmt.Errorf("--name is required") | |
| } | |
| db, err := connectKeyDb(cmd) | |
| if err != nil { | |
| return err | |
| } | |
| ctx, cancel := context.WithTimeout(context.Background(), 15*time.Second) | |
| defer cancel() | |
| if err := db.EnableKeyByName(ctx, name); err != nil { | |
| return fmt.Errorf("failed to enable key: %w", err) | |
| } | |
| log.Printf("API key %q enabled", name) | |
| return nil | |
| }, | |
| } | |
| var keyDisableCmd = &cobra.Command{ | |
| Use: "disable", | |
| Short: "Disable an API key by name", | |
| RunE: func(cmd *cobra.Command, args []string) error { | |
| name, _ := cmd.Flags().GetString("name") | |
| if name == "" { | |
| return fmt.Errorf("--name is required") | |
| } | |
| db, err := connectKeyDb(cmd) | |
| if err != nil { | |
| return err | |
| } | |
| defer db.Close() | |
| ctx, cancel := context.WithTimeout(context.Background(), 15*time.Second) | |
| defer cancel() | |
| if err := db.DisableKeyByName(ctx, name); err != nil { | |
| return fmt.Errorf("failed to disable key: %w", err) | |
| } | |
| log.Printf("API key %q disabled", name) | |
| return nil | |
| }, | |
| } | |
| var keyEnableCmd = &cobra.Command{ | |
| Use: "enable", | |
| Short: "Enable an API key by name", | |
| RunE: func(cmd *cobra.Command, args []string) error { | |
| name, _ := cmd.Flags().GetString("name") | |
| if name == "" { | |
| return fmt.Errorf("--name is required") | |
| } | |
| db, err := connectKeyDb(cmd) | |
| if err != nil { | |
| return err | |
| } | |
| defer db.Close() | |
| ctx, cancel := context.WithTimeout(context.Background(), 15*time.Second) | |
| defer cancel() | |
| if err := db.EnableKeyByName(ctx, name); err != nil { | |
| return fmt.Errorf("failed to enable key: %w", err) | |
| } | |
| log.Printf("API key %q enabled", name) | |
| return nil | |
| }, | |
| } |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@api/key_cmd.go` around lines 205 - 255, The disable and enable CLI handlers
(keyDisableCmd and keyEnableCmd) open a DB via connectKeyDb but never close it;
after successfully obtaining db (the value returned by connectKeyDb in both RunE
functions) add a deferred close (e.g., defer db.Close()) immediately after the
error check so the connection is always released before function exit; apply the
same fix in both keyDisableCmd and keyEnableCmd to mirror keyCreateCmd's
behavior.
Comment on lines
+257
to
+289
| var keyListCmd = &cobra.Command{ | ||
| Use: "list", | ||
| Short: "List all API keys", | ||
| RunE: func(cmd *cobra.Command, args []string) error { | ||
| db, err := connectKeyDb(cmd) | ||
| if err != nil { | ||
| return err | ||
| } | ||
|
|
||
| ctx, cancel := context.WithTimeout(context.Background(), 15*time.Second) | ||
| defer cancel() | ||
|
|
||
| keys, err := db.ListApiKeys(ctx) | ||
| if err != nil { | ||
| return fmt.Errorf("failed to list keys: %w", err) | ||
| } | ||
|
|
||
| if len(keys) == 0 { | ||
| log.Println("No API keys found.") | ||
| return nil | ||
| } | ||
|
|
||
| w := tabwriter.NewWriter(os.Stdout, 0, 0, 3, ' ', 0) | ||
| fmt.Fprintln(w, "PREFIX\tNAME\tRPM LIMIT\tACTIVE") | ||
| fmt.Fprintln(w, "------\t----\t---------\t------") | ||
| for _, k := range keys { | ||
| fmt.Fprintf(w, "%s\t%s\t%d\t%v\n", k.Prefix, k.Name, k.RpmLimit, k.IsActive) | ||
| } | ||
| w.Flush() | ||
|
|
||
| return nil | ||
| }, | ||
| } |
There was a problem hiding this comment.
Database connection leak in list command.
Same pattern — add defer db.Close() after obtaining the connection.
🔧 Proposed fix
db, err := connectKeyDb(cmd)
if err != nil {
return err
}
+ defer db.Close()📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| var keyListCmd = &cobra.Command{ | |
| Use: "list", | |
| Short: "List all API keys", | |
| RunE: func(cmd *cobra.Command, args []string) error { | |
| db, err := connectKeyDb(cmd) | |
| if err != nil { | |
| return err | |
| } | |
| ctx, cancel := context.WithTimeout(context.Background(), 15*time.Second) | |
| defer cancel() | |
| keys, err := db.ListApiKeys(ctx) | |
| if err != nil { | |
| return fmt.Errorf("failed to list keys: %w", err) | |
| } | |
| if len(keys) == 0 { | |
| log.Println("No API keys found.") | |
| return nil | |
| } | |
| w := tabwriter.NewWriter(os.Stdout, 0, 0, 3, ' ', 0) | |
| fmt.Fprintln(w, "PREFIX\tNAME\tRPM LIMIT\tACTIVE") | |
| fmt.Fprintln(w, "------\t----\t---------\t------") | |
| for _, k := range keys { | |
| fmt.Fprintf(w, "%s\t%s\t%d\t%v\n", k.Prefix, k.Name, k.RpmLimit, k.IsActive) | |
| } | |
| w.Flush() | |
| return nil | |
| }, | |
| } | |
| var keyListCmd = &cobra.Command{ | |
| Use: "list", | |
| Short: "List all API keys", | |
| RunE: func(cmd *cobra.Command, args []string) error { | |
| db, err := connectKeyDb(cmd) | |
| if err != nil { | |
| return err | |
| } | |
| defer db.Close() | |
| ctx, cancel := context.WithTimeout(context.Background(), 15*time.Second) | |
| defer cancel() | |
| keys, err := db.ListApiKeys(ctx) | |
| if err != nil { | |
| return fmt.Errorf("failed to list keys: %w", err) | |
| } | |
| if len(keys) == 0 { | |
| log.Println("No API keys found.") | |
| return nil | |
| } | |
| w := tabwriter.NewWriter(os.Stdout, 0, 0, 3, ' ', 0) | |
| fmt.Fprintln(w, "PREFIX\tNAME\tRPM LIMIT\tACTIVE") | |
| fmt.Fprintln(w, "------\t----\t---------\t------") | |
| for _, k := range keys { | |
| fmt.Fprintf(w, "%s\t%s\t%d\t%v\n", k.Prefix, k.Name, k.RpmLimit, k.IsActive) | |
| } | |
| w.Flush() | |
| return nil | |
| }, | |
| } |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@api/key_cmd.go` around lines 257 - 289, The list command (keyListCmd) opens a
DB via connectKeyDb but never closes it, causing a connection leak; after
successfully obtaining db (the variable returned from connectKeyDb) add a
deferred close call (defer db.Close()) immediately before creating the context
so the database is always closed when RunE returns, ensuring resources used by
db.ListApiKeys are released; place the defer right after the err check that
follows connectKeyDb to cover all return paths in RunE.
Comment on lines
108
to
+110
| if err != nil { | ||
| return nil | ||
| } |
There was a problem hiding this comment.
Silent error swallowing loses diagnostic information and may skip valid inserts.
When FindExistingAccounts fails, returning nil causes insertWithRetry to skip all addresses. At minimum, log the error for debugging. Consider returning the original addresses slice so insertion is still attempted.
🐛 Proposed fix to log the error and preserve addresses for insertion
existing, err := a.db.FindExistingAccounts(ctx, addresses, chainName, insertValidators)
if err != nil {
- return nil
+ l.Error().Caller().Stack().Err(err).Msg("error querying existing accounts")
+ return addresses // still attempt to insert all
}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| if err != nil { | |
| return nil | |
| } | |
| existing, err := a.db.FindExistingAccounts(ctx, addresses, chainName, insertValidators) | |
| if err != nil { | |
| l.Error().Caller().Stack().Err(err).Msg("error querying existing accounts") | |
| return addresses // still attempt to insert all | |
| } |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@indexer/address_cache/address.go` around lines 108 - 110, The call to
FindExistingAccounts in address.go currently swallows errors by returning nil
which prevents insertWithRetry from attempting any inserts; update the error
path in the block that checks "if err != nil" to log the error (use the existing
logger/context) and return the original addresses slice (or otherwise propagate
the error) so insertWithRetry still processes addresses; reference the
FindExistingAccounts call and the insertWithRetry flow to ensure you preserve
diagnostics and allow insertion attempts instead of silently skipping them.
Comment on lines
+176
to
+211
| // processBlock is a helper method to process a block and store it at a pre-allocated slice. | ||
| func (d *DataProcessor) processBlock( | ||
| idx int, | ||
| block *rpcClient.BlockResponse, | ||
| wg *sync.WaitGroup, | ||
| blocksData []sqlDataTypes.Blocks, | ||
| ) { | ||
| defer wg.Done() | ||
| hash, err := base64.StdEncoding.DecodeString(block.Result.BlockMeta.BlockID.Hash) | ||
| if err != nil { | ||
| l.Error(). | ||
| Caller(). | ||
| Stack(). | ||
| Msgf( | ||
| "Failed to decode block hash %s: %v", block.Result.BlockMeta.BlockID.Hash, err, | ||
| ) | ||
| return | ||
| } | ||
| height, err := strconv.ParseUint(block.Result.Block.Header.Height, 10, 64) | ||
| if err != nil { | ||
| l.Error(). | ||
| Caller(). | ||
| Stack(). | ||
| Msgf( | ||
| "Failed to parse block height %s: %v", block.Result.Block.Header.Height, err, | ||
| ) | ||
| return | ||
| } | ||
| blocksData[idx] = sqlDataTypes.Blocks{ | ||
| Hash: hash, | ||
| Height: height, | ||
| Timestamp: block.Result.Block.Header.Time, | ||
| ChainID: block.Result.Block.Header.ChainID, | ||
| ChainName: d.chainName, | ||
| } | ||
| } |
There was a problem hiding this comment.
Failed block processing leaves zero-value entries in the result slice.
When processBlock encounters an error (hash decode or height parse failure), it returns early without marking the slot as invalid. Unlike ProcessTransactions which uses a valid[] slice to filter out failed entries, ProcessBlocks will include zero-initialized sqlDataTypes.Blocks entries in the database insert.
This could insert blocks with zero Height, empty Hash, and zero-value Timestamp into the database.
🛠️ Suggested fix: Add a valid slice pattern consistent with ProcessTransactions
func (d *DataProcessor) ProcessBlocks(blocks []*rpcClient.BlockResponse, fromHeight uint64, toHeight uint64) {
blockAmount := len(blocks)
blocksData := make([]sqlDataTypes.Blocks, blockAmount)
+ valid := make([]bool, blockAmount)
wg := sync.WaitGroup{}
wg.Add(blockAmount)
for idx, block := range blocks {
- go d.processBlock(idx, block, &wg, blocksData)
+ go d.processBlock(idx, block, &wg, &valid[idx], blocksData)
}
wg.Wait()
+ // Collect only the entries that were successfully processed
+ result := make([]sqlDataTypes.Blocks, 0, blockAmount)
+ for idx, ok := range valid {
+ if ok {
+ result = append(result, blocksData[idx])
+ }
+ }
+
timeout := 10*time.Second + (time.Duration(blockAmount) * time.Second / 5)
// ... rest of function uses result instead of blocksDataAnd update processBlock to set *valid = true at the end on success.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@indexer/data_processor/operator.go` around lines 176 - 211, ProcessBlocks is
currently vulnerable because processBlock returns on error leaving zero-value
entries in blocksData; modify processBlock to accept a parallel valid []bool (or
*bool pointer per index) and set valid[idx]=true only on successful parsing
(after decoding hash and parsing height), and ensure the caller ProcessBlocks
allocates and passes that valid slice and filters blocksData to only include
entries where valid is true before DB insert (mirror the valid[] pattern used in
ProcessTransactions); update function signatures: processBlock(..., blocksData
[]sqlDataTypes.Blocks, valid []bool) and set valid[idx]=true at the end on
success.
Comment on lines
+83
to
+90
| query2 := ` | ||
| SELECT | ||
| encode(tx_hash, 'base64'), | ||
| block_height | ||
| FROM transaction_general | ||
| WHERE chain_name = $1 | ||
| AND block_height = (SELECT MAX(height) FROM blocks WHERE chain_name = $1) | ||
| ` |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Verify concurrent latest-block + independent MAX(height) query pattern
rg -n -C3 "GetLatestBlock|errgroup\\.WithContext|SELECT MAX\\(height\\) FROM blocks WHERE chain_name = \\$1|fetchTransactionData\\(ctx, query2, chainName\\)" pkgs/database/query_block.goRepository: Cogwheel-Validator/spectra-gnoland-indexer
Length of output: 924
🏁 Script executed:
cat -n pkgs/database/query_block.go | head -130Repository: Cogwheel-Validator/spectra-gnoland-indexer
Length of output: 3819
🏁 Script executed:
sed -n '122,240p' pkgs/database/query_block.goRepository: Cogwheel-Validator/spectra-gnoland-indexer
Length of output: 2926
🏁 Script executed:
# Check exact line numbers for query2 in GetLastXBlocks
sed -n '155,175p' pkgs/database/query_block.go | cat -nRepository: Cogwheel-Validator/spectra-gnoland-indexer
Length of output: 619
🏁 Script executed:
# Get complete line numbers for GetLastXBlocks query2 and concurrent execution
sed -n '138,190p' pkgs/database/query_block.go | cat -nRepository: Cogwheel-Validator/spectra-gnoland-indexer
Length of output: 1550
GetLatestBlock can return mismatched txs due to concurrent MAX(height) evaluation.
query1 (line 73-82: ORDER BY height DESC LIMIT 1) and query2 (line 89: independent SELECT MAX(height)) run concurrently in separate goroutines. If a new block is inserted between their executions, they observe different latest heights. This causes blocks[0].Height to not match the height in the txs map, resulting in missing or misattached transactions.
The same pattern affects GetLastXBlocks (lines 155-156), where two separate MAX() subqueries in the WHERE clause can diverge, and GetLastXBlocks is additionally vulnerable to returning x+1 transaction rows due to MAX() shifting during concurrent execution.
Fix by computing the block height once before fetching transactions:
Suggested fix for GetLatestBlock
Replace the concurrent execution with sequential fetching: retrieve the block height from query1, then use that height as a parameter to query2 instead of a dynamic MAX() subquery.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@pkgs/database/query_block.go` around lines 83 - 90, GetLatestBlock (and
similarly GetLastXBlocks) currently runs query1 (ORDER BY height DESC LIMIT 1)
and query2 (which uses SELECT MAX(height) in its WHERE) concurrently which can
return mismatched heights if a new block is inserted mid-run; change the flow to
first fetch the block height from query1 (use the returned blocks[0].Height) and
then run the transaction query (query2) using that concrete height as a
parameter instead of the MAX() subquery so both queries see the same height;
apply the same pattern in GetLastXBlocks by computing the exact height(s) up
front and passing them as parameters to the tx query rather than embedding MAX()
twice so you don’t get off-by-one or extra tx rows.
Comment on lines
+152
to
+154
| AND block_height <= (SELECT MAX(height) FROM blocks WHERE chain_name = $1) | ||
| AND block_height >= (SELECT MAX(height) FROM blocks WHERE chain_name = $1) - $2 | ||
| ORDER BY block_height DESC |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Verify lower-bound expression in GetLastXBlocks tx range query
rg -n -C2 "GetLastXBlocks|block_height >= \\(SELECT MAX\\(height\\) FROM blocks WHERE chain_name = \\$1\\) - \\$2" pkgs/database/query_block.goRepository: Cogwheel-Validator/spectra-gnoland-indexer
Length of output: 316
🏁 Script executed:
cat -n pkgs/database/query_block.go | sed -n '145,160p'Repository: Cogwheel-Validator/spectra-gnoland-indexer
Length of output: 596
🏁 Script executed:
rg -n "GetLastXBlocks" pkgs/database/query_block.go -A 10Repository: Cogwheel-Validator/spectra-gnoland-indexer
Length of output: 721
🏁 Script executed:
rg -n "block_height >=" pkgs/database/query_block.go -B 2 -A 2Repository: Cogwheel-Validator/spectra-gnoland-indexer
Length of output: 464
GetLastXBlocks transaction query pulls one extra height window.
The query at line 153 with block_height >= MAX(height) - $2 and block_height <= MAX(height) creates a range of size x+1, not x. For example, with MAX=100 and x=3, the current condition produces >= 97 AND <= 100, yielding 4 blocks instead of 3. This mismatches query1 (line 136–144), which correctly fetches exactly x blocks using LIMIT $2.
Suggested fix
- AND block_height >= (SELECT MAX(height) FROM blocks WHERE chain_name = $1) - $2
+ AND block_height >= (SELECT MAX(height) FROM blocks WHERE chain_name = $1) - ($2 - 1)📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| AND block_height <= (SELECT MAX(height) FROM blocks WHERE chain_name = $1) | |
| AND block_height >= (SELECT MAX(height) FROM blocks WHERE chain_name = $1) - $2 | |
| ORDER BY block_height DESC | |
| AND block_height <= (SELECT MAX(height) FROM blocks WHERE chain_name = $1) | |
| AND block_height >= (SELECT MAX(height) FROM blocks WHERE chain_name = $1) - ($2 - 1) | |
| ORDER BY block_height DESC |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@pkgs/database/query_block.go` around lines 152 - 154, The SQL in
GetLastXBlocks uses "block_height >= (SELECT MAX(height) ...) - $2" which yields
x+1 rows; change the lower bound to compute MAX - $2 + 1 so the range is
inclusive and returns exactly x rows (i.e. use "block_height >= (SELECT
MAX(height) FROM blocks WHERE chain_name = $1) - $2 + 1"). Update the query in
the GetLastXBlocks code path accordingly so it matches the intended x-sized
window (and remains ordered DESC).
Comment on lines
+53
to
+60
| query := ` | ||
| SELECT | ||
| SUM(block_count) as block_count | ||
| FROM block_counter | ||
| WHERE chain_name = $1 | ||
| AND time_bucket >= NOW() - INTERVAL '24 hours' | ||
| AND time_bucket < NOW() | ||
| ` |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Verify aggregate SUM usage in query_view.go where Scan targets int64
rg -n -C2 "SUM\\((block_count|transaction_count)\\)|Scan\\(&blockCount\\)|Scan\\(&totalTxCount\\)|Scan\\(&txCount24h\\)" pkgs/database/query_view.goRepository: Cogwheel-Validator/spectra-gnoland-indexer
Length of output: 1353
Wrap aggregate SUM() results with COALESCE() to handle empty result sets.
PostgreSQL returns NULL for SUM() on empty result sets. Scanning NULL into int64 variables causes an error instead of returning 0. This affects four methods:
GetBlockCount24h(line 55):SUM(block_count)→COALESCE(SUM(block_count), 0)GetBlockCountByDate(line 21):SUM(block_count)→COALESCE(SUM(block_count), 0)GetTotalTxCount(line 111):SUM(transaction_count)→COALESCE(SUM(transaction_count), 0)GetTotalTxCount24h(line 131):SUM(transaction_count)→COALESCE(SUM(transaction_count), 0)
Note: GetTxCountByDay and GetTxCountByHour already correctly use coalesce() (lines 159, 196).
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@pkgs/database/query_view.go` around lines 53 - 60, Queries that SUM() over
empty result sets can return NULL and break scanning into int64s; update the SQL
in GetBlockCount24h, GetBlockCountByDate, GetTotalTxCount, and
GetTotalTxCount24h to wrap the aggregate with COALESCE so it returns 0 instead
of NULL (e.g. change SUM(block_count) to COALESCE(SUM(block_count), 0) and
SUM(transaction_count) to COALESCE(SUM(transaction_count), 0)) so the scanned
values into the int64 variables succeed.
…roved clarity and validation
…nd enhance build outputs for multiple architectures
Merged
Merged
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary by CodeRabbit
New Features
Chores