v0.5.0#12
Merged
Merged
Conversation
… detailed API documentation. Added support for timestamp range, cursor, limit, and pagination in GetAddressTxs. Updated related tests and database interface to reflect new parameters.
…ommands for code and vulnerability check
…g. It did help at the time, but now it doesn't serve any purpose.
Implemented two new API routes for converting between Base64 and Base64Url formats. Added corresponding handler functions to process the conversions and validate input. Introduced new types for request and response structures in the humatypes package.
…s and add new database method to query by limit and offset
…rst dictionary for compression.
…. Added support for OpenTelemetry logging. In the data processor switched to mux+slice processing instead going over the channel.
…rolog to the stop signal.
… proto structure.
…g the trained dictionary.
…Update historic and live processing commands to support compression flag. Refactor event handling to utilize protobuf for serialized events.
📝 WalkthroughWalkthroughThis PR adds Zstandard event compression with dictionary training, cursor- and page-based pagination for address/transaction queries, structured logging (zerolog + OTLP), Base64 conversion utilities, multi-image Docker CI, new protobuf tooling, extensive docs, and removes many experimental files. Changes
Sequence Diagram(s)sequenceDiagram
participant Indexer
participant TimescaleDB
participant API
participant Trainer
Indexer->>Indexer: Collect events (RPC)
Indexer->>Indexer: serialize to protobuf
Indexer->>Indexer: compress with zstd (dict)
Indexer->>TimescaleDB: store transaction (Tx + compressed events)
API->>TimescaleDB: query transaction(s) (cursor/limit)
TimescaleDB-->>API: return FullTxData (compressed flag + data)
API->>API: if compressed -> decompress using dict -> unmarshal protobuf
Trainer->>TimescaleDB: sample events for training
Trainer->>Trainer: build zstd dictionary
Trainer->>Indexer: provide/update embedded dictionary (via pkgs/dict_loader or rebuild)
Estimated code review effort🎯 4 (Complex) | ⏱️ ~75 minutes Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 1 | ❌ 2❌ Failed checks (2 warnings)
✅ Passed checks (1 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
Review Summary by QodoImplement event compression, structured logging, and API enhancements with documentation
WalkthroughsDescription• **Event Compression**: Implemented zstandard (zstd) compression with protobuf serialization for
transaction events, including dictionary training utilities and embedded dictionary loading
• **Structured Logging Migration**: Replaced standard log package with centralized zerolog-based
structured logging across all indexer modules with OpenTelemetry integration
• **API Improvements**: Standardized endpoint paths to plural forms, implemented cursor-based
pagination for transactions and addresses, added base64 encoding conversion utilities
• **Database Query Refactoring**: Reorganized query functions into separate files (query_block.go,
queries_tx.go, queries_address.go) with support for multiple pagination modes and event
decompression
• **Protocol Buffers**: Moved protobuf definitions to pkgs/events_proto with proper package
structure and added attribute parsing with runtime type detection
• **Docker & Build Optimization**: Updated Dockerfiles to use distroless base images, added API
service configuration, improved binary stripping and security
• **Documentation**: Added comprehensive data model documentation, compression feature guide, and
fixed formatting/spelling issues across all docs
• **Dependencies**: Updated Go to 1.25.7, added zerolog/OpenTelemetry packages, gozstd compression
library, and updated core dependencies
• **Bug Fixes**: Fixed import paths, typos ("untill"→"until", "lenght"→"length"), and improved RPC
response error handling
Diagramflowchart LR
RPC["RPC Node"] -->|raw events| Indexer["Indexer<br/>Structured Logging"]
Indexer -->|serialize| Proto["Protobuf<br/>Serialization"]
Proto -->|compress| Zstd["Zstd Compression<br/>with Dictionary"]
Zstd -->|store| DB["TimescaleDB<br/>Compressed Events"]
DB -->|decompress| API["API Service<br/>Cursor Pagination"]
API -->|convert| Convert["Base64<br/>Conversion"]
Convert -->|response| Client["Client"]
File Changes1. pkgs/events_proto/events.pb.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: 10
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 (2)
indexer/db_init/tables.go (1)
550-560:⚠️ Potential issue | 🟠 MajorUnsafe SQL interpolation in user/privilege statements.
userName,tableName, and password text are embedded directly into SQL strings. This is injection-prone and can break on quoted input. Validate identifiers strictly and parameterize the password value.Suggested hardening patch
import ( "context" "fmt" + "regexp" "reflect" @@ ) + +var sqlIdentifierRe = regexp.MustCompile(`^[a-zA-Z_][a-zA-Z0-9_]*$`) @@ func (db *DBInitializer) CreateUser(userName string) error { + if !sqlIdentifierRe.MatchString(userName) { + return fmt.Errorf("invalid user name: %s", userName) + } @@ - sql := fmt.Sprintf("CREATE USER %s WITH PASSWORD '%s'", userName, password) - _, err = db.pool.Exec(context.Background(), sql) + sql := fmt.Sprintf("CREATE USER %s WITH PASSWORD $1", userName) + _, err = db.pool.Exec(context.Background(), sql, password) @@ func (db *DBInitializer) AppointPrivileges( userName string, privilage string, tableNames []string, ) error { + if !sqlIdentifierRe.MatchString(userName) { + return fmt.Errorf("invalid user name: %s", userName) + } @@ switch privilage { case "reader": for _, tableName := range tableNames { + if !sqlIdentifierRe.MatchString(tableName) { + return fmt.Errorf("invalid table name: %s", tableName) + } fmt.Fprintf(&sql, "GRANT SELECT ON TABLE %s TO %s;\n", tableName, userName) } case "writer": for _, tableName := range tableNames { + if !sqlIdentifierRe.MatchString(tableName) { + return fmt.Errorf("invalid table name: %s", tableName) + } fmt.Fprintf(&sql, "GRANT SELECT, INSERT, UPDATE ON TABLE %s TO %s;\n", tableName, userName) }Also applies to: 589-593
🤖 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 550 - 560, The CreateUser function embeds userName and password directly into the SQL string (sql variable) which is injection-prone; validate identifier inputs (e.g., userName, tableName) against a strict regex (letters, digits, underscore) before using them, and stop using fmt.Sprintf to inject passwords—use parameterized queries (db.Exec with placeholders) for the password and any other user-supplied values; only allow fmt.Sprintf-style assembly for identifiers after validation, and apply the same fix to the other similar blocks around lines referenced (e.g., the subsequent user/privilege statements).indexer/data_processor/operator.go (1)
580-625:⚠️ Potential issue | 🟠 MajorInvalid commits can produce zero-value rows that still get inserted.
With preallocated
validatorData, any worker that returns early leaves a default struct at its index, and those entries are still sent toInsertValidatorBlockSignings.🐛 Proposed fix
- validatorData := make([]sqlDataTypes.ValidatorBlockSigning, commitAmount) + validatorData := make([]sqlDataTypes.ValidatorBlockSigning, 0, commitAmount) @@ - for idx, commit := range commits { - go func(idx int, commit *rpcClient.CommitResponse) { + for _, commit := range commits { + go func(commit *rpcClient.CommitResponse) { @@ - mu.Lock() - validatorData[idx] = sqlDataTypes.ValidatorBlockSigning{ + mu.Lock() + validatorData = append(validatorData, sqlDataTypes.ValidatorBlockSigning{ BlockHeight: height, Timestamp: commit.GetTimestamp(), Proposer: signedVals.Proposer, SignedVals: signedVals.SignedVals, ChainName: d.chainName, - } + }) mu.Unlock() - }(idx, commit) + }(commit) }🤖 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 580 - 625, The preallocated validatorData slice allows goroutines that return early to leave zero-value entries which later get inserted; fix by making validatorData a zero-length slice (e.g., validatorData := make([]sqlDataTypes.ValidatorBlockSigning, 0, commitAmount)) and, inside the goroutine, replace the indexed assignment with a mutex-protected append (mu.Lock(); validatorData = append(validatorData, <constructed ValidatorBlockSigning>); mu.Unlock()); keep wg.Done() defer as-is and ensure the code that calls InsertValidatorBlockSignings uses the populated validatorData slice so no zero-value rows are sent.
🟠 Major comments (17)
indexer/rpc_client/client.go-98-105 (1)
98-105:⚠️ Potential issue | 🟠 MajorBound response-body size to avoid untrusted-memory blowups.
Reading arbitrary RPC response bodies with unbounded
io.ReadAllcan spike memory and cascade failures under bad upstream behavior.Suggested hardening
+const maxRPCBodyBytes = 4 << 20 // 4 MiB -body, err := io.ReadAll(resp.Body) +body, err := io.ReadAll(io.LimitReader(resp.Body, maxRPCBodyBytes+1)) if err != nil { return fmt.Errorf("failed to read response body: %w", err) } +if len(body) > maxRPCBodyBytes { + return fmt.Errorf("response body too large: exceeded %d bytes", maxRPCBodyBytes) +} if resp.StatusCode != http.StatusOK { - return fmt.Errorf("http error %s: %s", resp.Status, string(body)) + preview := string(body) + if len(preview) > 1024 { + preview = preview[:1024] + "..." + } + return fmt.Errorf("http error %s: %s", resp.Status, preview) }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@indexer/rpc_client/client.go` around lines 98 - 105, Replace the unbounded io.ReadAll(resp.Body) with a size-limited read to prevent OOM: define a package-level constant like maxResponseBodySize (e.g. 1<<20 for 1MiB), read via io.ReadAll(io.LimitReader(resp.Body, maxResponseBodySize)), and if the returned body length equals maxResponseBodySize treat it as an error (return a clear "response body too large" error) instead of proceeding; update the code around the existing resp, body, io.ReadAll and http.StatusOK checks to use the limited reader and fail fast on oversized responses.indexer/db_init/hypertable.go-48-53 (1)
48-53:⚠️ Potential issue | 🟠 MajorDon’t continue after hypertable/compression setup failures.
Line 48, Line 83, and Line 112 now only log on DB DDL failures, so initialization can proceed in a broken/partial state. Please propagate errors (or aggregate and return) so startup can fail deterministically.
Suggested direction
-func (init *DBInitializer) ConvertToHypertables(tableNames []string) { +func (init *DBInitializer) ConvertToHypertables(tableNames []string) error { for _, tableName := range tableNames { ... if err != nil { - l.Error()...Msgf("failed ...") + return fmt.Errorf("convert %s to hypertable: %w", tableName, err) } } + return nil }Apply the same pattern to
AlterCompressionSegmentsandAddCompressionPolicy.Also applies to: 83-88, 112-117
🤖 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 48 - 53, The code currently only logs failures when converting a table to a hypertable and when setting up compression, which allows init to continue in a broken state; update the hypertable conversion block (the log call that produces "failed to convert table %s to hypertable: %v") and the calls to AlterCompressionSegments and AddCompressionPolicy so that on any error you propagate or aggregate and return the error (e.g., wrap the original error and return it from the surrounding function) instead of only logging; ensure the calling initializer receives the error so startup can fail deterministically.indexer/indexer.go-30-31 (1)
30-31:⚠️ Potential issue | 🟠 MajorAvoid
Fatal()here; it bypasses deferred OTel shutdown.
Fatal()exits immediately viaos.Exit(1), skipping the deferred provider shutdown at line 30. This causes pending batched telemetry/logs to be lost on command errors. Explicitly shut down the provider before exiting.Suggested fix:
- Store the provider's
Shutdownmethod instead of usingdefer- On command error, log the error, explicitly call shutdown with a timeout, then
os.Exit(1)- Add
"time"to importsSuggested patch
import ( "context" "os" "os/signal" "syscall" + "time" @@ func main() { ctx, cancel := signal.NotifyContext(context.Background(), os.Interrupt, syscall.SIGTERM) defer cancel() + var shutdownProvider func(context.Context) error @@ if os.Getenv("OTEL_EXPORTER_OTLP_ENDPOINT") != "" { if exp, err := otlploghttp.New(ctx); err == nil { provider := sdklog.NewLoggerProvider( sdklog.WithProcessor(sdklog.NewBatchProcessor(exp)), ) global.SetLoggerProvider(provider) - defer provider.Shutdown(ctx) //nolint:errcheck + shutdownProvider = provider.Shutdown } } @@ if err := cmd.RootCmd.ExecuteContext(ctx); err != nil { - logger.Get().Fatal().Err(err).Msg("failed to execute command") + logger.Get().Error().Err(err).Msg("failed to execute command") + if shutdownProvider != nil { + shutdownCtx, shutdownCancel := context.WithTimeout(context.Background(), 5*time.Second) + if shutdownErr := shutdownProvider(shutdownCtx); shutdownErr != nil { + logger.Get().Error().Err(shutdownErr).Msg("failed to shutdown OTel logger provider") + } + shutdownCancel() + } + os.Exit(1) } }Also applies to: 40-42
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@indexer/indexer.go` around lines 30 - 31, The code currently defers provider.Shutdown(ctx) (provider.Shutdown) but later calls log.Fatal/cli.Fatal which calls os.Exit and skips deferred shutdown; change to capture the shutdown function (e.g., shutdown := provider.Shutdown), remove the defer, and on command error paths (the places using Fatal in this file—refer to the error handling after provider init and the similar block at lines referenced around 40-42) call shutdown with a cancellable context that has a timeout (use time.WithTimeout), wait for it to complete, log the original error, then call os.Exit(1); also add "time" to the imports.indexer/main_operator/operator.go-103-107 (1)
103-107:⚠️ Potential issue | 🟠 MajorHistoric shutdown path currently only logs and does not stop processing.
When cancellation arrives, the goroutine logs it, but
HistoricProcesskeeps running without a cancellation channel/context path.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@indexer/main_operator/operator.go` around lines 103 - 107, Historic processing isn't cancelled because HistoricProcess is called without a context; capture the signalHandler.Context() and propagate cancellation into HistoricProcess by changing its signature to accept a context.Context (or add a context param) and using signalHandler.Context() (or its .Done()) inside the goroutine call site: replace orch.HistoricProcess(...) with orch.HistoricProcess(signalHandler.Context(), runningFlags.FromHeight, runningFlags.ToHeight, runningFlags.CompressEvents) and implement context-aware cancellation inside HistoricProcess (check ctx.Done() and return promptly), so the goroutine logging the shutdown will actually stop processing.compression/train/process.go-44-67 (1)
44-67:⚠️ Potential issue | 🟠 MajorCollection loop can over-fetch, overwhelm DB, and hide partial failures.
The current fan-out may launch thousands of concurrent DB calls, the last chunk can request beyond
amount, and failed chunk fetches are only logged (function still returns success with partial data).✅ Proposed hardening
- goroutines = int(math.Ceil(float64(amount) / 100)) + const chunkSize uint64 = 100 + goroutines = int(math.Ceil(float64(amount) / float64(chunkSize))) transactions := make([]*database.Transaction, 0) wg := sync.WaitGroup{} wg.Add(goroutines) mu := sync.Mutex{} + errCh := make(chan error, goroutines) for i := 0; i < goroutines; i++ { go func(i int) { defer wg.Done() - offset := uint64(i) * limit + offset := uint64(i) * chunkSize + if offset >= amount { + return + } + batchLimit := min(chunkSize, amount-offset) log.Printf("getting the transactions from %s with limit %d and offset %d", chainName, limit, offset) ctx, cancel := context.WithTimeout(context.Background(), 10*time.Second) defer cancel() - txs, err := db.GetTransactionsByOffset(ctx, chainName, limit, offset) + txs, err := db.GetTransactionsByOffset(ctx, chainName, batchLimit, offset) if err != nil { - log.Printf("failed to get transactions from %s with limit %d and offset %d: %v", chainName, limit, offset, err) + errCh <- fmt.Errorf("failed to get transactions for offset %d: %w", offset, err) return } @@ }(i) } wg.Wait() + close(errCh) + for err := range errCh { + if err != nil { + return nil, err + } + }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@compression/train/process.go` around lines 44 - 67, The fan-out loop launching goroutines can spawn too many concurrent DB calls, request beyond the total amount, and silently drop failures; fix by bounding concurrency (introduce a semaphore/worker pool with a configurable maxWorkers and use it around each goroutine spawn), compute each chunk size so the last request does not exceed amount (calculate remaining = min(limit, amount - offset) and pass that to db.GetTransactionsByOffset instead of always using limit), and collect/propagate errors from db.GetTransactionsByOffset (e.g., send errors on a channel or capture the first error and return it) rather than only logging so the caller can detect partial failures; update references in this block (the goroutine closure that calls db.GetTransactionsByOffset, the transactions slice append protected by mu, and the wg synchronization) accordingly.docker-compose-dev.yml-11-11 (1)
11-11:⚠️ Potential issue | 🟠 MajorAvoid committed plaintext credentials in compose config.
Use environment-variable substitution (or
.env) instead of hardcoded passwords.Suggested fix
- POSTGRES_PASSWORD: 12345678 + POSTGRES_PASSWORD: ${POSTGRES_PASSWORD:?set POSTGRES_PASSWORD} @@ - DB_PASSWORD: 12345678 + DB_PASSWORD: ${POSTGRES_PASSWORD:?set POSTGRES_PASSWORD} @@ - DB_PASSWORD: 12345678 + DB_PASSWORD: ${POSTGRES_PASSWORD:?set POSTGRES_PASSWORD}Also applies to: 38-39, 64-64
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@docker-compose-dev.yml` at line 11, Replace hardcoded plaintext passwords in the docker-compose-dev.yml (e.g., the POSTGRES_PASSWORD entries currently set to "12345678" and the similar values noted at lines 38-39 and 64) with environment-variable substitution such as referencing ${POSTGRES_PASSWORD} (or a default like ${POSTGRES_PASSWORD:-}) and document the required variable in a .env.example file; ensure you remove the literal password from the compose file and update README to instruct developers to provide POSTGRES_PASSWORD via .env or their environment instead.api/handlers/address.go-24-41 (1)
24-41:⚠️ Potential issue | 🟠 MajorValidate incompatible query mode combinations before DB call.
Right now a request can send cursor + range + paging together. That creates ambiguous behavior and inconsistent pagination semantics. Reject mixed modes and allow only one strategy per request.
Suggested fix
var fromTs, toTs *time.Time @@ var cursor *string if input.Cursor != "" { cursor = &input.Cursor } + + modeCount := 0 + if fromTs != nil || toTs != nil { + modeCount++ + } + if cursor != nil { + modeCount++ + } + if limit != nil || page != nil { + modeCount++ + } + if modeCount > 1 { + return nil, huma.Error400BadRequest("invalid query parameters", nil) + } + addressTxs, nextCursor, txCount, err := h.db.GetAddressTxs(🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@api/handlers/address.go` around lines 24 - 41, Before constructing fromTs/toTs/limit/page/cursor and calling the DB, validate that the request uses only one pagination mode: treat cursor mode as input.Cursor != "", range mode as input.FromTimestamp or input.ToTimestamp not zero, and paging mode as input.Limit != 0 or input.Page != 0; if more than one mode is present, return a 400 error (or the handler's standard bad-request response) and do not proceed to the DB. Implement this check using the existing input, fromTs/toTs, limit/page, and cursor symbols so mixed combinations (cursor + range, cursor + paging, range + paging) are rejected up front.Makefile-1-12 (1)
1-12:⚠️ Potential issue | 🟠 MajorTop-level
.PHONYdeclaration is malformed.Lines 1-12 show tab-indented continuation, but tabs in Makefiles introduce recipe lines (shell commands), not target prerequisites. Only
buildfrom line 1 is marked phony; the remaining targets (install,clean,build-experimental,build-api,integration-test,test,vulnerability-scan,snyk,semgrep,code-quality) are not marked phony. Consolidate all targets onto a single line and remove the duplicate.PHONY: train-zstddeclaration at line 78.Suggested fix
-.PHONY: build - install - clean - build-experimental - install-experimental - build-api - integration-test - test - vulnerability-scan - snyk - semgrep - code-quality +.PHONY: build install clean build-experimental install-experimental build-api integration-test test vulnerability-scan snyk semgrep code-quality train-zstd @@ -.PHONY: train-zstd🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@Makefile` around lines 1 - 12, The top-level .PHONY declaration is malformed: only "build" is currently marked phony because the remaining targets are on subsequent tab-indented lines (interpreted as recipe lines). Edit the Makefile to place a single .PHONY: line listing all targets (build install clean build-experimental install-experimental build-api integration-test test vulnerability-scan snyk semgrep code-quality) on one line, and remove the duplicate ".PHONY: train-zstd" declaration found later so each phony target is declared exactly once.docker-compose.yml-35-36 (1)
35-36:⚠️ Potential issue | 🟠 MajorUse health-based dependency gating for DB readiness.
At Lines [35-36] and [54-55],
depends_ononly orders startup.indexer/apimay race before TimescaleDB is ready.🛠️ Proposed fix
- depends_on: - - timescaledb + depends_on: + timescaledb: + condition: service_healthy @@ - depends_on: - - timescaledb + depends_on: + timescaledb: + condition: service_healthyAlso applies to: 54-55
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@docker-compose.yml` around lines 35 - 36, Replace simple startup ordering with health-based gating: add a healthcheck block to the timescaledb service (e.g., a pg_isready or psql-based TEST command plus interval/timeout/retries) and update the depends_on entries for the indexer and api services to depend on timescaledb with condition: service_healthy instead of the current list form; ensure the service name references are the existing timescaledb, indexer, and api service keys so Docker Compose waits for DB readiness before starting indexer/api.docker-compose.yml-27-33 (1)
27-33:⚠️ Potential issue | 🟠 MajorDo not hardcode database credentials in service definitions.
At Lines [31] and [50], plaintext passwords are embedded in compose config. Move these to external environment variables/secrets.
🔒 Proposed fix
environment: DB_HOST: timescaledb DB_PORT: 5432 DB_USER: postgres - DB_PASSWORD: 12345678 + DB_PASSWORD: ${DB_PASSWORD:?set DB_PASSWORD} DB_NAME: gnoland @@ environment: DB_HOST: timescaledb DB_PORT: 5432 DB_USER: postgres - DB_PASSWORD: 12345678 + DB_PASSWORD: ${DB_PASSWORD:?set DB_PASSWORD} DB_NAME: gnolandAlso applies to: 46-51
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@docker-compose.yml` around lines 27 - 33, The docker-compose service environment block currently hardcodes DB credentials (DB_PASSWORD, DB_USER, DB_NAME, DB_HOST, DB_PORT); remove plaintext values and load them from external sources instead by using an env_file or Docker secrets for DB_PASSWORD (and other sensitive vars) and reference those variables in the service's environment section; update any other service blocks with the same environment keys to use the same env_file/secrets approach and ensure the compose file documents the required external .env or secret names.api/handlers/transactions.go-150-152 (1)
150-152:⚠️ Potential issue | 🟠 MajorDo not map all cursor query failures to 404.
At Line [151], every DB error is returned as
404 Not Found. This hides operational failures (timeouts, connection issues) as missing data and makes client behavior misleading.🐛 Proposed fix
if err != nil { - return nil, huma.Error404NotFound("Transactions by cursor not found", err) + return nil, huma.Error500InternalServerError("Failed to fetch transactions by cursor", err) }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@api/handlers/transactions.go` around lines 150 - 152, The current code maps every DB error to huma.Error404NotFound; change it to return 404 only when the DB indicates "not found" (e.g., errors.Is(err, sql.ErrNoRows) or errors.Is(err, gorm.ErrRecordNotFound)), otherwise return a 500/appropriate operational error (e.g., huma.Error500InternalServerError) and log the actual error. Update the block around the transactions-by-cursor lookup (where err is checked and huma.Error404NotFound is used) to switch on errors.Is(err, sql.ErrNoRows) / gorm.ErrRecordNotFound => return 404, else log the error and return a 500 error.api/huma-types/convert.go-4-4 (1)
4-4:⚠️ Potential issue | 🟠 MajorInput validation rejects valid 43-character unpadded base64 hashes.
The minLength/maxLength constraints at lines 4 and 8 enforce exactly 44 characters, but valid 32-byte payloads encode to 43 characters without padding. Since Huma validates these constraints before handler execution, requests with unpadded hashes are rejected before reaching
base64.StdEncoding.DecodeString()orbase64.URLEncoding.DecodeString(), which would otherwise accept both lengths.✅ Proposed fix
type ConvertFromBase64toBase64UrlInput struct { - TxHash64 string `query:"tx_hash_64" doc:"Input to convert" required:"true" minLength:"44" maxLength:"44"` + TxHash64 string `query:"tx_hash_64" doc:"Input to convert" required:"true" minLength:"43" maxLength:"44"` } @@ type ConvertFromBase64UrlToBase64Input struct { - TxHash64Url string `query:"tx_hash_64_url" doc:"Input to convert" required:"true" minLength:"44" maxLength:"44"` + TxHash64Url string `query:"tx_hash_64_url" doc:"Input to convert" required:"true" minLength:"43" maxLength:"44"` }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@api/huma-types/convert.go` at line 4, The struct tag for TxHash64 is too strict (requires 44 chars) and rejects valid 43-char unpadded base64; update the Huma validation for the TxHash64 field to allow both 43 and 44 characters (e.g., minLength:"43" maxLength:"44" or remove length constraints and validate manually) so the request reaches the handler, and ensure the handler decoding (base64.StdEncoding.DecodeString / base64.URLEncoding.DecodeString) is used to accept both padded and unpadded inputs.pkgs/database/queries_address.go-186-190 (1)
186-190:⚠️ Potential issue | 🟠 MajorAvoid lossy cursor timestamps and silent error cursors.
makeCursorParamrounds timestamps to seconds and returns"error decoding tx hash"as a cursor string on failure. This can break keyset pagination correctness and hide failures.Suggested fix
-func makeCursorParam( +func makeCursorParam( timestamp time.Time, txHash string, -) string { +) (string, error) { txHashBytes, err := base64.StdEncoding.DecodeString(txHash) if err != nil { - // TODO: log error - return "error decoding tx hash" + return "", fmt.Errorf("error decoding tx hash: %w", err) } - timestamp = timestamp.Round(time.Second) base64Url := base64.URLEncoding.Strict().EncodeToString(txHashBytes) - return timestamp.Format(time.RFC3339) + "|" + base64Url + return timestamp.UTC().Format(time.RFC3339Nano) + "|" + base64Url, nil }- nextCursor := makeCursorParam(lastAddressTx.Timestamp, lastAddressTx.Hash) - return &page, nextCursor, nil + nextCursor, err := makeCursorParam(lastAddressTx.Timestamp, lastAddressTx.Hash) + if err != nil { + return nil, "", err + } + return &page, nextCursor, nilAlso applies to: 252-259
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@pkgs/database/queries_address.go` around lines 186 - 190, The code builds a next-cursor using makeCursorParam(lastAddressTx.Timestamp, lastAddressTx.Hash) but makeCursorParam currently loses sub-second precision (rounds to seconds) and returns a literal error string ("error decoding tx hash") on failure, which corrupts keyset pagination and hides decode errors; change makeCursorParam to preserve full timestamp precision (e.g., RFC3339Nano or unix nanoseconds) and to return (string, error) instead of embedding error text, then update callers in queries_address.go (where page, lastAddressTx, Timestamp, Hash are used) to handle the error: call the new makeCursorParam, check the error, and return the error upstream instead of returning a malformed cursor; ensure similar fixes are applied to the other occurrence (lines ~252-259).pkgs/database/queries_tx.go-291-296 (1)
291-296:⚠️ Potential issue | 🟠 MajorGuard optional protobuf field dereference.
Line 295 dereferences
event.PkgPathwithout nil check. Sinceevent.PkgPathis an optional protobuf field of type*string, it can be nil and will panic at runtime if dereferenced without a guard.Suggested fix
+ pkgPath := "" + if event.PkgPath != nil { + pkgPath = *event.PkgPath + } events = append(events, Event{ AtType: event.AtType, Type: event.Type, Attributes: attributes, - PkgPath: *event.PkgPath, + PkgPath: pkgPath, })🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@pkgs/database/queries_tx.go` around lines 291 - 296, The code appends an Event using event.PkgPath but dereferences the optional protobuf field without a nil check; update the append to guard event.PkgPath (e.g., compute pkgPath := "" and if event.PkgPath != nil set pkgPath = *event.PkgPath) and use that variable for the Event.PkgPath field so you never dereference a nil pointer (refer to the Event struct, the events slice append, and the event.PkgPath symbol to locate the change).pkgs/database/queries_tx.go-14-17 (1)
14-17:⚠️ Potential issue | 🟠 MajorHandle zstd decoder initialization errors.
Ignoring the error from
zstd.NewReaderon line 16 can leave the global decoder in an invalid state. This causes a silent failure during package initialization that will later panic or fail whendecompressEvents(line 254) callszstdReader.DecodeAll().Move the initialization to an
init()function and handle the error:Suggested fix
+import "fmt" @@ -var zstdReader, _ = zstd.NewReader(nil, zstdDict) +var zstdReader *zstd.Decoder + +func init() { + var err error + zstdReader, err = zstd.NewReader(nil, zstdDict) + if err != nil { + panic(fmt.Sprintf("failed to initialize zstd decoder: %v", err)) + } +}🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@pkgs/database/queries_tx.go` around lines 14 - 17, The package-level zstd reader is created ignoring errors (dictBytes, zstdDict, zstdReader) which can leave zstdReader nil and later crash decompressEvents when calling zstdReader.DecodeAll(); move the zstd.NewReader(nil, zstdDict) call into an init() function, check and handle its returned (reader, err), log/return/report the error or panic early if initialization fails, and ensure zstdReader is only set when err == nil so decompressEvents sees a valid reader; update any code that assumes zstdReader is non-nil accordingly.api/huma-types/address.go-11-12 (1)
11-12:⚠️ Potential issue | 🟠 MajorUse pointer timestamps for truly optional query params.
FromTimestampandToTimestampare optional query parameters but currently use non-pointertime.Time. This forces the handler to convert them to pointers using.IsZero()checks, creating a semantic ambiguity: zero-valued timestamps cannot be distinguished from "not provided." The downstream database layer correctly expects pointers and uses nil checks for optionality. Use pointer receivers from the start to align with Go idioms and eliminate the conversion overhead.Suggested fix
- FromTimestamp time.Time `query:"from_timestamp" doc:"From timestamp (inclusive)" format:"date-time"` - ToTimestamp time.Time `query:"to_timestamp" doc:"To timestamp (inclusive)" format:"date-time"` + FromTimestamp *time.Time `query:"from_timestamp" doc:"From timestamp (inclusive)" format:"date-time"` + ToTimestamp *time.Time `query:"to_timestamp" doc:"To timestamp (inclusive)" format:"date-time"`🤖 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 11 - 12, Change the optional query fields FromTimestamp and ToTimestamp in the relevant struct (currently declared as time.Time) to pointers (*time.Time) so absence can be represented as nil; update the struct tags (keep `query:"from_timestamp" doc:"..." format:"date-time"`) but change the field types to *time.Time, and then update any code that reads these fields (handlers or methods that currently use .IsZero() checks) to use nil checks instead and pass the pointers directly to the downstream DB layer that expects *time.Time (refer to the struct name containing FromTimestamp/ToTimestamp and any handler function that converts them).pkgs/database/queries_address.go-145-156 (1)
145-156:⚠️ Potential issue | 🟠 MajorAdd tx.tx_hash to ORDER BY for first cursor page to ensure deterministic ordering.
The first cursor page (line 153) orders only by timestamp, while cursor continuation (line 175) uses
(timestamp, tx_hash)tuple comparison for keyset pagination. Without matching the ordering columns, ties on timestamp can cause row duplicates or skips across pages.Suggested fix
- ORDER BY tx.timestamp DESC + ORDER BY tx.timestamp DESC, tx.tx_hash DESC🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@pkgs/database/queries_address.go` around lines 145 - 156, The initial query that builds the first cursor page orders only by tx.timestamp, which mismatches the continuation keyset pagination that compares (timestamp, tx_hash); update the SELECT/ORDER BY used when constructing the first page (the query variable that currently orders "ORDER BY tx.timestamp DESC") to include tx.tx_hash (e.g., ORDER BY tx.timestamp DESC, tx.tx_hash DESC) so it deterministically orders ties and matches the continuation logic that uses tx_hash; ensure args (accountId, chainName, fetchLimit) and the encode(tx.tx_hash, 'base64') projection remain unchanged.
🟡 Minor comments (19)
docs/scaling.md-1-6 (1)
1-6:⚠️ Potential issue | 🟡 MinorFix title typo and intro grammar for readability.
There are a few user-facing wording issues here (
Scailing,self hosted,Regardless the...,Timescaledb) that make the section feel unpolished.✍️ Proposed edit
-# Scailing the indexer +# Scaling the indexer -If you use the Tiger Data (TimescaleDB cloud edition) they have some internal methods of scaling the database that -are not present for the self hosted version. -Regardless the Timescaledb is still a Postgres database and there are some ways to scale the indexer. +If you use Tiger Data (TimescaleDB Cloud), it includes internal database-scaling methods that +are not present in the self-hosted version. +Regardless, TimescaleDB is still a Postgres database, and there are several ways to scale the indexer.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@docs/scaling.md` around lines 1 - 6, Fix the title typo and improve wording in the heading and first paragraph: rename the heading "Scailing the indexer" to "Scaling the indexer", correct "self hosted" to "self-hosted", capitalize and spell "Timescaledb" as "TimescaleDB", and rephrase the sentence starting with "Regardless the Timescaledb is still a Postgres database..." to something like "Regardless, TimescaleDB is still a PostgreSQL database, and there are ways to scale the indexer." Update the instance referencing "Tiger Data (TimescaleDB cloud edition)" for consistent capitalization and punctuation so the intro reads clearly and professionally.docs/scaling.md-29-32 (1)
29-32:⚠️ Potential issue | 🟡 MinorFix subject-verb agreement and improve address-cache explanation.
The current sentences have grammar mismatches that make the behavior harder to understand.
✍️ Proposed edit
-The thing that you would need to pay most attention is to the address cache. The indexer has in memory cache that -ties the address to the integer value and are mapped everywhere where some sort of address is stored. -So for this to work you might need to copy all of the addresses from one database to another. I guess you could -also skip this part but then this would require some sophisticated way to query from all of the shards. +One area that needs special attention is the address cache. The indexer keeps an in-memory cache that +maps each address to an integer value used wherever an address is stored. +For sharding to work cleanly, you may need to copy addresses between databases. You could skip this step, +but then you would need a more sophisticated way to query across all shards.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@docs/scaling.md` around lines 29 - 32, Fix subject-verb agreement and clarify the address-cache behavior in scaling.md: rewrite the paragraph so verbs agree with their subjects and state explicitly that the indexer maintains an in-memory address cache that maps addresses to integer IDs (reference the terms "address cache" and "indexer"), explain that those integer IDs are used wherever addresses are stored, and then describe the migration options clearly — either copy all address mappings from one database to another or implement a cross-shard query mechanism — so readers understand the consequences and requirements.docs/scaling.md-15-17 (1)
15-17:⚠️ Potential issue | 🟡 MinorTighten the read-replica paragraph wording.
Current phrasing is grammatically awkward and repetitive, which reduces clarity.
✍️ Proposed edit
-The Postgres has a feature of read replicas. This can be used to scale the indexer if there are a lot of read -operations. The read replicas are a copy of the database that is updated asynchronously. You would need to set up -all of the read replicas to gather the data from the master node. +Postgres supports read replicas. This can help scale the indexer when read traffic is high. +Read replicas are asynchronously updated copies of the primary database. You would need to configure +all read replicas to replicate from the primary node.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@docs/scaling.md` around lines 15 - 17, Tighten the paragraph that begins "The Postgres has a feature of read replicas." by replacing the awkward, repetitive wording with a single clear sentence such as: "Postgres supports read replicas—asynchronously updated copies of the primary database—that can be used to scale read-heavy indexers; configuring them is outside the scope of this project." Update the paragraph in docs/scaling.md where that sentence appears so it reads smoothly and concisely.docs/benchmarks.md-24-29 (1)
24-29:⚠️ Potential issue | 🟡 MinorFix grammar error.
"with this settings" should be "with these settings" for correct subject-verb agreement.
📝 Proposed fix
-If you are using your own RPC node, only dedicated for the indexer by default settings it should be able to handle -up to 900 requests concurrently. If you run the node with this settings you could in theory push the indexer to use +If you are using your own RPC node, only dedicated for the indexer by default settings it should be able to handle +up to 900 requests concurrently. If you run the node with these settings you could in theory push the indexer to use block chunk to size of 450 and transaction chunk to size of 900. However only do this if you are the only one using🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@docs/benchmarks.md` around lines 24 - 29, In the benchmarks document update the sentence that currently reads "If you run the node with this settings you could in theory push..." to use correct agreement by replacing "with this settings" with "with these settings"; locate the phrase "with this settings" in the paragraph mentioning RPC node concurrency and change it to "with these settings" (ensure the surrounding sentence remains grammatical).docs/benchmarks.md-20-22 (1)
20-22:⚠️ Potential issue | 🟡 MinorFix grammar error.
The phrase "the not causing" is grammatically incorrect and should be revised for clarity.
📝 Proposed fix
-For normal processing of the data, the recommended block chunk should be anywhere between 50-150. The transaction -chunk should be anywhere between 50-300. This is a good balance between the performance and the not causing the RPC -node to be overloaded. +For normal processing of the data, the recommended block chunk should be anywhere between 50-150. The transaction +chunk should be anywhere between 50-300. This is a good balance between performance and not causing the RPC +node to be overloaded.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@docs/benchmarks.md` around lines 20 - 22, Fix the grammar in the sentence that reads "This is a good balance between the performance and the not causing the RPC node to be overloaded." by rephrasing it to be clear and grammatical—e.g., change it to "This is a good balance between performance and avoiding overloading the RPC node." Update the sentence in the benchmarks text where the block/transaction chunk recommendations are described.integration/synthetic/query_operator.go-7-7 (1)
7-7:⚠️ Potential issue | 🟡 MinorFix RNG seeding for deterministic synthetic data generation.
Line 152 uses unseeded global
rand.Float32(), but the file claims to generate "consistent testing" data. The rest of the codebase properly seeds RNG instances (e.g.,pkgs/generator/crypto.go,pkgs/generator/general.gouserand.New(rand.NewSource(seed))). Replace the global RNG call with a seeded instance stored inSyntheticQueryOperatorto ensure reproducible synthetic data, aligning with the pattern used elsewhere in the generator package.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@integration/synthetic/query_operator.go` at line 7, The code uses the global unseeded RNG via rand.Float32() (around the use in SyntheticQueryOperator) which breaks reproducibility; modify the SyntheticQueryOperator struct to hold a seeded *rand.Rand (constructed with rand.New(rand.NewSource(seed))) and replace the global rand.Float32() call with op.rng.Float32() (or similar) so synthetic data is deterministic and consistent with other generators like pkgs/generator/crypto.go and pkgs/generator/general.go.README.md-169-170 (1)
169-170:⚠️ Potential issue | 🟡 MinorFix DB terminology typos for consistency.
Use “an easier experience” and “PostgreSQL” (not “PostgresSQL”).
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@README.md` around lines 169 - 170, Update the two README bullet points to fix terminology: change "can provide a easier experience" to "can provide an easier experience" and replace "PostgresSQL" with the correct "PostgreSQL" (the lines containing the phrases "an easier experience" and "PostgresSQL" in the two bullets about SQL database and TimescaleDB).README.md-7-19 (1)
7-19:⚠️ Potential issue | 🟡 MinorDocumentation wording in this section needs a grammar pass.
There are several agreement/word-choice issues in this block that make the intro harder to read. Please proofread this section before merge.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@README.md` around lines 7 - 19, The README intro paragraph for "Spectra Gnoland Indexer (SGI)" contains multiple grammar and word-choice issues; perform a copy edit of that block (the paragraph starting with "The Spectra Gnoland Indexer(SGI) is a tool...") to fix punctuation, spelling, subject-verb agreement, run-on sentences, and awkward phrasing, e.g., add space in "Indexer (SGI)", reword sentences about node endpoints/P2P and node roles for clarity, break long sentences into shorter ones, and ensure consistent terms like "TimescaleDB" and "RPC nodes"; keep the original meaning but produce clear, grammatically correct, and concise prose.docs/requirements.md-3-4 (1)
3-4:⚠️ Potential issue | 🟡 MinorPlease fix remaining wording/typo errors in requirements.
This patch includes several clear mistakes (e.g., “could can”, “arround”, “these amount”, “PostgresSQL”) that should be corrected to keep requirements precise.
Also applies to: 23-29, 53-54, 68-69
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@docs/requirements.md` around lines 3 - 4, Fix the typos and wording issues in requirements.md: change "could can" to "can", "arround" to "around", "these amount" to "this amount" (or "these amounts" if plural), and correct "PostgresSQL" to "PostgreSQL" (or "Postgres" consistently); also review and correct the similar mistakes on the other affected ranges (lines referenced in the review: 23-29, 53-54, 68-69) for grammar/typos and ensure consistent capitalization and terminology (e.g., "Tiger Data (TimescaleDB cloud edition)" if applicable); run a quick spellcheck/proofread over the whole file to catch remaining minor errors.docs/api.md-17-39 (1)
17-39:⚠️ Potential issue | 🟡 MinorUpdate the route-count statement to match the new endpoint list.
This section now documents more than 5 endpoints, so the route-count text above is stale and should be updated for consistency.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@docs/api.md` around lines 17 - 39, The route-count summary above the API endpoints is outdated; update the route-count statement to reflect the current number of endpoints listed under the "Blocks", "Transactions", "Addresses", and "Utilities" sections (the list now contains more than five endpoints such as /blocks/{height}, /blocks/latest, /transactions/{tx_hash}, /transactions/{tx_hash}/message, /address/{address}/txs, and the two /convert routes). Edit the route-count text so it correctly states the new total (or uses a non-specific phrasing like "multiple endpoints" if you prefer to avoid frequent updates) and ensure it remains consistent with the endpoints shown in the "Blocks", "Transactions", "Addresses", and "Utilities" sections.docs/setup.md-42-42 (1)
42-42:⚠️ Potential issue | 🟡 MinorFix user-facing wording/typos in updated sections.
There are a few readability issues in the modified text (e.g., “a top of the database”, “These mods”, and awkward phrasing in the live/historic guidance).
✍️ Proposed wording cleanup
-However if you are planning to add a lot of services a top of the database maybe increase it. The docker compose is +However, if you plan to add many services on top of the database, consider increasing it. The docker compose is @@ -These mods can be used differently together. For example you might get access to the archive RPC node. But you +These modes can be used together in different ways. For example, you might get access to an archive RPC node, but you @@ -Maybe you need to view at some segment of the blockchain individually. You can set up different database within the +Maybe you need to inspect a specific segment of the blockchain individually. You can set up a different database within the @@ -If you started the indexer with the live mode without the prior data it will try to start the indexer from the -height 1. If the RPC node doesn't have that height it will fail. So you can use the skip-db-check flag so that the +If you start the indexer in live mode without prior data, it will try to start from +height 1. If the RPC node doesn't have that height, it will fail. You can use the skip-db-check flag so that live modeAlso applies to: 249-259
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@docs/setup.md` at line 42, Fix the user-facing wording and typos in docs/setup.md: replace "However if you are planning to add a lot of services a top of the database maybe increase it." with clearer phrasing such as "However, if you plan to add many services on top of the database, consider increasing its resources." Replace informal fragments like "These mods" with "These modifications" and rework the live/historic guidance in the other updated block (lines referenced 249-259) to use concise, parallel phrasing (e.g., "Use the live configuration for real-time data and the historic configuration for archived data") and add missing commas and articles for readability.docs/compression.md-35-36 (1)
35-36:⚠️ Potential issue | 🟡 MinorMinor grammar fix: use hyphen for compound adjective.
"production ready" should be hyphenated when used as a compound adjective before a noun.
📝 Proposed fix
-The current dictionary is not production ready. It was trained from the real data but the sample was small. +The current dictionary is not production-ready. It was trained from the real data but the sample was small.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@docs/compression.md` around lines 35 - 36, The phrase "production ready" in docs/compression.md should be hyphenated as "production-ready" when used as a compound adjective before "dictionary"; update the sentence in the paragraph that currently reads "The current dictionary is not production ready." to "The current dictionary is not production-ready." so the compound adjective is grammatically correct.pkgs/database/query_block.go-124-152 (1)
124-152:⚠️ Potential issue | 🟡 MinorMissing
rows.Err()check andORDER BYclause.Two issues in
GetFromToBlocks:
- Unlike
GetLastXBlocks, this method doesn't checkrows.Err()after iterating, which could miss iteration errors.- The query lacks an
ORDER BYclause, resulting in undefined row order (PostgreSQL doesn't guarantee order without it).🔧 Proposed fix
WHERE height >= $1 AND height <= $2 AND chain_name = $3 + ORDER BY height ASC ` rows, err := t.pool.Query(ctx, query, fromHeight, toHeight, chainName)blocks = append(blocks, block) } + if err := rows.Err(); err != nil { + return nil, err + } return blocks, nil }🤖 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 124 - 152, In GetFromToBlocks, add an ORDER BY clause to the SQL (e.g., "ORDER BY height ASC") so results have deterministic ordering, and after iterating the rows in the for rows.Next() loop call and return any iteration error by checking rows.Err() before returning blocks; update the query string in GetFromToBlocks and insert a rows.Err() check (and handle/return that error) to match the pattern used in GetLastXBlocks.indexer/cmd/live.go-4-4 (1)
4-4:⚠️ Potential issue | 🟡 MinorRemove leftover debug print from CLI path.
Line 79 writes unstructured stdout output and duplicates logger intent.
Suggested fix
-import ( - "fmt" +import ( @@ - fmt.Println(compressEvents) mainOperator.InitMainOperator(configPath, ".", rateLimitFlags, runningFlags)Also applies to: 79-79
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@indexer/cmd/live.go` at line 4, Remove the leftover unstructured stdout debug print (the fmt.Println/Printf call that writes to stdout around line 79) from indexer/cmd/live.go; delete the fmt import if it becomes unused and, if logging is required, replace that print with the structured logger already used in this file (use the existing logger call rather than writing to stdout).api/handlers/address_test.go-38-39 (1)
38-39:⚠️ Potential issue | 🟡 MinorUse
require.Lenbefore indexingAddressTxs.Line 39 can panic if the length assertion fails because
assertis non-fatal. Userequire.Len(...)beforeresponse.Body.AddressTxs[0].Suggested fix
- assert.Equal(t, 3, len(response.Body.AddressTxs)) + require.Len(t, response.Body.AddressTxs, 3) assert.Equal(t, "tx_hash_1", response.Body.AddressTxs[0].Hash)🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@api/handlers/address_test.go` around lines 38 - 39, The test currently uses assert.Equal to check the length then indexes response.Body.AddressTxs which can panic if the assertion fails; replace the non-fatal assert length check with a fatal check (use require.Len(t, response.Body.AddressTxs, 3)) before accessing response.Body.AddressTxs[0], keeping the subsequent assertion (assert.Equal(t, "tx_hash_1", response.Body.AddressTxs[0].Hash)) unchanged so the index is safe; update the test in address_test.go to import/use require and swap the length assertion accordingly.docs/data-model.md-52-53 (1)
52-53:⚠️ Potential issue | 🟡 MinorFix wording typo in processing description.
At Line [52], “data is gathered and processes” should be “data is gathered and processed”.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@docs/data-model.md` around lines 52 - 53, The sentence contains a wording typo: replace "data is gathered and processes" with "data is gathered and processed" in the paragraph that starts with "data is gathered and processes and all of the transaction general data and messages contained in the transaction"; ensure the corrected phrase reads "data is gathered and processed" so the description is grammatically correct.docs/data-model.md-90-91 (1)
90-91:⚠️ Potential issue | 🟡 MinorAddress table uniqueness constraints look inconsistent.
At Lines [90-91] and [95-96], marking
chain_nameasUNIQUEwould allow only one row per chain in each address table. This likely should be a composite uniqueness on(address, chain_name)instead.Also applies to: 95-96
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@docs/data-model.md` around lines 90 - 91, The address table currently marks chain_name as UNIQUE (alongside address), which incorrectly enforces one row per chain; change the constraint so that the uniqueness is a composite key on (address, chain_name) instead of a single-column UNIQUE on chain_name — update the table definitions referencing the address table and the columns address and chain_name to use a composite UNIQUE(address, chain_name) (apply the same fix to the second occurrence noted).compression/train/init.go-45-50 (1)
45-50:⚠️ Potential issue | 🟡 MinorTighten config path validation and fix message mismatch.
At Line [49], the message says only
.ymlis valid, but.yamlis also accepted. Also, empty string should return a required-path error.✅ Proposed fix
- if configPath == nil { + if configPath == nil || strings.TrimSpace(*configPath) == "" { return config, fmt.Errorf("config path is required") } if !strings.HasSuffix(*configPath, ".yml") && !strings.HasSuffix(*configPath, ".yaml") { - return config, fmt.Errorf("config path must end with .yml") + return config, fmt.Errorf("config path must end with .yml or .yaml") }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@compression/train/init.go` around lines 45 - 50, The config path validation around the configPath variable should be tightened: ensure you treat both nil and empty (e.g., strings.TrimSpace(*configPath) == "") as a missing required path and return an error from the same check, and update the suffix validation to require either ".yml" or ".yaml" with an error message that matches (e.g., "config path must end with .yml or .yaml") — locate the validation logic that returns errors around configPath in compression/train/init.go and adjust the conditions and messages accordingly.compression/cmd/main.go-31-34 (1)
31-34:⚠️ Potential issue | 🟡 MinorFail fast when
--configis missing.At Line [31], an empty config path falls through and later triggers a misleading extension error. Validate emptiness right after reading the flag.
✅ Proposed fix
import ( "log" "os" + "strings" @@ configPath, err := cmd.Flags().GetString("config") if err != nil { log.Fatalf("failed to get config path: %v", err) } + if strings.TrimSpace(configPath) == "" { + log.Fatal("config path is required") + }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@compression/cmd/main.go` around lines 31 - 34, After reading the config flag into configPath using cmd.Flags().GetString("config"), validate that configPath is not empty and fail fast if it is: immediately check if configPath == "" and call log.Fatalf with a clear message (e.g., "missing --config flag") so the program exits with a helpful error instead of allowing later misleading errors; add this check right after the GetString call where configPath is defined.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: ae406382-b4e3-4236-a149-480ce333d446
⛔ Files ignored due to path filters (4)
go.sumis excluded by!**/*.sumindexer/events_proto/events.pb.gois excluded by!**/*.pb.gopkgs/dict_loader/events.zstd.binis excluded by!**/*.binpkgs/events_proto/events.pb.gois excluded by!**/*.pb.go
📒 Files selected for processing (77)
.github/workflows/release.yml.gitignoreDockerfileDockerfile-apiMakefileREADME.mdapi/handlers/address.goapi/handlers/address_test.goapi/handlers/convert_base64.goapi/handlers/database_test.goapi/handlers/interface.goapi/handlers/transactions.goapi/handlers/transactions_test.goapi/huma-types/address.goapi/huma-types/convert.goapi/huma-types/transaction.goapi/main.gocompression/cmd/main.gocompression/train/init.gocompression/train/process.goconfig-api.yml.exampledocker-compose-dev.ymldocker-compose.ymldocs/README.mddocs/api.mddocs/benchmarks.mddocs/compression.mddocs/data-model.mddocs/requirements.mddocs/scaling.mddocs/setup.mdexperiments/experiment1/enc_dec_test.goexperiments/experiment10/main.goexperiments/experiment11/main.goexperiments/experiment2/main.goexperiments/experiment3/rate_limited_rpc_example.goexperiments/experiment3/rate_limited_rpc_test.goexperiments/experiment4/encode_proto.goexperiments/experiment6/main.goexperiments/experiment7/main.goexperiments/experiment8/main.goexperiments/experiment9/main.gogo.modindexer/cmd/func.goindexer/cmd/historic.goindexer/cmd/live.goindexer/cmd/root.goindexer/cmd/setup.goindexer/context_hook/hook.goindexer/data_processor/event.goindexer/data_processor/operator.goindexer/db_init/hypertable.goindexer/db_init/tables.goindexer/indexer.goindexer/main_operator/operator.goindexer/orchestrator/operator.goindexer/orchestrator/operator_test.goindexer/query/operator.goindexer/retry/worker.goindexer/rpc_client/client.gointegration/HISTORIC_REPORTS.MDintegration/synthetic/init.gointegration/synthetic/query_operator.gopkgs/database/queries_address.gopkgs/database/queries_tx.gopkgs/database/query_block.gopkgs/database/query_msg.gopkgs/database/types_api.gopkgs/dict_loader/loader.gopkgs/events_proto/parse.gopkgs/generator/general.gopkgs/logger/logger.gopkgs/sql_data_types/table.goproto/buf.gen.yamlproto/buf.yamlproto/events.prototraining-config.example.yml
💤 Files with no reviewable changes (11)
- experiments/experiment3/rate_limited_rpc_example.go
- experiments/experiment2/main.go
- experiments/experiment11/main.go
- experiments/experiment6/main.go
- experiments/experiment10/main.go
- experiments/experiment3/rate_limited_rpc_test.go
- experiments/experiment4/encode_proto.go
- experiments/experiment9/main.go
- experiments/experiment8/main.go
- experiments/experiment1/enc_dec_test.go
- experiments/experiment7/main.go
There was a problem hiding this comment.
Actionable comments posted: 9
🧹 Nitpick comments (4)
Dockerfile-api (3)
19-19: Empty placeholder config may cause runtime issues.
touch config-api.ymlcreates an empty file. Perapi/config/loader.go(lines 54-70),LoadConfigusesyaml.Unmarshalon the file contents. While Host and Port have defaults, an empty file may still cause parsing issues or missing required fields at runtime.Consider either:
- Not copying a config file (let users mount their own)
- Creating a minimal valid YAML config with defaults
Option: Create minimal valid config
-RUN touch config-api.yml +RUN echo "# Default API config - override at runtime" > config-api.yml🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@Dockerfile-api` at line 19, The Dockerfile currently runs "touch config-api.yml" which creates an empty config that can break yaml.Unmarshal in LoadConfig (function LoadConfig in api/config/loader.go); replace this placeholder with either (A) remove the touch and let operators mount their own config, or (B) create a minimal valid YAML config file containing the required fields and sensible defaults (Host, Port, and any required keys referenced by LoadConfig) so yaml.Unmarshal always receives valid YAML; update the RUN step to write that minimal YAML instead of touching an empty file and ensure the filename matches what LoadConfig expects.
20-20:chmod +xis unnecessary for Go binaries.Go's build output is executable by default. This line can be removed.
Remove unnecessary chmod
RUN touch config-api.yml -RUN chmod +x build/api🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@Dockerfile-api` at line 20, Remove the redundant executable permission change: delete the RUN chmod +x build/api instruction in the Dockerfile since Go build artifacts (the build/api binary) are already executable by default; simply remove that RUN line to simplify the Dockerfile and avoid unnecessary steps.
1-1: Update to Go 1.26.0, the latest stable release.Go 1.25 is available, but Go 1.26.0 was released on February 10, 2026 and is now the current stable version. Update the base image to
golang:1.26to use the latest stable release.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@Dockerfile-api` at line 1, Update the Dockerfile base image FROM directive to use the new Go stable release by changing the existing FROM golang:1.25 reference to FROM golang:1.26 (or golang:1.26.0) in the Dockerfile's FROM line so the build uses Go 1.26; ensure any other Dockerfiles or CI workflows referencing golang:1.25 are updated consistently.indexer/cmd/setup.go (1)
290-292: Add contextual error logging for config creation failuresLine 290-Line 292 returns raw errors without command context. Logging file path here keeps error diagnostics consistent with the rest of this file.
💡 Proposed fix
overwrite, _ := cmd.Flags().GetBool("overwrite") if err := createConfig(overwrite, configFileName); err != nil { + l.Error().Err(err).Str("file", configFileName).Msg("failed to create config file") return err }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@indexer/cmd/setup.go` around lines 290 - 292, Wrap the raw error returned from createConfig(overwrite, configFileName) with contextual information including the configFileName (and command/context if appropriate) instead of returning err directly; e.g., replace the plain return err with returning a wrapped error that includes "failed to create config" and the configFileName using fmt.Errorf or errors.Wrap so callers/logs show the file path and command context; locate the call to createConfig and update its error return accordingly.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@indexer/cmd/setup.go`:
- Around line 229-243: Validation branches currently return cmd.Usage(), which
can be nil and causes RunE to exit with success; update each validation block
(the privilege and userName checks around the privilege variable and userName
variable) to call cmd.Usage() for UI output but then return a non-nil error
(e.g., errors.New("invalid privilege") or errors.New("user name is required"))
so RunE fails properly; ensure you import or use the errors package and return
the specific error after calling cmd.Usage().
In `@indexer/data_processor/event.go`:
- Line 83: The compression behavior is inconsistent: the code only compresses
when useCompressed && evCount >= 2 but the function/docs promise compressed
output whenever useCompressed is true; change the implementation in the event
processing function to honor useCompressed deterministically (remove or adjust
the evCount >= 2 gate so that when useCompressed is true the payload is always
compressed) OR update the function contract/comments to state that compression
is only applied for evCount >= 2; update the comment/README and any callers
expecting compression via the useCompressed flag accordingly and keep references
to the useCompressed variable and the evCount check (evCount >= 2) so reviewers
can find the change.
- Around line 128-133: The code creates protoEv := &events_proto.Event{ ...
PkgPath: &event.PkgPath } inside a range loop which takes the address of the
range variable (event) reused each iteration, causing pointer aliasing; fix it
by capturing the field into a new local variable and taking its address (e.g.
pkgPath := event.PkgPath; PkgPath: &pkgPath) or by assigning a non-pointer value
if appropriate, replacing the use of &event.PkgPath in the events_proto.Event
initialization to avoid referencing the reused range variable.
In `@pkgs/database/queries_address.go`:
- Around line 257-259: The cursor encoding currently calls timestamp =
timestamp.Round(time.Second) which strips sub-second precision and can break
pagination; remove the rounding and format the timestamp with full precision
(e.g., use timestamp.Format(time.RFC3339Nano) instead of time.RFC3339) so the
cursor preserves ordering; apply the same change to the other occurrence (lines
referenced around the second cursor creation) so both cursor constructions (the
block with timestamp.Round and the later similar block) stop truncating
sub-second precision.
- Around line 145-156: The initial page's ORDER BY only uses tx.timestamp while
later cursor pages order by (tx.timestamp, tx.tx_hash), causing unstable
pagination; update the initial query (the string assigned to variable `query` in
this file where args are appended with `accountId, chainName, fetchLimit`) to
ORDER BY both `tx.timestamp` and `tx.tx_hash` with the same directions used for
subsequent pages (e.g., add `, tx.tx_hash` with the same DESC/ASC as the other
queries) so all cursor pages use an identical, stable sort on `address_tx`
(`tx.timestamp`, `tx.tx_hash`).
In `@pkgs/database/queries_tx.go`:
- Around line 151-159: GetTransactionsByOffset is selecting tx fields but omits
tx_events_compressed and compression_on, so ToTransaction can't reconstruct
compressed events; update the SELECT and corresponding Scan usage in
GetTransactionsByOffset (and the similar block at lines referenced) to include
tx_events_compressed and compression_on alongside tx_events so the row scanner
passes those values into ToTransaction (or the struct it populates) allowing
decompression logic to run correctly.
- Around line 295-306: The decodeEvents function currently always calls
attribute.GetStringValue() (losing non-string types) and unconditionally
dereferences event.PkgPath (panicking if nil); update the attribute loop in
decodeEvents to inspect the attribute value oneof (e.g., switch on
attribute.Value's kind or use the generated getters for String/Int/Bool/Double)
and convert each type to a canonical string or typed representation for the
Attribute.Value field, and change the Event construction to safely handle
optional PkgPath (check event.PkgPath for nil and use a zero/empty value or omit
it instead of dereferencing). Ensure you modify the Attribute population code
and the Event{ PkgPath: ... } assignment accordingly.
- Around line 14-17: The code ignores the error returned by zstd.NewReader which
can leave zstdReader nil and cause a panic when calling DecodeAll (see symbols
dictBytes, zstdDict, zstdReader, zstd.NewReader, DecodeAll); update
initialization to capture and handle the error: call zstd.NewReader(nil,
zstdDict) and if it returns an error, log/return it (or panic early) and ensure
zstdReader is non-nil before use (or fall back to a safe decoder), and propagate
the error from the package init so callers don’t hit a nil zstdReader at
runtime.
In `@pkgs/database/types_api.go`:
- Around line 108-122: The ToTransaction function currently rejects a nil decode
function unconditionally; only require decode when f.CompressionOn is true. Move
the decode == nil check inside the if f.CompressionOn branch (or only perform it
when CompressionOn), call decode(f.TxEventsCompressed) there and handle its
error, and in the else branch use the uncompressed events (f.TxEvents) to
populate tx.Events; ensure you still set tx.Events appropriately and return
informative errors when decode is required but missing or fails.
---
Nitpick comments:
In `@Dockerfile-api`:
- Line 19: The Dockerfile currently runs "touch config-api.yml" which creates an
empty config that can break yaml.Unmarshal in LoadConfig (function LoadConfig in
api/config/loader.go); replace this placeholder with either (A) remove the touch
and let operators mount their own config, or (B) create a minimal valid YAML
config file containing the required fields and sensible defaults (Host, Port,
and any required keys referenced by LoadConfig) so yaml.Unmarshal always
receives valid YAML; update the RUN step to write that minimal YAML instead of
touching an empty file and ensure the filename matches what LoadConfig expects.
- Line 20: Remove the redundant executable permission change: delete the RUN
chmod +x build/api instruction in the Dockerfile since Go build artifacts (the
build/api binary) are already executable by default; simply remove that RUN line
to simplify the Dockerfile and avoid unnecessary steps.
- Line 1: Update the Dockerfile base image FROM directive to use the new Go
stable release by changing the existing FROM golang:1.25 reference to FROM
golang:1.26 (or golang:1.26.0) in the Dockerfile's FROM line so the build uses
Go 1.26; ensure any other Dockerfiles or CI workflows referencing golang:1.25
are updated consistently.
In `@indexer/cmd/setup.go`:
- Around line 290-292: Wrap the raw error returned from createConfig(overwrite,
configFileName) with contextual information including the configFileName (and
command/context if appropriate) instead of returning err directly; e.g., replace
the plain return err with returning a wrapped error that includes "failed to
create config" and the configFileName using fmt.Errorf or errors.Wrap so
callers/logs show the file path and command context; locate the call to
createConfig and update its error return accordingly.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 29d763f3-67db-4fcf-bf1d-1422ebc94276
⛔ Files ignored due to path filters (1)
pkgs/events_proto/events_proto/events.pb.gois excluded by!**/*.pb.go
📒 Files selected for processing (8)
.github/workflows/release.ymlDockerfile-apiindexer/cmd/setup.goindexer/data_processor/event.gopkgs/database/queries_address.gopkgs/database/queries_tx.gopkgs/database/types_api.goproto/events_proto/events.proto
Comment on lines
229
to
243
| privilege, _ := cmd.Flags().GetString("privilege") | ||
| if privilege == "" { | ||
| log.Fatalf("privilege is required") | ||
| l.Error().Msg("privilege is required") | ||
| return cmd.Usage() | ||
| } else if privilege != "reader" && privilege != "writer" { | ||
| log.Fatalf("invalid privilege: %s", privilege) | ||
| l.Error().Str("privilege", privilege).Msg("invalid privilege") | ||
| return cmd.Usage() | ||
| } | ||
|
|
||
| // get the user name from the flags | ||
| userName, _ := cmd.Flags().GetString("user") | ||
| if userName == "" { | ||
| log.Fatalf("user name is required") | ||
| l.Error().Msg("user name is required") | ||
| return cmd.Usage() | ||
| } |
There was a problem hiding this comment.
🧩 Analysis chain
🌐 Web query:
Cobra (*Command).Usage() return type error behavior
💡 Result:
In spf13/cobra, (*cobra.Command).Usage() returns an error (not a string). It simply calls the currently active UsageFunc and returns whatever that function returns. [1][2]
Default behavior (when you did not override UsageFunc):
- Cobra renders the usage template to
cmd.OutOrStderr()and returns the template execution error (if any). [2] - If rendering fails, the default
UsageFuncalso prints that rendering error (viaPrintErrln) and then returns it. [2] - If rendering succeeds, it returns
nil. [2]
Related: UsageString() captures usage into a buffer, but it calls CheckErr(cmd.Usage()), and CheckErr exits the process with code 1 if the error is non-nil—so template/rendering errors can terminate your program when using UsageString(). [2][3]
Sources
- [1] Cobra API docs (
func (c *Command) Usage() error) - [2]
command.go(v1.10.2) implementation ofUsage(), defaultUsageFunc(), andUsageString() - [3] Cobra API docs for
CheckErrbehavior
🏁 Script executed:
# Check the actual file to understand context
fd setup.go -t f | head -5Repository: Cogwheel-Validator/spectra-gnoland-indexer
Length of output: 106
🏁 Script executed:
# Read the setup.go file around lines 229-243
sed -n '220,250p' indexer/cmd/setup.goRepository: Cogwheel-Validator/spectra-gnoland-indexer
Length of output: 920
🏁 Script executed:
# Check if there are other validation failures returning cmd.Usage() in the same file
rg "return cmd\.Usage\(\)" indexer/cmd/setup.goRepository: Cogwheel-Validator/spectra-gnoland-indexer
Length of output: 151
🏁 Script executed:
# Check the broader context to see the function signature and understand the RunE behavior
sed -n '200,270p' indexer/cmd/setup.go | head -80Repository: Cogwheel-Validator/spectra-gnoland-indexer
Length of output: 2251
🏁 Script executed:
# Check if there's an imports section to see current state
sed -n '1,30p' indexer/cmd/setup.goRepository: Cogwheel-Validator/spectra-gnoland-indexer
Length of output: 849
Validation failures return success exit code
At lines 232, 235, and 242, returning cmd.Usage() returns nil when template rendering succeeds, causing RunE to exit with code 0 despite validation failures. Return a real error after printing usage.
Proposed fix
import (
"context"
+ "errors"
"time"
privilege, _ := cmd.Flags().GetString("privilege")
if privilege == "" {
l.Error().Msg("privilege is required")
- return cmd.Usage()
+ _ = cmd.Usage()
+ return errors.New("privilege is required")
} else if privilege != "reader" && privilege != "writer" {
l.Error().Str("privilege", privilege).Msg("invalid privilege")
- return cmd.Usage()
+ _ = cmd.Usage()
+ return errors.New("invalid privilege: must be reader or writer")
}
// get the user name from the flags
userName, _ := cmd.Flags().GetString("user")
if userName == "" {
l.Error().Msg("user name is required")
- return cmd.Usage()
+ _ = cmd.Usage()
+ return errors.New("user name is required")
}📝 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
| privilege, _ := cmd.Flags().GetString("privilege") | |
| if privilege == "" { | |
| log.Fatalf("privilege is required") | |
| l.Error().Msg("privilege is required") | |
| return cmd.Usage() | |
| } else if privilege != "reader" && privilege != "writer" { | |
| log.Fatalf("invalid privilege: %s", privilege) | |
| l.Error().Str("privilege", privilege).Msg("invalid privilege") | |
| return cmd.Usage() | |
| } | |
| // get the user name from the flags | |
| userName, _ := cmd.Flags().GetString("user") | |
| if userName == "" { | |
| log.Fatalf("user name is required") | |
| l.Error().Msg("user name is required") | |
| return cmd.Usage() | |
| } | |
| privilege, _ := cmd.Flags().GetString("privilege") | |
| if privilege == "" { | |
| l.Error().Msg("privilege is required") | |
| _ = cmd.Usage() | |
| return errors.New("privilege is required") | |
| } else if privilege != "reader" && privilege != "writer" { | |
| l.Error().Str("privilege", privilege).Msg("invalid privilege") | |
| _ = cmd.Usage() | |
| return errors.New("invalid privilege: must be reader or writer") | |
| } | |
| // get the user name from the flags | |
| userName, _ := cmd.Flags().GetString("user") | |
| if userName == "" { | |
| l.Error().Msg("user name is required") | |
| _ = cmd.Usage() | |
| return errors.New("user name is required") | |
| } |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@indexer/cmd/setup.go` around lines 229 - 243, Validation branches currently
return cmd.Usage(), which can be nil and causes RunE to exit with success;
update each validation block (the privilege and userName checks around the
privilege variable and userName variable) to call cmd.Usage() for UI output but
then return a non-nil error (e.g., errors.New("invalid privilege") or
errors.New("user name is required")) so RunE fails properly; ensure you import
or use the errors package and return the specific error after calling
cmd.Usage().
| return nil, fmt.Errorf("compressed format not yet implemented") | ||
| events := &txResponse.Result.TxResult.ResponseBase.Events | ||
| evCount := len(*events) | ||
| if useCompressed && evCount >= 2 { |
There was a problem hiding this comment.
Compression contract is inconsistent with the function docs.
Line 83 only compresses when evCount >= 2, but the docs say useCompressed=true returns compressed format. Either align behavior or update the contract/comments so callers can rely on deterministic output format.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@indexer/data_processor/event.go` at line 83, The compression behavior is
inconsistent: the code only compresses when useCompressed && evCount >= 2 but
the function/docs promise compressed output whenever useCompressed is true;
change the implementation in the event processing function to honor
useCompressed deterministically (remove or adjust the evCount >= 2 gate so that
when useCompressed is true the payload is always compressed) OR update the
function contract/comments to state that compression is only applied for evCount
>= 2; update the comment/README and any callers expecting compression via the
useCompressed flag accordingly and keep references to the useCompressed variable
and the evCount check (evCount >= 2) so reviewers can find the change.
Comment on lines
+257
to
+259
| timestamp = timestamp.Round(time.Second) | ||
| base64Url := base64.URLEncoding.Strict().EncodeToString(txHashBytes) | ||
| return timestamp.Format(time.RFC3339) + "|" + base64Url |
There was a problem hiding this comment.
Cursor timestamp precision is reduced, which can break page boundaries.
Line 257 rounds timestamps to whole seconds before cursor encoding. That loses ordering precision and can cause skipped/duplicated rows when multiple txs share the same second.
💡 Suggested fix
- timestamp = timestamp.Round(time.Second)
base64Url := base64.URLEncoding.Strict().EncodeToString(txHashBytes)
- return timestamp.Format(time.RFC3339) + "|" + base64Url
+ return timestamp.Format(time.RFC3339Nano) + "|" + base64Url- timestamp, err := time.Parse(time.RFC3339, parts[0])
+ timestamp, err := time.Parse(time.RFC3339Nano, parts[0])Also applies to: 269-270
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@pkgs/database/queries_address.go` around lines 257 - 259, The cursor encoding
currently calls timestamp = timestamp.Round(time.Second) which strips sub-second
precision and can break pagination; remove the rounding and format the timestamp
with full precision (e.g., use timestamp.Format(time.RFC3339Nano) instead of
time.RFC3339) so the cursor preserves ordering; apply the same change to the
other occurrence (lines referenced around the second cursor creation) so both
cursor constructions (the block with timestamp.Round and the later similar
block) stop truncating sub-second precision.
Comment on lines
+151
to
+159
| SELECT | ||
| encode(tx.tx_hash, 'base64') AS tx_hash, | ||
| tx.timestamp, | ||
| tx.block_height, | ||
| tx.tx_events, | ||
| tx.gas_used, | ||
| tx.gas_wanted, | ||
| tx.fee, | ||
| tx.msg_types |
There was a problem hiding this comment.
Offset query bypasses compressed-event decoding fields.
GetTransactionsByOffset does not select/scan tx_events_compressed and compression_on, so compressed rows cannot be reconstructed through ToTransaction in this path.
💡 Suggested fix
SELECT
encode(tx.tx_hash, 'base64') AS tx_hash,
tx.timestamp,
tx.block_height,
tx.tx_events,
+ tx.tx_events_compressed,
+ tx.compression_on,
tx.gas_used,
tx.gas_wanted,
tx.fee,
tx.msg_types
@@
- err := rows.Scan(&transaction.TxHash, &transaction.Timestamp, &transaction.BlockHeight, &transaction.TxEvents, &transaction.GasUsed, &transaction.GasWanted, &transaction.Fee, &transaction.MsgTypes)
+ err := rows.Scan(
+ &transaction.TxHash,
+ &transaction.Timestamp,
+ &transaction.BlockHeight,
+ &transaction.TxEvents,
+ &transaction.TxEventsCompressed,
+ &transaction.CompressionOn,
+ &transaction.GasUsed,
+ &transaction.GasWanted,
+ &transaction.Fee,
+ &transaction.MsgTypes,
+ )Also applies to: 173-177
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@pkgs/database/queries_tx.go` around lines 151 - 159, GetTransactionsByOffset
is selecting tx fields but omits tx_events_compressed and compression_on, so
ToTransaction can't reconstruct compressed events; update the SELECT and
corresponding Scan usage in GetTransactionsByOffset (and the similar block at
lines referenced) to include tx_events_compressed and compression_on alongside
tx_events so the row scanner passes those values into ToTransaction (or the
struct it populates) allowing decompression logic to run correctly.
Comment on lines
+108
to
+122
| func (f *FullTxData) ToTransaction(decode func([]byte) (*[]Event, error)) (*Transaction, error) { | ||
| if decode == nil { | ||
| return nil, errors.New("decode function is nil") | ||
| } | ||
| tx := &Transaction{ | ||
| TxHash: f.TxHash, | ||
| Timestamp: f.Timestamp, | ||
| BlockHeight: f.BlockHeight, | ||
| GasUsed: f.GasUsed, | ||
| GasWanted: f.GasWanted, | ||
| Fee: f.Fee, | ||
| MsgTypes: f.MsgTypes, | ||
| } | ||
| if f.CompressionOn { | ||
| events, err := decode(f.TxEventsCompressed) |
There was a problem hiding this comment.
decode should only be required when compression is enabled.
Line 109 currently errors on decode == nil even for uncompressed transactions (CompressionOn == false). That introduces an unnecessary failure mode for callers using plain TxEvents.
💡 Suggested fix
func (f *FullTxData) ToTransaction(decode func([]byte) (*[]Event, error)) (*Transaction, error) {
- if decode == nil {
- return nil, errors.New("decode function is nil")
- }
tx := &Transaction{
TxHash: f.TxHash,
Timestamp: f.Timestamp,
BlockHeight: f.BlockHeight,
@@
}
if f.CompressionOn {
+ if decode == nil {
+ return nil, errors.New("decode function is required when compression is enabled")
+ }
events, err := decode(f.TxEventsCompressed)🤖 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 108 - 122, The ToTransaction
function currently rejects a nil decode function unconditionally; only require
decode when f.CompressionOn is true. Move the decode == nil check inside the if
f.CompressionOn branch (or only perform it when CompressionOn), call
decode(f.TxEventsCompressed) there and handle its error, and in the else branch
use the uncompressed events (f.TxEvents) to populate tx.Events; ensure you still
set tx.Events appropriately and return informative errors when decode is
required but missing or fails.
There was a problem hiding this comment.
Actionable comments posted: 3
♻️ Duplicate comments (4)
indexer/data_processor/event.go (1)
75-76:⚠️ Potential issue | 🟡 MinorCompression contract still conflicts with runtime behavior.
Line 75 says
useCompressed=truereturns compressed format, but Line 91 still requiresevCount >= 2. Either update the doc contract or make behavior deterministic onuseCompressedalone.Also applies to: 91-91
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@indexer/data_processor/event.go` around lines 75 - 76, The comment vs runtime disagree: the header says useCompressed=true should return compressed format but the implementation still gates compression on evCount >= 2; update the code so behavior is deterministic based only on useCompressed (or update the doc to state the evCount requirement). Concretely, in event.go change the branch that currently checks evCount before compressing to instead check useCompressed (i.e., if useCompressed then produce compressed output regardless of evCount; otherwise produce native), and adjust or remove the evCount condition and update the header comment to match the final behavior; reference the variables useCompressed and evCount in the affected function to locate the logic to change.indexer/cmd/setup.go (1)
224-242:⚠️ Potential issue | 🟠 MajorAvoid
Fatal()insideRunE; return explicit errors after printing usage.On Line 224, Line 231, Line 234, Line 241, Line 248, Line 260, and Line 267,
l.Fatal()in aRunEpath can short-circuit command error propagation. Also, Line 232, Line 235, and Line 242 returningcmd.Usage()is unreliable for validation failures; return a non-nil validation error after calling usage.💡 Proposed fix
import ( "context" + "errors" "time" @@ params, err := parseCommonFlags(cmd, "gnoland") if err != nil { - l.Fatal().Err(err).Msg("failed to parse flags") + l.Error().Err(err).Msg("failed to parse flags") return err } @@ privilege, _ := cmd.Flags().GetString("privilege") if privilege == "" { - l.Fatal().Msg("privilege is required") - return cmd.Usage() + l.Error().Msg("privilege is required") + _ = cmd.Usage() + return errors.New("privilege is required") } else if privilege != "reader" && privilege != "writer" { - l.Fatal().Str("privilege", privilege).Msg("invalid privilege") - return cmd.Usage() + l.Error().Str("privilege", privilege).Msg("invalid privilege") + _ = cmd.Usage() + return errors.New("invalid privilege: must be reader or writer") } @@ userName, _ := cmd.Flags().GetString("user") if userName == "" { - l.Fatal().Msg("user name is required") - return cmd.Usage() + l.Error().Msg("user name is required") + _ = cmd.Usage() + return errors.New("user name is required") } @@ params.password, err = promptPassword() if err != nil { - l.Fatal().Err(err).Msg("failed to read password") + l.Error().Err(err).Msg("failed to read password") return err } @@ err = dbInit.CreateUser(userName) if err != nil { - l.Fatal().Err(err).Str("user", userName).Msg("failed to create user") + l.Error().Err(err).Str("user", userName).Msg("failed to create user") return err } @@ err = dbInit.AppointPrivileges(userName, privilege, []string{}) if err != nil { - l.Fatal().Err(err).Str("user", userName).Str("privilege", privilege).Msg("failed to appoint privileges") + l.Error().Err(err).Str("user", userName).Str("privilege", privilege).Msg("failed to appoint privileges") return err }#!/bin/bash # Verify logger fatal semantics and affected call sites. fd logger.go sed -n '1,240p' pkgs/logger/logger.go rg -n -C2 '\.Fatal\(' indexer/cmd/setup.go rg -n -C2 'return cmd\.Usage\(\)' indexer/cmd/setup.goAlso applies to: 248-249, 260-268
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@indexer/cmd/setup.go` around lines 224 - 242, Replace l.Fatal() calls inside the RunE implementation with non-fatal logging (e.g., l.Error() or l.Warn()), call cmd.Usage() only to print usage, and then return an explicit non-nil validation error (e.g., fmt.Errorf("missing privilege") or fmt.Errorf("invalid privilege: %s", privilege)). Specifically, update the checks that use cmd.Flags().GetString("privilege") and cmd.Flags().GetString("user") (and any other similar flag checks) to log via l.Error()/l.Warn(), invoke cmd.Usage() to show help, and return a clear error instead of returning cmd.Usage() or calling l.Fatal(); preserve existing log fields (like .Str("privilege", privilege)) when converting to non-fatal logs. Ensure all instances of l.Fatal() in RunE are handled this way so command error propagation remains intact.pkgs/database/queries_tx.go (1)
168-176:⚠️ Potential issue | 🟠 MajorOffset pagination still omits compression fields used by
ToTransaction.At Line 168-Line 176 and Line 190,
GetTransactionsByOffsetdoes not select/scantx_events_compressedandcompression_on. Inpkgs/database/types_api.go(FullTxData.ToTransaction), decoding is gated byCompressionOn, so compressed rows are not reconstructed on this path.Suggested fix
SELECT encode(tx.tx_hash, 'base64') AS tx_hash, tx.timestamp, tx.block_height, tx.tx_events, + tx.tx_events_compressed, + tx.compression_on, tx.gas_used, tx.gas_wanted, tx.fee, tx.msg_types @@ - err := rows.Scan(&transaction.TxHash, &transaction.Timestamp, &transaction.BlockHeight, &transaction.TxEvents, &transaction.GasUsed, &transaction.GasWanted, &transaction.Fee, &transaction.MsgTypes) + err := rows.Scan( + &transaction.TxHash, + &transaction.Timestamp, + &transaction.BlockHeight, + &transaction.TxEvents, + &transaction.TxEventsCompressed, + &transaction.CompressionOn, + &transaction.GasUsed, + &transaction.GasWanted, + &transaction.Fee, + &transaction.MsgTypes, + )Also applies to: 190-195
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@pkgs/database/queries_tx.go` around lines 168 - 176, GetTransactionsByOffset is missing the compressed fields so rows with compressed event payloads never reconstruct via FullTxData.ToTransaction; update the SELECT in GetTransactionsByOffset to include tx_events_compressed and compression_on (same as the cursor pagination path at lines ~190) and update the corresponding scan/dest variables in the GetTransactionsByOffset query handling so FullTxData.CompressionOn and FullTxData.TxEventsCompressed are populated before calling ToTransaction.pkgs/database/queries_address.go (1)
257-259:⚠️ Potential issue | 🟠 MajorCursor timestamp precision is still truncated (previously reported).
This still rounds to seconds and serializes/parses with
time.RFC3339, which can break page boundaries when multiple transactions share a second.🧭 Proposed fix
- timestamp = timestamp.Round(time.Second) base64Url := base64.URLEncoding.Strict().EncodeToString(txHashBytes) - return timestamp.Format(time.RFC3339) + "|" + base64Url + return timestamp.Format(time.RFC3339Nano) + "|" + base64Url @@ - timestamp, err := time.Parse(time.RFC3339, parts[0]) + timestamp, err := time.Parse(time.RFC3339Nano, parts[0])#!/bin/bash # Verify cursor precision handling in pkgs/database/queries_address.go cat -n pkgs/database/queries_address.go | sed -n '248,274p'Expected after fix: no
Round(time.Second)andRFC3339Nanoused in both cursor marshal/unmarshal.Also applies to: 269-270
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@pkgs/database/queries_address.go` around lines 257 - 259, Remove the second-level rounding and switch to nanosecond precision when serializing/parsing the cursor: stop calling timestamp = timestamp.Round(time.Second) and instead format the timestamp with time.RFC3339Nano (replace timestamp.Format(time.RFC3339) with timestamp.Format(time.RFC3339Nano)), and update the corresponding cursor parse/unmarshal logic to use time.Parse(time.RFC3339Nano, ...) so page boundaries aren’t lost when multiple transactions share a second; apply the same change to the other cursor serialization site around the base64Url/txHashBytes code (the other occurrence noted at the 269-270 region).
🧹 Nitpick comments (3)
pkgs/database/queries_tx.go (1)
3-8: Use the structured logger consistently in DB query errors.Line 75 uses
log.Printlnwhile this file already useslogger.Get()(l) for structured logging. Switching this path keeps logs consistent with the rest of the module.Suggested refactor
import ( "context" "encoding/base64" - "log" "strconv" @@ - log.Println("error getting transaction", err) + l.Error().Err(err).Str("chain_name", chainName).Str("tx_hash", txHash).Msg("error getting transaction") return nil, errAlso applies to: 75-76
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@pkgs/database/queries_tx.go` around lines 3 - 8, Replace the plain std log usage at the log.Println call with the module's structured logger obtained via logger.Get() (variable l) so DB query errors are logged consistently; update the call site that currently uses log.Println to use l.Error / l.Errorf (or l.Errorw with fields) and include the error/context fields, remove the unused "log" import, and apply the same replacement for the other occurrence noted (the two occurrences around the log.Println call).pkgs/database/queries_address.go (2)
248-256: Don’t return sentinel cursor text on decode failure; propagate the error.Returning
"error decoding tx hash"creates an invalid cursor string and hides the underlying failure path.🛠️ Proposed fix
-func makeCursorParam( +func makeCursorParam( timestamp time.Time, txHash string, -) string { +) (string, error) { txHashBytes, err := base64.StdEncoding.DecodeString(txHash) if err != nil { - // TODO: log error - return "error decoding tx hash" + return "", fmt.Errorf("error decoding tx hash: %w", err) } timestamp = timestamp.Round(time.Second) base64Url := base64.URLEncoding.Strict().EncodeToString(txHashBytes) - return timestamp.Format(time.RFC3339) + "|" + base64Url + return timestamp.Format(time.RFC3339) + "|" + base64Url, nil } @@ - nextCursor := makeCursorParam(lastAddressTx.Timestamp, lastAddressTx.Hash) - return &page, nextCursor, nil + nextCursor, err := makeCursorParam(lastAddressTx.Timestamp, lastAddressTx.Hash) + if err != nil { + return nil, "", err + } + return &page, nextCursor, nilAlso applies to: 189-190
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@pkgs/database/queries_address.go` around lines 248 - 256, The function makeCursorParam should not swallow the base64 decode error; change its signature to return (string, error), return ("", err) (or wrapped fmt.Errorf with context) when base64.StdEncoding.DecodeString(txHash) fails instead of returning the sentinel string, and update all callers of makeCursorParam to handle the error. Apply the same change to the other similar helper that currently returns "error decoding tx hash" so both places propagate the decode error and include contextual error messages.
25-27: Update the function doc comment to match the actual return signature.The comment still documents two return values, but the function now returns
(*[]AddressTx, string, uint64, error).Also applies to: 37-37
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@pkgs/database/queries_address.go` around lines 25 - 27, Update the function doc comment that currently lists two return values ("- []*AddressTx" and "- error") so it matches the actual return signature of the function (returning (*[]AddressTx, string, uint64, error)); specifically replace the old two-item bullets with four bullets describing: *[]AddressTx (transactions), string (cursor/next-token), uint64 (count/total) and error to reflect the new return types used by the function.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@pkgs/database/queries_address.go`:
- Around line 136-141: Validate and sanitize the incoming limit before doing any
arithmetic or slice indexing: ensure that if limit is nil you set it to
&defaultLimit, then clamp *limit to a safe positive range (e.g., treat
non-positive values as default and cap very large values to a sensible maxLimit)
before computing fetchLimit so the addition cannot overflow; also guard any
access like page[len(page)-1] by checking len(page) > 0 (or only perform that
logic when you successfully fetched at least one row) so you never index an
empty slice. Reference: variables limit, defaultLimit, fetchLimit and the code
that reads page[len(page)-1].
- Around line 219-220: The ORDER BY clause using only tx.timestamp is unstable
for OFFSET pagination; update the query that contains "ORDER BY tx.timestamp
DESC" (in pkgs/database/queries_address.go) to add a deterministic secondary
sort key such as a unique transaction identifier (e.g., tx.id DESC or tx.hash
DESC) so rows with identical timestamps have a stable order; ensure the same
secondary key is used wherever pagination is applied so LIMIT $3 OFFSET $4
returns consistent, non-duplicating pages.
In `@pkgs/database/queries_tx.go`:
- Around line 95-110: The initial page query orders only by tx.timestamp which
can cause skips/duplicates versus cursor pagination that orders by
(tx.timestamp, tx.tx_hash); update the ORDER BY in the first-page query (the
query using transaction_general tx and selecting encode(tx.tx_hash, 'base64') AS
tx_hash) to ORDER BY tx.timestamp DESC, tx.tx_hash DESC (matching the cursor
ordering/direction), and make the same change to the other similar ORDER BY
clauses referenced around lines 214-216 and 240-242 so all pages use the
identical (tx.timestamp, tx.tx_hash) tie-breaker ordering.
---
Duplicate comments:
In `@indexer/cmd/setup.go`:
- Around line 224-242: Replace l.Fatal() calls inside the RunE implementation
with non-fatal logging (e.g., l.Error() or l.Warn()), call cmd.Usage() only to
print usage, and then return an explicit non-nil validation error (e.g.,
fmt.Errorf("missing privilege") or fmt.Errorf("invalid privilege: %s",
privilege)). Specifically, update the checks that use
cmd.Flags().GetString("privilege") and cmd.Flags().GetString("user") (and any
other similar flag checks) to log via l.Error()/l.Warn(), invoke cmd.Usage() to
show help, and return a clear error instead of returning cmd.Usage() or calling
l.Fatal(); preserve existing log fields (like .Str("privilege", privilege)) when
converting to non-fatal logs. Ensure all instances of l.Fatal() in RunE are
handled this way so command error propagation remains intact.
In `@indexer/data_processor/event.go`:
- Around line 75-76: The comment vs runtime disagree: the header says
useCompressed=true should return compressed format but the implementation still
gates compression on evCount >= 2; update the code so behavior is deterministic
based only on useCompressed (or update the doc to state the evCount
requirement). Concretely, in event.go change the branch that currently checks
evCount before compressing to instead check useCompressed (i.e., if
useCompressed then produce compressed output regardless of evCount; otherwise
produce native), and adjust or remove the evCount condition and update the
header comment to match the final behavior; reference the variables
useCompressed and evCount in the affected function to locate the logic to
change.
In `@pkgs/database/queries_address.go`:
- Around line 257-259: Remove the second-level rounding and switch to nanosecond
precision when serializing/parsing the cursor: stop calling timestamp =
timestamp.Round(time.Second) and instead format the timestamp with
time.RFC3339Nano (replace timestamp.Format(time.RFC3339) with
timestamp.Format(time.RFC3339Nano)), and update the corresponding cursor
parse/unmarshal logic to use time.Parse(time.RFC3339Nano, ...) so page
boundaries aren’t lost when multiple transactions share a second; apply the same
change to the other cursor serialization site around the base64Url/txHashBytes
code (the other occurrence noted at the 269-270 region).
In `@pkgs/database/queries_tx.go`:
- Around line 168-176: GetTransactionsByOffset is missing the compressed fields
so rows with compressed event payloads never reconstruct via
FullTxData.ToTransaction; update the SELECT in GetTransactionsByOffset to
include tx_events_compressed and compression_on (same as the cursor pagination
path at lines ~190) and update the corresponding scan/dest variables in the
GetTransactionsByOffset query handling so FullTxData.CompressionOn and
FullTxData.TxEventsCompressed are populated before calling ToTransaction.
---
Nitpick comments:
In `@pkgs/database/queries_address.go`:
- Around line 248-256: The function makeCursorParam should not swallow the
base64 decode error; change its signature to return (string, error), return ("",
err) (or wrapped fmt.Errorf with context) when
base64.StdEncoding.DecodeString(txHash) fails instead of returning the sentinel
string, and update all callers of makeCursorParam to handle the error. Apply the
same change to the other similar helper that currently returns "error decoding
tx hash" so both places propagate the decode error and include contextual error
messages.
- Around line 25-27: Update the function doc comment that currently lists two
return values ("- []*AddressTx" and "- error") so it matches the actual return
signature of the function (returning (*[]AddressTx, string, uint64, error));
specifically replace the old two-item bullets with four bullets describing:
*[]AddressTx (transactions), string (cursor/next-token), uint64 (count/total)
and error to reflect the new return types used by the function.
In `@pkgs/database/queries_tx.go`:
- Around line 3-8: Replace the plain std log usage at the log.Println call with
the module's structured logger obtained via logger.Get() (variable l) so DB
query errors are logged consistently; update the call site that currently uses
log.Println to use l.Error / l.Errorf (or l.Errorw with fields) and include the
error/context fields, remove the unused "log" import, and apply the same
replacement for the other occurrence noted (the two occurrences around the
log.Println call).
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 0dd88064-eb22-4a69-94ba-f7f061772db0
📒 Files selected for processing (4)
indexer/cmd/setup.goindexer/data_processor/event.gopkgs/database/queries_address.gopkgs/database/queries_tx.go
Comment on lines
+136
to
+141
| if limit == nil { | ||
| limit = &defaultLimit | ||
| } | ||
| // Fetch limit+1 to detect if there are more rows; only return limit to the caller. | ||
| fetchLimit := *limit + 1 | ||
| var query string |
There was a problem hiding this comment.
Validate limit before fetchLimit/slice math to prevent panic.
When limit == 0, Line 188 dereferences page[len(page)-1] on an empty slice and panics. Also, Line 140 can overflow when incrementing a very large limit.
💥 Proposed fix
func (t *TimescaleDb) getAddressTxsCursorQuery(
@@
) (*[]AddressTx, string, error) {
if limit == nil {
limit = &defaultLimit
}
+ if *limit == 0 {
+ return nil, "", fmt.Errorf("limit must be greater than 0")
+ }
// Fetch limit+1 to detect if there are more rows; only return limit to the caller.
fetchLimit := *limit + 1
+ if fetchLimit == 0 {
+ return nil, "", fmt.Errorf("limit is too large")
+ }
@@
if len(*addressTxs) > int(*limit) {
page := (*addressTxs)[:int(*limit)]
lastAddressTx := page[len(page)-1]Also applies to: 186-189
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@pkgs/database/queries_address.go` around lines 136 - 141, Validate and
sanitize the incoming limit before doing any arithmetic or slice indexing:
ensure that if limit is nil you set it to &defaultLimit, then clamp *limit to a
safe positive range (e.g., treat non-positive values as default and cap very
large values to a sensible maxLimit) before computing fetchLimit so the addition
cannot overflow; also guard any access like page[len(page)-1] by checking
len(page) > 0 (or only perform that logic when you successfully fetched at least
one row) so you never index an empty slice. Reference: variables limit,
defaultLimit, fetchLimit and the code that reads page[len(page)-1].
Comment on lines
+219
to
+220
| ORDER BY tx.timestamp DESC | ||
| LIMIT $3 OFFSET $4 |
There was a problem hiding this comment.
Use a stable secondary sort key for OFFSET pagination.
ORDER BY tx.timestamp DESC alone is unstable for rows sharing the same timestamp, which can duplicate/skip records across pages.
📌 Proposed fix
- ORDER BY tx.timestamp DESC
+ ORDER BY tx.timestamp DESC, tx.tx_hash DESC
LIMIT $3 OFFSET $4📝 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
| ORDER BY tx.timestamp DESC | |
| LIMIT $3 OFFSET $4 | |
| ORDER BY tx.timestamp DESC, tx.tx_hash DESC | |
| LIMIT $3 OFFSET $4 |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@pkgs/database/queries_address.go` around lines 219 - 220, The ORDER BY clause
using only tx.timestamp is unstable for OFFSET pagination; update the query that
contains "ORDER BY tx.timestamp DESC" (in pkgs/database/queries_address.go) to
add a deterministic secondary sort key such as a unique transaction identifier
(e.g., tx.id DESC or tx.hash DESC) so rows with identical timestamps have a
stable order; ensure the same secondary key is used wherever pagination is
applied so LIMIT $3 OFFSET $4 returns consistent, non-duplicating pages.
Comment on lines
+95
to
+110
| query := ` | ||
| SELECT | ||
| encode(tx.tx_hash, 'base64') AS tx_hash, | ||
| tx.timestamp, | ||
| tx.block_height, | ||
| tx.tx_events, | ||
| tx.tx_events_compressed, | ||
| tx.compression_on, | ||
| tx.gas_used, | ||
| tx.gas_wanted, | ||
| tx.fee, | ||
| tx.msg_types | ||
| FROM transaction_general tx | ||
| WHERE tx.chain_name = $1 | ||
| ORDER BY tx.timestamp DESC | ||
| LIMIT $2 |
There was a problem hiding this comment.
Align first-page ordering with cursor ordering.
Line 109 orders only by tx.timestamp, but cursor pagination (Line 240 and Line 241) uses (tx.timestamp, tx.tx_hash). With duplicate timestamps, page boundaries can skip/duplicate rows between the initial page and subsequent cursor pages.
Suggested fix
FROM transaction_general tx
WHERE tx.chain_name = $1
- ORDER BY tx.timestamp DESC
+ ORDER BY tx.timestamp DESC, tx.tx_hash DESC
LIMIT $2Also applies to: 214-216, 240-242
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@pkgs/database/queries_tx.go` around lines 95 - 110, The initial page query
orders only by tx.timestamp which can cause skips/duplicates versus cursor
pagination that orders by (tx.timestamp, tx.tx_hash); update the ORDER BY in the
first-page query (the query using transaction_general tx and selecting
encode(tx.tx_hash, 'base64') AS tx_hash) to ORDER BY tx.timestamp DESC,
tx.tx_hash DESC (matching the cursor ordering/direction), and make the same
change to the other similar ORDER BY clauses referenced around lines 214-216 and
240-242 so all pages use the identical (tx.timestamp, tx.tx_hash) tie-breaker
ordering.
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
API Changes
Documentation
Infrastructure