Conversation
AkramBitar
added
bug
AkramBitar
marked this pull request as draft
September 22, 2026 21:26
Effi-S
self-requested a review
September 23, 2026 08:35
Effi-S
approved these changes
Sep 23, 2026
Effi-S
marked this pull request as ready for review
September 23, 2026 09:28
AkramBitar
force-pushed
the
2042
branch
3 times, most recently
from
September 23, 2026 12:19
0346703 to
89a0635
Compare
Effi-S
self-requested a review
September 23, 2026 12:28
AkramBitar
added a commit
that referenced
this pull request
Sep 23, 2026
Adopt the WalletStoreService.IdentityExists signature introduced in #2409, so a failed lookup is no longer indistinguishable from "the binding does not exist". Both implementations are updated. The KVS store previously swallowed the composite-key error and answered false; it now wraps it. Only key construction can be reported that way, because this backend's Exists is FSC's GetExisting, which drops the underlying store error - the same caveat already documented on GetWalletID. Registry.ContainsIdentity cannot grow an error return: it backs driver.Wallet.Contains, which returns a plain bool. It therefore logs the failure before reporting false, so a storage problem is at least visible. This duplicates the IdentityExists slice of #2409 verbatim so the two branches converge without conflict; whichever lands second drops the hunks as already applied. Signed-off-by: AkramBitar <akram@il.ibm.com>
Effi-S
requested changes
Sep 28, 2026
Effi-S
requested changes
Sep 29, 2026
Effi-S
approved these changes
Sep 29, 2026
Fixes a batch of medium/low findings from a bug-hunting review of the shared SQL storage layer (token/services/storage/db/sql/common). Correctness - keystore.Put() reported success on a real value conflict: it wrapped an already-nil error, and wrapping nil yields nil. - Foreign-key violations are detected from the code the driver reports through its own typed error (Postgres 23503, SQLITE_CONSTRAINT_FOREIGNKEY) rather than from the message text, whose wording is not part of any driver's contract and has changed across releases. A driver exposing neither accessor is deliberately left unclassified, which is the conservative branch in every caller. - IdentityExists returns (bool, error), so a failed lookup no longer reads as "not found". - tokenlock.Lock returned nil for any error that was not a unique-key violation, telling the caller it held a lock it did not. Performance - SetSpendableBySupportedTokenFormats no longer rewrites the whole Tokens table on every reconciliation; both updates are predicated on the flag actually having to change. This also fixes a latent bug: with no supported format, every token was marked spendable. Robustness - The prepared-statement cache evicts a statement that fails at execute time, so the next call re-prepares instead of degrading to the unprepared path forever. Eviction is narrow on purpose: an ordinary failure such as a constraint violation leaves a perfectly valid statement, and a permanent failure unrelated to validity would add a DEALLOCATE to every call. Only the two PostgreSQL states that mean the statement itself is gone qualify; SQLITE_SCHEMA is left out because modernc.org/sqlite prepares through sqlite3_prepare_v2 and SQLite re-prepares transparently instead of returning the code, so no real backend can reach such a branch. Cleanup - Use the FSC errors package instead of fmt.Errorf, wrap or log previously raw or discarded errors, and delete the callerless legacy query builder in query.go. Also adds a cond.Not combinator, regression tests for each behavioural fix, real-driver dbtest cases for the keystore conflict and the foreign-key mapping, and a "Store Contract Notes" section in docs/development/storage.md. The foreign-key code table is local because FSC's per-driver ErrorMapper, which these stores already inject to recognise UniqueKeyViolation, has no ForeignKeyViolation sentinel to map onto; when it gains one, the predicate should give way to the injected SQLErrorWrapper rather than a second table. Fixes #2042 Signed-off-by: AkramBitar <akram@il.ibm.com>
Effi-S
pushed a commit
that referenced
this pull request
Oct 1, 2026
Adopt the WalletStoreService.IdentityExists signature introduced in #2409, so a failed lookup is no longer indistinguishable from "the binding does not exist". Both implementations are updated. The KVS store previously swallowed the composite-key error and answered false; it now wraps it. Only key construction can be reported that way, because this backend's Exists is FSC's GetExisting, which drops the underlying store error - the same caveat already documented on GetWalletID. Registry.ContainsIdentity cannot grow an error return: it backs driver.Wallet.Contains, which returns a plain bool. It therefore logs the failure before reporting false, so a storage problem is at least visible. This duplicates the IdentityExists slice of #2409 verbatim so the two branches converge without conflict; whichever lands second drops the hunks as already applied. Signed-off-by: AkramBitar <akram@il.ibm.com> Signed-off-by: Effi-S <effi.szt@gmail.com>
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Fixes a batch of medium/low findings from a bug-hunting review of the shared SQL
storage layer (
token/services/storage/db/sql/common).Correctness
keystore.Put()reported success on a real value conflict — it wrapped analready-
nilerror, and wrappingnilyieldsnil.23503,SQLITE_CONSTRAINT_FOREIGNKEY) instead of the message text, whosewording is not part of any driver's contract.
IdentityExistsreturns(bool, error), so a failed lookup no longer reads as"not found".
tokenlock.Lockreturnednilfor any error that wasn't a unique-keyviolation, telling the caller it held a lock it did not.
Performance
SetSpendableBySupportedTokenFormatsno longer rewrites the wholeTokenstable on every reconciliation; both updates are predicated on the flag actually
having to change. This also fixes a latent bug: with no supported format, every
token was marked spendable.
Robustness
the next call re-prepares instead of degrading to the unprepared path forever.
Cleanup
errorspackage instead offmt.Errorf, wrap or log previously rawor discarded errors, and delete the callerless legacy query builder in
query.go.Also adds a
cond.Notcombinator, regression tests for each behavioural fix,real-driver
dbtestcases for the keystore conflict and the foreign-key mapping,and a "Store Contract Notes" section in
docs/development/storage.md.Two findings from the issue needed no change here: the
StorePublicParamscheck-then-insert race was already closed by
ON CONFLICT DO NOTHING, andtablename.go'sfmt.Errorfcalls were already converted, both on main.Fixes #2042