chore: review feedback improvements

Co-authored-by: Daniel Olshansky <[email protected]>

pkg/client/interface.go

@@ -363,7 +363,7 @@ type BankQueryClient interface {
 
 // QueryCache is a key/value store style interface for a cache of a single type.
 // It is intended to be used to cache query responses (or derivatives thereof),
-// where each key uniquely indexes its most recent value.
+// where each key uniquely indexes the most recent query response.
 type QueryCache[T any] interface {
 	Get(key string) (T, error)
 	Set(key string, value T) error

pkg/client/query/cache/config.go

@@ -13,8 +13,8 @@ const (
 	LeastFrequentlyUsed
 )
 
-// queryCacheConfig is the configuration for query caches. It is intended to be
-// configured via QueryCacheOptionFn functions.
+// queryCacheConfig is the configuration for query caches.
+// It is intended to be configured via QueryCacheOptionFn functions.
 type queryCacheConfig struct {
 	// MaxKeys is the maximum number of items (key/value pairs) the cache can
 	// hold before it starts evicting.
@@ -24,8 +24,8 @@ type queryCacheConfig struct {
 	// MAY not be evicted but SHOULD not be considered as cache hits.
 	TTL time.Duration
 
-	// historical determines whether the cache will cache a single value for each
-	// key (false), or whether it will cache a history of values for each key (true).
+	// historical determines whether each key will point to a single values (false)
+	// or a history (i.e. reverse chronological list) of values (true).
 	historical bool
 	// pruneOlderThan is the number of past blocks for which to keep historical
 	// values. If 0, no historical pruning is performed. It only applies when

pkg/client/query/cache/memory.go

@@ -16,7 +16,8 @@ var (
 	DefaultQueryCacheConfig = queryCacheConfig{
 		EvictionPolicy: FirstInFirstOut,
 		// TODO_MAINNET(@bryanchriswhite): Consider how we can "guarantee" good
-		// alignment between the TTL and the block production rate.
+		// alignment between the TTL and the block production rate,
+		// by accessing onchain block times directly.
 		TTL: time.Minute,
 	}
 )
@@ -71,7 +72,7 @@ func NewInMemoryCache[T any](opts ...QueryCacheOptionFn) *InMemoryCache[T] {
 // Get retrieves the value from the cache with the given key. If the cache is
 // configured for historical mode, it will return the value at the latest **known**
 // height, which is only updated on calls to SetAtHeight, and therefore is not
-// guaranteed to be the current height.
+// guaranteed to be the current height w.r.t the blockchain.
 func (c *InMemoryCache[T]) Get(key string) (T, error) {
 	if c.config.historical {
 		return c.GetAtHeight(key, c.latestHeight.Load())
@@ -88,7 +89,9 @@ func (c *InMemoryCache[T]) Get(key string) (T, error) {
 	}
 
 	cItem := item.(cacheItem[T])
-	if c.config.TTL > 0 && time.Since(cItem.timestamp) > c.config.TTL {
+	isTTLEnabled := c.config.TTL > 0
+	isCacheItemExpired := time.Since(cItem.timestamp) > c.config.TTL
+	if isTTLEnabled && isCacheItemExpired {
 		// DEV_NOTE: Intentionally not pruning here to improve concurrent speed;
 		// otherwise, the read lock would be insufficient. The value will be
 		// overwritten by the next call to Set().