Issue-2063 GetWalletID used to swallow every storage error and return an empty string issue

Signed-off-by: Effi-S <effi.szt@gmail.com>

Author: Effi-S

token/services/identity/driver/storage.go

@@ -83,7 +83,10 @@ type WalletID = string
 //
 //go:generate counterfeiter -o mock/wss.go -fake-name WalletStoreService . WalletStoreService
 type WalletStoreService interface {
-	// GetWalletID fetches a walletID that is bound to the identity passed
+	// GetWalletID fetches a walletID that is bound to the identity passed.
+	// It returns an empty WalletID and no error if the identity has no stored
+	// binding; a non-nil error indicates a genuine storage failure and must not
+	// be interpreted by callers as "no binding".
 	GetWalletID(ctx context.Context, identity token.Identity, roleID int) (WalletID, error)
 	// GetWalletIDs fetches all walletID's that have been stored so far without duplicates
 	GetWalletIDs(ctx context.Context, roleID int) ([]WalletID, error)

token/services/identity/role/registry.go

@@ -22,6 +22,87 @@ type WalletFactory interface {
 	NewWallet(ctx context.Context, id idriver.WalletID, role idriver.IdentityRoleType, is IdentitySupport, info idriver.IdentityInfo) (driver.Wallet, error)
 }
 
+// WalletIDStatus classifies the outcome of resolving an identity to the wallet id
+// bound to it. It exists so callers branch on an explicit, named state instead of
+// re-deriving intent from the ambiguous "(string, error)" shape — where ("", nil),
+// ("", err) and ("id", nil) each mean something different and the difference is easy
+// to get wrong (see issue #2063).
+type WalletIDStatus int
+
+const (
+	// WalletIDUnknown is the zero value and never returned by GetWalletID. It is the
+	// guard against a WalletIDResolution constructed without going through GetWalletID
+	// (a mock, or a future constructor that forgets to set Status): because callers act
+	// on a fallthrough only when authoritative() is true — Bound or Unbound — this zero
+	// value is treated as a lookup failure, not as a safe "no binding" that would create
+	// a duplicate wallet.
+	WalletIDUnknown WalletIDStatus = iota
+	// WalletIDBound means storage holds a wallet id for the identity. WalletID is set.
+	WalletIDBound
+	// WalletIDUnbound means storage answered authoritatively that the identity has no
+	// wallet binding. This is a definitive, successful miss: it is safe to fall through
+	// to the next resolution step and, ultimately, to create a wallet.
+	WalletIDUnbound
+	// WalletIDFailed means the storage lookup itself failed (timeout, connection reset,
+	// ...), so whether a binding exists is UNKNOWN. Err carries the cause. Callers MUST
+	// NOT treat this as WalletIDUnbound: doing so lets a transient blip masquerade as an
+	// unregistered identity and triggers the creation of a duplicate wallet.
+	WalletIDFailed
+)
+
+// WalletIDResolution is the explicit result of resolving an identity to its bound
+// wallet id. It is the single shared value every wallet-lookup fallback branches on,
+// so the "not found" vs "could not check" distinction is decided once (in GetWalletID)
+// rather than re-inferred at each call site.
+type WalletIDResolution struct {
+	// Status is the outcome of the lookup. Always inspect it via Bound/Unbound/Failed
+	// before reading the other fields, and branch exhaustively: any status that is
+	// neither Bound nor Unbound (Failed, or the WalletIDUnknown zero value) is not
+	// authoritative and must abort the lookup rather than fall through to creation.
+	Status WalletIDStatus
+	// WalletID is the bound wallet id; meaningful only when Status is WalletIDBound.
+	WalletID idriver.WalletID
+	// Err is the underlying storage failure; set only when Status is WalletIDFailed.
+	Err error
+}
+
+// Bound reports whether the identity has a wallet id bound to it.
+func (r WalletIDResolution) Bound() bool { return r.Status == WalletIDBound }
+
+// Unbound reports whether storage answered authoritatively that the identity has no
+// wallet binding. This is the ONLY non-Bound state that a caller may act on by falling
+// through to the next resolution step and, ultimately, wallet creation. Every other
+// state — Failed, or the zero value produced by a WalletIDResolution built without
+// going through GetWalletID — leaves the binding unknown and must abort the lookup.
+func (r WalletIDResolution) Unbound() bool { return r.Status == WalletIDUnbound }
+
+// Failed reports whether the storage lookup failed, leaving the binding unknown.
+// A failed resolution must abort the enclosing lookup, never fall through to creation.
+func (r WalletIDResolution) Failed() bool { return r.Status == WalletIDFailed }
+
+// authoritative reports whether the resolution definitively answers whether a wallet is
+// bound — i.e. it came back from GetWalletID as Bound or Unbound. Any other status
+// (Failed, or the zero-value WalletIDUnknown of a resolution built outside GetWalletID)
+// is non-authoritative: the binding is unknown and the caller MUST abort rather than
+// fall through to wallet creation. This is the guard the WalletIDUnknown zero value was
+// introduced to provide.
+func (r WalletIDResolution) authoritative() bool { return r.Bound() || r.Unbound() }
+
+// abortError returns the error a non-authoritative resolution must abort a wallet lookup
+// with. It preserves the storage cause for a WalletIDFailed and synthesizes one for a
+// zero-value / unknown resolution (whose Err is nil), so an unknown status can never
+// collapse into a nil error — via errors.WithMessagef(nil, ...) returning nil — and be
+// silently mistaken for a successful lookup. It must only be called once Bound and
+// Unbound have been ruled out (i.e. authoritative() is false).
+func (r WalletIDResolution) abortError(id driver.WalletLookupID) error {
+	cause := r.Err
+	if cause == nil {
+		cause = errors.Errorf("non-authoritative wallet id resolution status [%d]", r.Status)
+	}
+
+	return errors.WithMessagef(cause, "failed to lookup wallet [%s]", id)
+}
+
 // Registry manages wallets whose long-term identities have a given role.
 //
 // Concurrency and invariants:
@@ -82,11 +163,17 @@ func (r *Registry) Lookup(ctx context.Context, id driver.WalletLookupID) (driver
 		if ok {
 			r.Logger.DebugfContext(ctx, "lookup failed, check if there is a wallet for identity [%s]", passedIdentity)
 			// is this identity registered
-			wID, err := r.GetWalletID(ctx, passedIdentity)
-			if err == nil && len(wID) != 0 {
-				r.Logger.DebugfContext(ctx, "lookup failed, there is a wallet for identity [%s]: [%s]", passedIdentity, wID)
+			res := r.GetWalletID(ctx, passedIdentity)
+			if !res.authoritative() {
+				// A storage failure — or a resolution that never went through GetWalletID —
+				// leaves the binding unknown; it must not be treated as "not registered", or
+				// a transient blip would fall through to wallet creation and duplicate state.
+				return nil, nil, "", res.abortError(id)
+			}
+			if res.Bound() {
+				r.Logger.DebugfContext(ctx, "lookup failed, there is a wallet for identity [%s]: [%s]", passedIdentity, res.WalletID)
 				// we got a hit
-				walletID = wID
+				walletID = res.WalletID
 				ident = passedIdentity
 				fail = false
 			}
@@ -111,36 +198,48 @@ func (r *Registry) Lookup(ctx context.Context, id driver.WalletLookupID) (driver
 	if ok {
 		r.Logger.DebugfContext(ctx, "no wallet found, check if there is a wallet for identity [%s]", passedIdentity)
 		// is this identity registered
-		passedWalletID, err := r.GetWalletID(ctx, passedIdentity)
-		if err == nil && len(passedWalletID) != 0 {
-			r.Logger.DebugfContext(ctx, "no wallet found, there is a wallet for identity [%s]: [%s]", passedIdentity, passedWalletID)
+		res := r.GetWalletID(ctx, passedIdentity)
+		if !res.authoritative() {
+			// A storage failure — or a resolution that never went through GetWalletID —
+			// leaves the binding unknown; it must not be treated as "not registered", or
+			// a transient blip would fall through to wallet creation and duplicate state.
+			return nil, nil, "", res.abortError(id)
+		}
+		if res.Bound() {
+			r.Logger.DebugfContext(ctx, "no wallet found, there is a wallet for identity [%s]: [%s]", passedIdentity, res.WalletID)
 			// we got a hit
 			r.WalletMu.RLock()
-			walletEntry, ok = r.Wallets[passedWalletID]
+			walletEntry, ok = r.Wallets[res.WalletID]
 			r.WalletMu.RUnlock()
 			if ok {
-				return walletEntry, nil, passedWalletID, nil
+				return walletEntry, nil, res.WalletID, nil
 			}
-			r.Logger.DebugfContext(ctx, "no wallet found, there is a wallet for identity [%s]: [%s] but it has not been recreated yet", passedIdentity, passedWalletID)
+			r.Logger.DebugfContext(ctx, "no wallet found, there is a wallet for identity [%s]: [%s] but it has not been recreated yet", passedIdentity, res.WalletID)
 		}
-		walletIdentifiers = append(walletIdentifiers, passedWalletID)
+		walletIdentifiers = append(walletIdentifiers, res.WalletID)
 	}
 
 	r.Logger.DebugfContext(ctx, "no wallet found for [%s] at [%s]", passedIdentity, logging.Prefix(wID))
 	if len(ident) != 0 {
-		identityWID, err := r.GetWalletID(ctx, ident)
-		r.Logger.DebugfContext(ctx, "wallet for identity [%s] -> [%s:%s]", ident, identityWID, err)
-		if err == nil && len(identityWID) != 0 {
+		res := r.GetWalletID(ctx, ident)
+		r.Logger.DebugfContext(ctx, "wallet for identity [%s] -> [%s:%d]", ident, res.WalletID, res.Status)
+		if !res.authoritative() {
+			// A storage failure — or a resolution that never went through GetWalletID —
+			// leaves the binding unknown; it must not be treated as "not registered", or
+			// a transient blip would fall through to wallet creation and duplicate state.
+			return nil, nil, "", res.abortError(id)
+		}
+		if res.Bound() {
 			r.WalletMu.RLock()
-			w, ok := r.Wallets[identityWID]
+			w, ok := r.Wallets[res.WalletID]
 			r.WalletMu.RUnlock()
 			if ok {
-				r.Logger.DebugfContext(ctx, "found wallet [%s:%s:%s:%s]", ident, walletID, w.ID(), identityWID)
+				r.Logger.DebugfContext(ctx, "found wallet [%s:%s:%s:%s]", ident, walletID, w.ID(), res.WalletID)
 
-				return w, nil, identityWID, nil
+				return w, nil, res.WalletID, nil
 			}
 		}
-		walletIdentifiers = append(walletIdentifiers, identityWID)
+		walletIdentifiers = append(walletIdentifiers, res.WalletID)
 	}
 
 	for _, walletIdentifier := range walletIdentifiers {
@@ -233,16 +332,31 @@ func (r *Registry) GetIdentityMetadata(ctx context.Context, identity driver.Iden
 	return json.Unmarshal(raw, &meta)
 }
 
-// GetWalletID returns the wallet identifier bound to the passed identity
-func (r *Registry) GetWalletID(ctx context.Context, identity driver.Identity) (string, error) {
+// GetWalletID resolves the wallet identifier bound to the passed identity.
+//
+// It is the single point that translates the storage layer's (WalletID, error)
+// convention into an explicit WalletIDResolution, so no caller has to re-derive the
+// meaning of ("", nil) vs ("", err). The storage contract reports an unbound identity
+// as ("", nil); a non-nil error is a genuine storage failure (timeout, connection
+// reset, ...) whose result is therefore WalletIDFailed, never WalletIDUnbound. Keeping
+// the two apart here is what prevents a transient blip from looking like an
+// unregistered identity and triggering the creation of a duplicate wallet (issue #2063).
+func (r *Registry) GetWalletID(ctx context.Context, identity driver.Identity) WalletIDResolution {
 	wID, err := r.Storage.GetWalletID(ctx, identity, int(r.Role.ID()))
 	if err != nil {
-		//nolint:nilerr
-		return "", nil
+		return WalletIDResolution{
+			Status: WalletIDFailed,
+			Err:    errors.Wrapf(err, "failed to get wallet id for identity [%s]", identity),
+		}
+	}
+	if len(wID) == 0 {
+		r.Logger.DebugfContext(ctx, "no wallet bound to identity [%s]", identity)
+
+		return WalletIDResolution{Status: WalletIDUnbound}
 	}
 	r.Logger.DebugfContext(ctx, "wallet [%s] is bound to identity [%s]", wID, identity)
 
-	return wID, nil
+	return WalletIDResolution{Status: WalletIDBound, WalletID: wID}
 }
 
 func (r *Registry) WalletByID(ctx context.Context, role idriver.IdentityRoleType, id driver.WalletLookupID) (driver.Wallet, error) {

token/services/identity/role/registry_test.go

@@ -135,17 +135,100 @@ func TestBindIdentityAndContainsAndMetadataAndGetWalletID(t *testing.T) {
 	require.NoError(t, reg.GetIdentityMetadata(ctx, []byte("id"), "w", &meta))
 	require.Equal(t, "v", meta["k"])
 
-	// GetWalletID when storage returns value
+	// GetWalletID when storage returns a bound wallet id.
 	storage.GetWalletIDReturns("w", nil)
-	wid, err := reg.GetWalletID(ctx, []byte("id"))
-	require.NoError(t, err)
-	require.Equal(t, "w", wid)
-
-	// GetWalletID when storage returns error -> suppressed to empty string
+	res := reg.GetWalletID(ctx, []byte("id"))
+	require.Equal(t, role.WalletIDBound, res.Status)
+	require.True(t, res.Bound())
+	require.NoError(t, res.Err)
+	require.Equal(t, "w", res.WalletID)
+
+	// GetWalletID when storage authoritatively reports no binding -> WalletIDUnbound,
+	// a clean miss with no error and no wallet id.
+	storage.GetWalletIDReturns("", nil)
+	resUnbound := reg.GetWalletID(ctx, []byte("id"))
+	require.Equal(t, role.WalletIDUnbound, resUnbound.Status)
+	require.False(t, resUnbound.Bound())
+	require.True(t, resUnbound.Unbound())
+	require.False(t, resUnbound.Failed())
+	require.NoError(t, resUnbound.Err)
+	require.Empty(t, resUnbound.WalletID)
+
+	// GetWalletID when storage fails -> WalletIDFailed with the error preserved, so a
+	// transient storage blip cannot masquerade as "no binding".
 	storage.GetWalletIDReturns("", errors.New("boom"))
-	wid2, err2 := reg.GetWalletID(ctx, []byte("id"))
-	require.NoError(t, err2)
-	require.Empty(t, wid2)
+	resFailed := reg.GetWalletID(ctx, []byte("id"))
+	require.Equal(t, role.WalletIDFailed, resFailed.Status)
+	require.True(t, resFailed.Failed())
+	require.False(t, resFailed.Bound())
+	require.False(t, resFailed.Unbound())
+	require.Error(t, resFailed.Err)
+	require.Empty(t, resFailed.WalletID)
+}
+
+// TestWalletIDResolution_ZeroValueIsNotAuthoritative guards the WalletIDUnknown zero
+// value: a WalletIDResolution built without going through GetWalletID (a mock, or a
+// future constructor that forgets to set Status) must be indistinguishable from a
+// failure at every call site, never from an authoritative "unbound" miss. Callers act
+// on a fallthrough only when a resolution is Bound or Unbound, so the zero value —
+// being neither — is treated as a lookup failure rather than a safe "create a wallet".
+func TestWalletIDResolution_ZeroValueIsNotAuthoritative(t *testing.T) {
+	var zero role.WalletIDResolution
+
+	require.Equal(t, role.WalletIDUnknown, zero.Status)
+	require.False(t, zero.Bound(), "zero value must not read as a bound wallet id")
+	require.False(t, zero.Unbound(), "zero value must not read as an authoritative miss")
+	require.False(t, zero.Failed(), "zero value carries no storage error")
+	require.NoError(t, zero.Err)
+	require.Empty(t, zero.WalletID)
+}
+
+// TestLookup_StorageErrorDoesNotFallThrough pins the fix for #2063: a transient
+// storage error out of GetWalletID must abort Lookup rather than be swallowed as
+// "no binding". Otherwise the fallback chain would fall through to wallet creation
+// and persist a duplicate wallet for an identity that already has one.
+func TestLookup_StorageErrorDoesNotFallThrough(t *testing.T) {
+	t.Run("MapToIdentity fails, view-identity fallback hits storage error", func(t *testing.T) {
+		reg, storage, role, wf := newRegistryWithFakes()
+		ctx := t.Context()
+
+		role.MapToIdentityReturns(nil, "", errors.New("no mapping"))
+		storage.GetWalletIDReturns("", errors.New("transient DB blip"))
+
+		_, _, _, err := reg.Lookup(ctx, []byte("id-with-binding"))
+		require.Error(t, err)
+		require.Equal(t, 0, wf.NewWalletCallCount())
+	})
+
+	t.Run("MapToIdentity succeeds, cache miss, view-identity fallback hits storage error", func(t *testing.T) {
+		reg, storage, role, wf := newRegistryWithFakes()
+		ctx := t.Context()
+
+		// mapping resolves to a wallet id that is not in the cache, so Lookup falls
+		// through to the identity->wallet storage probe, which fails transiently.
+		role.MapToIdentityReturns([]byte("id-with-binding"), "w-not-cached", nil)
+		storage.GetWalletIDReturns("", errors.New("transient DB blip"))
+
+		_, _, _, err := reg.Lookup(ctx, []byte("id-with-binding"))
+		require.Error(t, err)
+		require.Equal(t, 0, wf.NewWalletCallCount())
+	})
+}
+
+// TestWalletByID_StorageErrorDoesNotCreateDuplicate ensures that when the cache
+// misses and the storage lookup fails transiently, WalletByID surfaces the error
+// instead of creating a brand-new wallet (the duplicate-wallet bug of #2063).
+func TestWalletByID_StorageErrorDoesNotCreateDuplicate(t *testing.T) {
+	reg, storage, role, wf := newRegistryWithFakes()
+	ctx := t.Context()
+
+	role.MapToIdentityReturns(nil, "", errors.New("no mapping"))
+	storage.GetWalletIDReturns("", errors.New("transient DB blip"))
+
+	w, err := reg.WalletByID(ctx, 0, []byte("id-with-binding"))
+	require.Error(t, err)
+	require.Nil(t, w)
+	require.Equal(t, 0, wf.NewWalletCallCount())
 }
 
 func TestWalletIDs_MergesRoleAndStorage(t *testing.T) {

token/services/storage/db/kvs/walletdb.go

@@ -85,9 +85,16 @@ func (s *WalletStore) GetWalletID(ctx context.Context, identity driver2.Identity
 	if err != nil {
 		return "", errors.Wrapf(err, "failed to create key")
 	}
+	// kvs.Get returns an error both for a missing key and for a real store failure.
+	// The WalletStoreService contract requires that "no binding" be reported as ("", nil)
+	// so that callers can distinguish it from a transient storage error; probe with Exists
+	// first (as IdentityExists does) and only Get when a binding is present.
+	if !s.kvs.Exists(ctx, k) {
+		return "", nil
+	}
 	var wID storage.WalletID
 	if err := s.kvs.Get(ctx, k, &wID); err != nil {
-		return "", err
+		return "", errors.Wrapf(err, "failed to get wallet id for identity [%v]", idHash)
 	}
 
 	return wID, nil

token/services/storage/db/kvs/walletdb_test.go

@@ -50,3 +50,34 @@ func TestWalletStoreGetConfID(t *testing.T) {
 	require.NoError(t, err)
 	assert.Empty(t, got)
 }
+
+// TestWalletStoreGetWalletID asserts the not-found contract that the role Registry relies on to
+// tell a transient storage error apart from "this identity has no binding": an unbound identity
+// must resolve to ("", nil), and a bound identity must round-trip its wallet id. If GetWalletID
+// returned an error for a missing key (as kvs.Get does), the registry would abort every lookup
+// for a genuinely-unregistered identity instead of creating its wallet.
+func TestWalletStoreGetWalletID(t *testing.T) {
+	backend, err := NewInMemory()
+	require.NoError(t, err)
+	// NewInMemory shares a global in-memory backing store across the package's tests, so use a
+	// tmsID and identities unique to this test to stay isolated from any other stored bindings.
+	tmsID := token.TMSID{Network: "getwalletid", Channel: "getwalletid", Namespace: "getwalletid"}
+	db := NewWalletStore(backend, tmsID)
+	ctx := t.Context()
+
+	// miss: never bound -> ("", nil), NOT an error
+	got, err := db.GetWalletID(ctx, []byte("gwid-grace"), 0)
+	require.NoError(t, err)
+	assert.Empty(t, got)
+
+	// bound under role 0 -> round-trips the wallet id
+	require.NoError(t, db.StoreIdentity(ctx, []byte("gwid-grace"), "eID", "grace_wallet", 0, nil, "conf-1"))
+	got, err = db.GetWalletID(ctx, []byte("gwid-grace"), 0)
+	require.NoError(t, err)
+	assert.Equal(t, "grace_wallet", got)
+
+	// the same identity under a different role is still an independent miss
+	got, err = db.GetWalletID(ctx, []byte("gwid-grace"), 1)
+	require.NoError(t, err)
+	assert.Empty(t, got)
+}