Skip to content

Surface skipped entries in the Vaults query instead of silently omitting them #229

Description

@nullpointer0x00

Summary

The Vaults gRPC handler (keeper/query_server.go:46-58) skips any vault whose GetVault lookup fails or returns nil, logging the error and returning (false, nil) from the pagination callback. The response is a 200/OK with a silently incomplete list, and Pagination.Total still counts the skipped key, so the returned slice can be shorter than Total with no signal to the caller that anything was omitted.

func(key sdk.AccAddress, _ []byte) (include bool, err error) {
    vault, err := k.GetVault(ctx, key)
    if err != nil {
        k.getLogger(ctx).Error("failed to get vault during pagination", "key", key.String(), "error", err)
        return false, nil          // skipped silently
    }
    if vault == nil {
        k.getLogger(ctx).Error("nil vault found during pagination", "key", key.String())
        return false, nil          // skipped silently
    }
    vaults = append(vaults, *vault)
    return true, nil
}

The outer pagination error is propagated as Internal (query_server.go:63-64), so per-entry failures are handled differently from the outer failure within the same function.

Background

This is the deliberate resolution of #129, which fixed a worse problem (the error was previously dropped with _, risking a nil-pointer panic that crashes the node during the query). #129 explicitly offered two options: propagate the error and stop, or log-and-skip. Log-and-skip (fail-open) was chosen. This issue is about making that fail-open behavior transparent rather than silent.

Why not just propagate the error

Returning the error from the callback would make a single undecodable vault break the entire Vaults list endpoint for every caller (fail-closed). For a list/monitoring query, continuing to serve the healthy vaults is usually preferable. The goal here is to keep best-effort results while signaling that entries were omitted.

Proposed change

Add transparency to the existing fail-open behavior:

  • Add a skipped signal to QueryVaultsResponse (e.g. a repeated skipped_addresses field, or at minimum a skipped count), populated with the addresses whose lookup failed or returned nil.
  • Keep serving the successfully-decoded vaults (do not fail the whole query on one bad entry).
  • Optionally reconcile the Pagination.Total/returned-length relationship, or document that Total counts collection keys while skipped entries are reported separately.

Tasks

  • Add a skipped_addresses (or skipped-count) field to QueryVaultsResponse in the proto and regenerate.
  • Populate it from the pagination callback when GetVault errors or returns nil, instead of silently dropping.
  • Add a test with a key in the Vaults collection whose account is missing/undecodable, asserting the healthy vaults are returned and the bad address is reported as skipped.

Related: #129 (the deliberate log-and-skip resolution this builds on).

Metadata

Metadata

Assignees

No one assigned

    Labels

    enhancementNew feature or request

    Type

    No type

    Fields

    No fields configured for issues without a type.

    Projects

    No projects

    Milestone

    No milestone

    Relationships

    None yet

    Development

    No branches or pull requests

    Issue actions