Summary
Registry.GetWalletID swallows every storage error and returns ("", nil), which its callers treat
identically to "no wallet is bound to this identity." A transient storage error (DB blip, timeout)
therefore looks exactly like "not registered yet," and the wallet-lookup fallback chain in Lookup
creates a brand-new wallet and a second identity binding for an identity that already has one.
Where
token/services/identity/role/registry.go:237-246:
func (r *Registry) GetWalletID(ctx context.Context, identity driver.Identity) (string, error) {
wID, err := r.Storage.GetWalletID(ctx, identity, int(r.Role.ID()))
if err != nil {
//nolint:nilerr
return "", nil
}
...
return wID, nil
}
All three call sites treat err == nil && len(wID) == 0 as "not found, try the next fallback":
registry.go:85-92, registry.go:114-117, registry.go:131-133.
Impact
Lookup (registry.go:72-166) is the fallback path used by WalletByID (registry.go:271-281)
whenever the in-memory cache misses. If the storage-level GetWalletID call fails transiently (not
"no row found," but an actual error — timeout, connection reset, etc.), the registry cannot
distinguish that from "this identity has never been registered." It falls through to
WalletFactory.NewWallet (registry.go:293), creating a second wallet and a second identity→wallet
binding for the same identity. This duplicates wallet state and, depending on the wallet
implementation, can duplicate key material bookkeeping.
Reproduction
Not yet committed. Mock idriver.WalletStoreService.GetWalletID to return a non-nil error (e.g. a
simulated transient DB error) on the first call for an identity that has a real binding, and a
successful lookup on a second, independent call. Assert Registry.GetWalletID propagates the error
rather than returning ("", nil), and that WalletByID does not create a duplicate wallet when the
underlying storage error is transient.
Suggested fix
Propagate the error and let callers decide:
func (r *Registry) GetWalletID(ctx context.Context, identity driver.Identity) (string, error) {
wID, err := r.Storage.GetWalletID(ctx, identity, int(r.Role.ID()))
if err != nil {
return "", errors.Wrapf(err, "failed to get wallet id for identity [%s]", identity)
}
...
return wID, nil
}
This requires updating the three call sites in Lookup to distinguish "storage error" (abort/retry)
from "no row found" (continue to the next fallback) — today they're merged into one case. This
should land together with #8 (WalletByID write-lock scope), since both touch the same
creation path and fixing one without the other leaves the duplicate-wallet risk only partially
addressed.
Severity
MEDIUM — requires a transient storage failure to trigger, but results in persisted duplicate wallet
state, which is hard to reconcile after the fact.
Summary
Registry.GetWalletIDswallows every storage error and returns("", nil), which its callers treatidentically to "no wallet is bound to this identity." A transient storage error (DB blip, timeout)
therefore looks exactly like "not registered yet," and the wallet-lookup fallback chain in
Lookupcreates a brand-new wallet and a second identity binding for an identity that already has one.
Where
token/services/identity/role/registry.go:237-246:All three call sites treat
err == nil && len(wID) == 0as "not found, try the next fallback":registry.go:85-92,registry.go:114-117,registry.go:131-133.Impact
Lookup(registry.go:72-166) is the fallback path used byWalletByID(registry.go:271-281)whenever the in-memory cache misses. If the storage-level
GetWalletIDcall fails transiently (not"no row found," but an actual error — timeout, connection reset, etc.), the registry cannot
distinguish that from "this identity has never been registered." It falls through to
WalletFactory.NewWallet(registry.go:293), creating a second wallet and a second identity→walletbinding for the same identity. This duplicates wallet state and, depending on the wallet
implementation, can duplicate key material bookkeeping.
Reproduction
Not yet committed. Mock
idriver.WalletStoreService.GetWalletIDto return a non-nil error (e.g. asimulated transient DB error) on the first call for an identity that has a real binding, and a
successful lookup on a second, independent call. Assert
Registry.GetWalletIDpropagates the errorrather than returning
("", nil), and thatWalletByIDdoes not create a duplicate wallet when theunderlying storage error is transient.
Suggested fix
Propagate the error and let callers decide:
This requires updating the three call sites in
Lookupto distinguish "storage error" (abort/retry)from "no row found" (continue to the next fallback) — today they're merged into one case. This
should land together with #8 (
WalletByIDwrite-lock scope), since both touch the samecreation path and fixing one without the other leaves the duplicate-wallet risk only partially
addressed.
Severity
MEDIUM — requires a transient storage failure to trigger, but results in persisted duplicate wallet
state, which is hard to reconcile after the fact.