add caching to ETHResolver #87

volodymyr-basiuk · 2025-06-25T13:07:27Z

Adding caching to ETH Resolver similar to iden3/js-iden3-auth#105

Usage:

stateCache := cache.NewInMemoryCache[state.ResolvedState](10000, time.Hour)
 resolver := state.NewETHResolver("", "", &state.ResolverOptions{
   	StateCacheOptions: &state.CacheOptions{
   		Cache: stateCache,
   	},
   })

ilya-korotya · 2025-06-27T08:56:04Z

cache/cache.go

+)
+
+// ICache is a generic interface for a cache implementation.
+type ICache[T any] interface {


I<Name> is not go style for interface naming, consider use Cache

ilya-korotya · 2025-06-27T08:58:09Z

cache/cache.go

+}
+
+// NewInMemoryCache creates a new in-memory cache with the specified size and default TTL.
+func NewInMemoryCache[T any](size int64, defaultTTL time.Duration) ICache[T] {


Methods should return implementation of an interface not interface type.

ilya-korotya · 2025-06-27T09:02:51Z

cache/cache.go

+}
+
+// Set adds an item to the cache with a specified key and value and optional ttl.
+func (c *inMemoryCache[T]) Set(key string, value T, ttl ...time.Duration) {


Use golang optionals patter: https://hayageek.com/optional-parameters-in-go/

ilya-korotya · 2025-06-27T09:05:00Z

cache/cache_test.go

+
+	time.Sleep(200 * time.Millisecond)
+
+	_, ok := c.Get("short")


Use golang assert/require library:

require.False(t, ok)

The pkg is already imported: https://github.com/iden3/go-iden3-auth/blob/main/go.mod#L21

ilya-korotya · 2025-06-27T09:06:23Z

cache/cache_test.go

Rewrite all tests to use the pkg https://github.com/iden3/go-iden3-auth/blob/main/go.mod#L21

ilya-korotya · 2025-06-27T09:09:12Z

state/resolver-cache_test.go

Same. Please use assert library for these unit tests.

ilya-korotya · 2025-06-27T09:13:58Z

state/resolver.go

-// NewETHResolver create ETH resolver for state.
-func NewETHResolver(url, contract string) *ETHResolver {
+// NewETHResolverWithErr create ETH resolver for state or return error.
+func NewETHResolverWithErr(url, contract string, opts *ResolverOptions) (*ETHResolver, error) {


Let's do the code more golang style.

Rename the function to NewETHResolverWithOpts;

Change function's signature to: NewETHResolverWithOpts(url, contract string, opts ...Options)

Use golang options patter to pass options.

ilya-korotya · 2025-06-27T09:19:36Z

state/resolver.go

+// Close closes the ETHResolver and its underlying client.
+func (r *ETHResolver) Close() error {
+	if r.ethClient != nil {
+		r.ethClient.Close()


If the close function returns an error try to handle the error with deffer or just print some log

No return value from client close function

ilya-korotya · 2025-07-02T11:34:48Z

cache/cache.go

 // NewInMemoryCache creates a new in-memory cache with the specified size and default TTL.
-func NewInMemoryCache[T any](size int64, defaultTTL time.Duration) ICache[T] {
+func NewInMemoryCache[T any](size int64, defaultTTL time.Duration) *InMemoryCache[T] {


Is possible to pass 0?

ilya-korotya · 2025-07-02T11:37:49Z

cache/cache_test.go

-	if !ok || val != "bar" {
-		t.Errorf("expected 'bar', got '%v', ok: %v", val, ok)
-	}
+	assert.True(t, ok, "expected 'foo' to be set")


user require instead of asset. No sense continue testing if first test was failed.

ilya-korotya · 2025-07-02T11:40:27Z

state/resolver.go

-// ResolverOptions is the full config for ETHResolver
-type ResolverOptions struct {
+// ResolverOptions is a function that modifies the ResolverConfig.
+type ResolverOptions func(*ResolverConfig)


No need add prefix Resolver. Because you will have prefix resolver when you will use the pacakge
For example

import resolver resolver.Config

Bad example

resolver.ResolverConfig

Copilot

Pull Request Overview

Adds in-memory caching support to ETHResolver to avoid redundant blockchain calls and centralizes default TTL configurations.

Introduces CacheOptions and ResolverOptions for configurable state and root caches.
Modifies ETHResolver to check cache before on-chain resolution and store results with appropriate TTL.
Defines default TTL constants and adds unit tests for both resolver and generic cache behavior.

Reviewed Changes

Copilot reviewed 7 out of 8 changed files in this pull request and generated 1 comment.

Show a summary per file

File	Description
state/resolver.go	Added cache config types, integrated caching into resolve methods, and a `Close` method.
state/resolver-cache_test.go	New tests verifying that `Resolve` and `ResolveGlobalRoot` use the cache when present.
pubsignals/verifyopts.go	Switched hardcoded verify delays to use shared constants.
constants/constants.go	Defined default cache TTL constants and grouping types.
cache/cache.go	Introduced `Cache` interface and `InMemoryCache` using `ccache`.
cache/cache_test.go	Comprehensive tests for set/get, eviction, and TTL behavior.
go.mod	Added `github.com/karlseguin/ccache/v3` dependency.

Comments suppressed due to low confidence (2)

state/resolver-cache_test.go:41

[nitpick] The test name refers to Gist but is exercising ResolveGlobalRoot. For clarity and consistency, rename it to TestResolveGlobalRoot_UsesCacheIfPresent.

func TestGistResolve_UsesCacheIfPresent(t *testing.T) {

state/resolver.go:39

[nitpick] The comment implies this function generates default values, but it actually applies the provided CacheOptions. Update the comment to reflect that it registers custom options.

// WithStateCacheOptions creates a new ResolverConfig with default values for state cache options.

state/resolver.go

ilya-korotya · 2025-07-02T11:43:34Z

Use required instead of assert for all unit tests where no sense to continue testing after an error.

volodymyr-basiuk added 2 commits June 25, 2025 16:07

add caching to ETHResolver

532bd77

fix linter

1af21b7

volodymyr-basiuk requested review from Copilot, vmidyllic, ilya-korotya and Kolezhniuk June 25, 2025 13:14

This comment was marked as outdated.

Sign in to view

volodymyr-basiuk added 2 commits June 25, 2025 16:24

fix nil opts

77c3cb3

reuse stateCaller

8db94f2

volodymyr-basiuk requested a review from Copilot June 25, 2025 13:32

This comment was marked as outdated.

Sign in to view

volodymyr-basiuk added 2 commits June 25, 2025 16:45

fix unit

139ef36

fix comment

2495929

vmidyllic previously approved these changes Jun 26, 2025

View reviewed changes

Kolezhniuk previously approved these changes Jun 27, 2025

View reviewed changes

ilya-korotya previously approved these changes Jun 27, 2025

View reviewed changes

resolve comments (go-style ..opts and ussing assert)

5ab2214

volodymyr-basiuk dismissed stale reviews from ilya-korotya, Kolezhniuk, and vmidyllic via 5ab2214 June 27, 2025 10:23

volodymyr-basiuk requested a review from ilya-korotya June 27, 2025 10:24

ilya-korotya previously approved these changes Jul 2, 2025

View reviewed changes

ilya-korotya requested a review from Copilot July 2, 2025 11:41

Copilot AI reviewed Jul 2, 2025

View reviewed changes

state/resolver.go Show resolved Hide resolved

resolve comments

767e381

volodymyr-basiuk dismissed ilya-korotya’s stale review via 767e381 July 2, 2025 11:48

volodymyr-basiuk requested review from ilya-korotya and vmidyllic July 2, 2025 12:09

ilya-korotya approved these changes Jul 2, 2025

View reviewed changes

volodymyr-basiuk merged commit 3827619 into main Jul 2, 2025
3 checks passed

volodymyr-basiuk deleted the feat/caching branch July 2, 2025 14:56

add caching to ETHResolver #87

add caching to ETHResolver #87

Uh oh!

Conversation

volodymyr-basiuk commented Jun 25, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

This comment was marked as outdated.

Uh oh!

This comment was marked as outdated.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Pull Request Overview

Reviewed Changes

Uh oh!

Uh oh!

ilya-korotya commented Jul 2, 2025

Uh oh!

Uh oh!

Uh oh!

volodymyr-basiuk commented Jun 25, 2025 •

edited

Loading