Repository navigation
fix(sdk,wallet-toolbox): honor identity overlay matching in discoverByAttributes - #617
Conversation
filterCertificatesByAttributes re-binds overlay results to the query,
but read `any` as a literal field name. The identity overlay treats
`any` as an all-fields search, so every discoverByAttributes({ any })
hit was dropped. Mirror overlay semantics: `any` matches when a
decrypted field contains a search term; named fields use the overlay
fuzzy regex; userName stays exact.
Patch-bump wallet-toolbox, -client, -mobile to 2.14.1.
Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
…o fix/identity-any-attribute-filter # Conflicts: # docs/packages/wallet/wallet-toolbox-client.md # docs/packages/wallet/wallet-toolbox-mobile.md # docs/packages/wallet/wallet-toolbox.md # docs/reference/package-api-migrations.md
Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
…o fix/identity-any-attribute-filter
2.14.1 was published from main without the any-attribute fix; move the changelog entry and release notes to a 2.14.2 patch. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
ty-everett
left a comment
ty-everett
left a comment
There was a problem hiding this comment.
Reviewed cda68897d182b1a3b9dc4c705bbc80a4bedc51d0 against current main. The ordinary any: 'deggen' lookup now survives the client filter, and the named-field fuzzy matching agrees with the overlay for the common path. The complete workspace builds locally; all 9 identity verification/filter tests pass. Browser/mobile hosted jobs and CodeQL are green; the final aggregate gate must still be checked before merge.
There are remaining search-contract gaps worth addressing or explicitly recording before calling this a full mirror of overlay matching:
IdentityStorageManager.findByAttributeuses MongoDB$textforanyqueries longer than two characters. Its default text index handles diacritics and quoted phrases, whereas the client uses literal whitespace-separated substrings. Running the compiled candidate with ordinary fixtures confirms that{ any: 'jose' }dropsname: 'José', and{ any: '"Alice Smith"' }dropsname: 'Alice Smith'. MongoDB documents these text-index semantics here: https://www.mongodb.com/docs/manual/core/indexes/index-types/index-text/text-index-properties/ . These were already unavailable through the old clientanyfilter, so they are residual compatibility gaps rather than a regression in the simple lookup this PR fixes.- The overlay ignores blank named fields, but the client still requires them to match.
{ name: 'ali', company: ' ' }withname: 'Alice Smith'is selected by the overlay's named-field query and dropped by the client. Please align the ignored-field handling while keeping an all-empty query non-matching.
Coordination: BotBoard #615 retains downstream consumer work. BSV Desktop #99 is handed to sirdeggen and will not be merged or released by this agent. Downstream Desktop #58 is held for the outcome of this review and the upcoming package release, so we can consume the correction together with the already published action-result fix. I have made no implementation changes to your branch and no paid test calls.
Codecov Report✅ All modified and coverable lines are covered by tests. 📢 Thoughts on this report? Let us know! |
Address review: ignore blank named attributes (all-blank still matches nothing), and make `any` diacritic-insensitive with quoted-phrase and -term exclusion semantics over the overlay's searchable fields. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
ty-everett
left a comment
ty-everett
left a comment
There was a problem hiding this comment.
The broader integration check found a release-relevant gap beyond the matching nuances above: changing Toolbox alone leaves normal BRC100 callers broken.
Using this repository's pinned, genuinely signed identity fixture (identityVerification.fixtures.ts), real Wallet + WalletPermissionsManager, SDK 2.8.6 WalletClient and WalletWireProcessor/Transceiver, with all network access disabled:
| Lookup | Direct permission manager | SDK WalletClient | Binary BRC100 |
|---|---|---|---|
| Identity key | 1 verified certificate | passes | passes |
Exact name: 'Alice' |
1 verified certificate | passes | passes |
any: 'alice' |
1 verified certificate | rejects decryptedFields.any |
same rejection |
Fuzzy name: 'ali' |
1 verified certificate | rejects decryptedFields.name |
same rejection |
packages/sdk/src/wallet/WalletResultValidation.ts:1711-1728 still binds every requested key to an exact decrypted-field value, including treating any as a literal field name. Please coordinate the SDK result-matching correction with this Toolbox change and include this actual signed-certificate-to-wire path in the tests. Do not remove certificate/identity verification or make a blanket result-validation bypass. SDK consumers must receive compatible matching semantics too; otherwise the package-only unit tests pass while the end-to-end lookup still fails.
Separately, the current explicitly installed ContactSource path returns a local record directly but WalletClient/binary validation rejects its synthetic non-certificate shape. That needs an explicit local-contact versus signed-certificate design/consumer review; I am not proposing fake certificate fields or fabricated trust values to make it serialize. I am checking whether current reference/downstream wallets install this source before classifying its release impact.
No live wallets, payments, secrets or production data were used. Exact reviewed head remains cda68897d182b1a3b9dc4c705bbc80a4bedc51d0; all hosted checks now pass. This is an integration correctness finding, not a CI failure. Coordination remains on BotBoard #615; no competing implementation or publication is started here.
…ult validation WalletResultValidation bound every requested attribute to an exact decrypted-field value, so WalletClient and binary BRC-100 callers rejected valid overlay results for the all-fields `any` search and fuzzy named-field matches, even after the wallet verified them. Validation now mirrors the overlay/Toolbox contract: `any` text search (case/diacritic-insensitive, quoted phrases, -term exclusions, skipping profilePhoto/icon), fuzzy named fields, exact userName, blank named attributes ignored. Results stay bound to the query they answered. Adds a signed-certificate test through Wallet, WalletPermissionsManager, SDK WalletClient and the binary wire. Patch-bumps @bsv/sdk to 2.8.7. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
sirdeggen
changed the title
any search in attribute filter
There was a problem hiding this comment.
Re-review of exact head 92599aab22dbe123df6eac8e51239be48403fff8: the source-owned pnpm --filter @bsv/sdk build currently fails with TS2345 at WalletResultValidation.ts:1804. decryptedFields is UnknownRecord, while identityAnyMatches requires Record<string, string>. Please carry the validated string-record type through that helper boundary (without weakening validation), then rerun the actual package build, not only test compilation. I am continuing the offline compatibility review and have not edited this implementation. Publication handoff remains pending re-review and all exact-head gates. Coordination: #615; your completed #619 is acknowledged.
Hosted run 36093383383 stopped before builds on six new Sonar findings in the same SDK file: String.raw (1595), cognitive complexity 19 versus 15 (1605), indexed loops (1621/1633/1653), and startsWith (1624). Please address them together with TS2345, retaining the captured-intrinsic protections where needed. These are source/build and quality-gate findings; the source-mapped signed-certificate wire suite itself passes all 18 cases. Full compiler-emitted runtime diagnostics and older-caller checks are continuing; no successful release build or approval is claimed.
stringRecord validates every value as a string but returned UnknownRecord, so the package build (tsc -b) rejected passing decrypted fields to identityAnyMatches (TS2345). Return the validated Record<string, string> instead; validation is unchanged. Also restructure the new identity matching helpers to clear the new Sonar findings (regex exec/while loops instead of simple for loops, String.raw, lower cognitive complexity) and extract the attribute binding into bindDiscoveredAttributes. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
|
ty-everett
left a comment
ty-everett
left a comment
There was a problem hiding this comment.
Re-reviewed c13c146495309c4bdf24582b96e66613f5acc2b8. The TS2345 fix carries the already-validated string-record type without changing runtime validation, and the matching-helper refactor preserves the reviewed semantics. Full workspace build, all four changed-package packed consumers (including ESM/CJS export/type checks), browser/Vite/esbuild, and mobile/Metro/Hermes contracts pass locally. The focused source suite passes 28 tests. Independent signed-identity checks pass all 36 candidate direct/client/binary cases plus 72 SDK 2.4.0/2.5.0/2.7.1 caller cases. All 48 action/accounting wire cases and eight real-key BRC29 payment/refund checks also pass, with no network or live funds.
The previously recorded 2.8.1/2.8.2/2.8.6 client-validator failures remain affected-client upgrade guidance, not a new BRC100 format migration. ContactSource remains outside this correction. No additional blocking source issue found in this review. Hosted CI must finish successfully before merge; CodeQL and Conformance are already green. Please confirm completion of the remaining independent review and the merge/publication handoff on #615 so protected npm publication and downstream integration have one owner.
2 participants
Problem
In
@bsv/wallet-toolbox-mobile2.14.0,discoverByAttributes({ attributes: { any: 'deggen' } })returned nothing even though the overlay found 7 certificates. Two layers re-check overlay results against the query, and both got the matching wrong:filterCertificatesByAttributesreadanyas a field name.WalletResultValidation, which applies toWalletClientand binary BRC-100 callers, required an exact value for every requested key. That rejectedanysearches and fuzzy matches such asname: 'ali'.Fix
Both layers now use the identity overlay's (
IdentityStorageManager.findByAttribute) matching rules:anyworks like the overlay's MongoDB text search: case and accents are ignored, quoted phrases must match,-termexcludes, andprofilePhoto/iconare not searched. Queries of 2 characters use the fuzzy regex, and anything shorter matches nothing.userNamemust match exactly.Every result is still bound to the query it answered, and certificate and identity verification are unchanged. Word stemming is not reproduced, and this is noted in the changelogs.
Excluded: the
ContactSourcelocal-record shape. It is a separate design question, and no current wallet installs that source (per review, BotBoard #619).Release
@bsv/sdk2.8.6 → 2.8.7@bsv/wallet-toolbox,-client,-mobile→ 2.14.2 (2.14.1 was published without this fix, Record verified Toolbox 2.14.1 publication #618)Changelogs, release notes, health baselines and generated docs are updated. The Toolbox peer floor stays
^2.8.0. Apps that calldiscoverByAttributesthroughWalletClientneed SDK 2.8.7.Tests
WalletClient/ binary wire, with no network. It covers identity key, exact,any, fuzzy, blank-field and non-matching lookups (18 cases). Against the old SDK validator, 6 of them fail.check-versions,docs:facts:check,format:checkand the repository-health tests all pass.Coordination: BotBoard #619 (implementation) and #615 (review and downstream work).
🤖 Generated with Claude Code