feat(kotlin-sdk): tx-label & asset-lock-kind DAO resolver queries - #4251
feat(kotlin-sdk): tx-label & asset-lock-kind DAO resolver queries#4251bfoss765 wants to merge 2 commits into
Conversation
Read-only DAO queries backing the app's transaction-label resolver. No schema
change — every column read already exists in DashDatabase v9, so no migration
and no new exported schema are required.
TransactionDao:
- transactionKindForTxid(wireTxid) / transactionKindForDisplayTxid(hex):
resolve a tx's transactionTypeKind, used to label the withdraw/unshield
case (AssetUnlock == 7), which has no asset_locks row. Adds a private
displayHexToWireTxid() companion (explorer display hex → wire BLOB PK).
AssetLockDao:
- fundingTypeForTxid(txidDisplayHex): the fundingTypeRaw of the asset lock
whose outPointHex PK is prefixed by the display txid.
Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
|
Warning Review limit reached
Next review available in: 55 minutes Enable usage-based reviews in Billing to review now. Otherwise, wait until the next included review is available. How can I continue?After more reviews become available, a review can be triggered using the To avoid repeated limits, reduce automatic review volume by pausing incremental auto-reviews earlier, using label-based review opt-in, excluding WIP or generated PR titles, or requesting reviews manually when the PR is ready. If your team needs uninterrupted high-volume reviews, an organization admin can enable usage-based reviews. How do review limits work?CodeRabbit enforces per-developer PR review limits for each organization. Most developers receive the normal plan review availability. For paid Pro and Pro+ PR reviews, CodeRabbit uses adaptive limits for sustained high-volume activity. When a developer's recent PR review activity reaches the 95th percentile or higher among CodeRabbit users, additional reviews become available more gradually as earlier reviews age out of the rolling window. Please refer docs for additional details. Review details⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (2)
📝 WalkthroughWalkthroughThe Kotlin SDK adds DAO methods to resolve transaction kinds and asset lock funding types from wire-order or display-order transaction identifiers. ChangesTransaction lookup APIs
Estimated code review effort: 2 (Simple) | ~10 minutes Suggested reviewers: 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
|
✅ Final review complete — no blockers (commit 176f8ed) |
thepastaclaw
left a comment
thepastaclaw
left a comment
There was a problem hiding this comment.
Preliminary review — Codex only
The txid display-to-wire reversal matches the persisted transaction and outpoint encodings, the raw transaction/funding discriminants are consistent with their Rust definitions, and these read-only queries require no Room migration. However, the exact head omits two DAO methods explicitly promised by the PR and required by the stated Android cutover, so this PR does not yet deliver its advertised API surface.
Validated blockers were found in the Codex precheck. Sonnet is deferred until a fresh Codex revalidation clears the blocker gate.
Review provenance
- Codex reviewers:
gpt-5.6-sol— general (completed) - Verifier:
gpt-5.6-sol— verifier - Sonnet: not run (deferred by blocker gate)
🔴 1 blocking | 🟡 2 suggestion(s)
1 additional finding(s) omitted (not in diff).
🤖 Prompt for all review comments with AI agents
These findings are from an automated code review. Verify each finding against the current code and only fix it if needed.
In `packages/kotlin-sdk/sdk/src/main/kotlin/org/dashfoundation/dashsdk/persistence/dao/ShieldedDao.kt`:
- [BLOCKING] packages/kotlin-sdk/sdk/src/main/kotlin/org/dashfoundation/dashsdk/persistence/dao/ShieldedDao.kt:52-56: Add the two promised anchored-note DAO methods
The PR description explicitly includes `getUnspentAnchoredNotesByWallet` and `minUnspentAnchoredBlockHeight` among the four symbols required for the Android cutover, but this exact head proceeds directly from `observeUnspentNotesByWallet` to `upsertNote` without defining either method. Commit `7bc8a845c67c6010e72b171a1d8f70f498220574` in still-open PR #4204 adds those methods, but it is not an ancestor of this head; the corresponding pre-commit `ShieldedDao` section is identical to this PR's base. Merging PR #4251 alone therefore does not provide the advertised API and leaves the cutover with unresolved references. Include the queries here, or explicitly declare and enforce PR #4204 as a merge prerequisite and update this PR's stated deliverables.
In `packages/kotlin-sdk/sdk/src/main/kotlin/org/dashfoundation/dashsdk/persistence/dao/AssetLockDao.kt`:
- [SUGGESTION] packages/kotlin-sdk/sdk/src/main/kotlin/org/dashfoundation/dashsdk/persistence/dao/AssetLockDao.kt:87-91: Validate txids before using them as a LIKE pattern
Unlike `transactionKindForDisplayTxid`, this resolver does not enforce its documented 64-character hexadecimal input contract. SQLite interprets `%` and `_` in the bound value as wildcards, so malformed inputs such as `%` or 64 underscores match unrelated asset-lock rows and `LIMIT 1` returns one row's funding type rather than null. Validate and canonicalize the input in SQL, then compare the exact 64-character outpoint prefix; generated `outPointHex` values are lowercase, while `lower(:txidHex)` preserves support for uppercase canonical txids.
In `packages/kotlin-sdk/sdk/src/main/kotlin/org/dashfoundation/dashsdk/persistence/dao/TransactionDao.kt`:
- [SUGGESTION] packages/kotlin-sdk/sdk/src/main/kotlin/org/dashfoundation/dashsdk/persistence/dao/TransactionDao.kt:42-56: Add Room tests for the new resolver contracts
No SDK test invokes `transactionKindForTxid`, `transactionKindForDisplayTxid`, or `fundingTypeForTxid`. Add in-memory Room coverage using a non-palindromic txid to pin the display-to-wire byte reversal, plus missing, malformed, and uppercase input cases. The asset-lock tests should also cover multiple vouts sharing a txid and define the intended behavior if their funding types differ, because the current nullable scalar query uses `LIMIT 1` and otherwise leaves that selection dependent on row scan order.
| @Query( | ||
| "SELECT fundingTypeRaw FROM asset_locks " + | ||
| "WHERE outPointHex LIKE :txidHex || ':%' LIMIT 1" | ||
| ) | ||
| suspend fun fundingTypeForTxid(txidHex: String): Int? |
There was a problem hiding this comment.
🟡 Suggestion: Validate txids before using them as a LIKE pattern
Unlike transactionKindForDisplayTxid, this resolver does not enforce its documented 64-character hexadecimal input contract. SQLite interprets % and _ in the bound value as wildcards, so malformed inputs such as % or 64 underscores match unrelated asset-lock rows and LIMIT 1 returns one row's funding type rather than null. Validate and canonicalize the input in SQL, then compare the exact 64-character outpoint prefix; generated outPointHex values are lowercase, while lower(:txidHex) preserves support for uppercase canonical txids.
| @Query( | |
| "SELECT fundingTypeRaw FROM asset_locks " + | |
| "WHERE outPointHex LIKE :txidHex || ':%' LIMIT 1" | |
| ) | |
| suspend fun fundingTypeForTxid(txidHex: String): Int? | |
| @Query( | |
| "SELECT fundingTypeRaw FROM asset_locks " + | |
| "WHERE length(:txidHex) = 64 " + | |
| "AND lower(:txidHex) NOT GLOB '*[^0-9a-f]*' " + | |
| "AND substr(outPointHex, 1, 65) = lower(:txidHex) || ':' " + | |
| "LIMIT 1" | |
| ) | |
| suspend fun fundingTypeForTxid(txidHex: String): Int? |
source: ['codex']
There was a problem hiding this comment.
Resolved in this update — Validate txids before using them as a LIKE pattern no longer present.
Auto-resolved by the review system based on the latest commit diff. If you believe this was closed in error, reopen the thread.
| @Query("SELECT transactionTypeKind FROM transactions WHERE txid = :txidWire LIMIT 1") | ||
| suspend fun transactionKindForTxid(txidWire: ByteArray): Int? | ||
|
|
||
| /** | ||
| * Convenience over [transactionKindForTxid] keyed by the explorer | ||
| * DISPLAY txid hex (64 lowercase chars, wire order reversed) the | ||
| * resolver already holds — the same display form used by | ||
| * [AssetLockDao.fundingTypeForTxid]. Reverses to wire order before the | ||
| * BLOB PK match. Returns null for malformed hex (not 64 hex chars) or | ||
| * when no such tx is stored. Not a Room query — a plain default method | ||
| * delegating to [transactionKindForTxid]. | ||
| */ | ||
| suspend fun transactionKindForDisplayTxid(txidDisplayHex: String): Int? { | ||
| val wire = displayHexToWireTxid(txidDisplayHex) ?: return null | ||
| return transactionKindForTxid(wire) |
There was a problem hiding this comment.
🟡 Suggestion: Add Room tests for the new resolver contracts
No SDK test invokes transactionKindForTxid, transactionKindForDisplayTxid, or fundingTypeForTxid. Add in-memory Room coverage using a non-palindromic txid to pin the display-to-wire byte reversal, plus missing, malformed, and uppercase input cases. The asset-lock tests should also cover multiple vouts sharing a txid and define the intended behavior if their funding types differ, because the current nullable scalar query uses LIMIT 1 and otherwise leaves that selection dependent on row scan order.
source: ['codex']
There was a problem hiding this comment.
Resolved in this update — Add Room tests for the new resolver contracts no longer present.
Auto-resolved by the review system based on the latest commit diff. If you believe this was closed in error, reopen the thread.
|
Confirmed no migration is needed — both columns pre-exist and Room's compile-time validation accepted the queries, so that claim is verified rather than assumed. 1. 2. Zero tests for all three new methods. Delete the byte-reversal loop at 3. The description says DashDatabase v9; this branch is at v7 ( Minor: Process note: the 🔴 blocker on this PR is filed against |
|
You're right about the preliminary blocker. I revalidated the exact head ( I've queued a same-SHA automated re-review with this thread context so the deferred second stage can run and supersede the incorrect GitHub would not allow this account to directly dismiss the existing review, so the fresh review must supersede it. @coderabbitai review |
|
🐇 ✅ Action performedReview finished.
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
🧹 Nitpick comments (1)
packages/kotlin-sdk/sdk/src/main/kotlin/org/dashfoundation/dashsdk/persistence/dao/TransactionDao.kt (1)
30-57: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueUse two-space indentation in these Kotlin changes.
The added Kotlin lines use four spaces. Update them to use two spaces.
packages/kotlin-sdk/sdk/src/main/kotlin/org/dashfoundation/dashsdk/persistence/dao/TransactionDao.kt#L30-L57: use two-space indentation for the resolver query and default method.packages/kotlin-sdk/sdk/src/main/kotlin/org/dashfoundation/dashsdk/persistence/dao/TransactionDao.kt#L122-L141: use two-space indentation for the companion object and converter.packages/kotlin-sdk/sdk/src/main/kotlin/org/dashfoundation/dashsdk/persistence/dao/AssetLockDao.kt#L77-L91: use two-space indentation for the resolver documentation and query.As per coding guidelines, "Follow repository EditorConfig settings: 2-space indentation, 4 spaces for Rust files."
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@packages/kotlin-sdk/sdk/src/main/kotlin/org/dashfoundation/dashsdk/persistence/dao/TransactionDao.kt` around lines 30 - 57, Use two-space indentation for the resolver query and default method around transactionKindForTxid and transactionKindForDisplayTxid in packages/kotlin-sdk/sdk/src/main/kotlin/org/dashfoundation/dashsdk/persistence/dao/TransactionDao.kt:30-57; apply the same indentation to the companion object and converter in TransactionDao.kt:122-141 and the resolver documentation and query in AssetLockDao.kt:77-91. Preserve all code and behavior unchanged.Source: Coding guidelines
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Nitpick comments:
In
`@packages/kotlin-sdk/sdk/src/main/kotlin/org/dashfoundation/dashsdk/persistence/dao/TransactionDao.kt`:
- Around line 30-57: Use two-space indentation for the resolver query and
default method around transactionKindForTxid and transactionKindForDisplayTxid
in
packages/kotlin-sdk/sdk/src/main/kotlin/org/dashfoundation/dashsdk/persistence/dao/TransactionDao.kt:30-57;
apply the same indentation to the companion object and converter in
TransactionDao.kt:122-141 and the resolver documentation and query in
AssetLockDao.kt:77-91. Preserve all code and behavior unchanged.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro Plus
Run ID: 07e300c0-df80-4178-a390-a3a36e83a7be
📒 Files selected for processing (2)
packages/kotlin-sdk/sdk/src/main/kotlin/org/dashfoundation/dashsdk/persistence/dao/AssetLockDao.ktpackages/kotlin-sdk/sdk/src/main/kotlin/org/dashfoundation/dashsdk/persistence/dao/TransactionDao.kt
thepastaclaw
left a comment
thepastaclaw
left a comment
There was a problem hiding this comment.
Final validation — Codex + Sonnet
This PR adds two read-only Room resolver queries (TransactionDao.transactionKindForDisplayTxid/transactionKindForTxid and AssetLockDao.fundingTypeForTxid) reusing existing DashDatabase columns with no migration; the display-to-wire txid reversal is correct and Room's compile-time validation accepted the queries against the actual schema. The prior blocking ShieldedDao finding is confirmed misfiled and is dropped: the PR body explicitly scopes those two anchored-note methods to #4204, and the diff never touches ShieldedDao.kt. Two suggestion-level issues persist on the exact head — fundingTypeForTxid doesn't enforce its own documented hex-only contract before using the input as a LIKE pattern, and none of the three new resolver methods have test coverage — plus a minor doc nitpick that the PR description cites schema v9 while DashDatabase.kt declares version 7. Source: reviewers gpt-5.6-sol and claude-sonnet-5 (general); verifier claude-sonnet-5.
Review provenance
- Codex reviewers:
gpt-5.6-sol— general (completed) - Verifier:
claude-sonnet-5— final-verifier - Sonnet reviewers:
claude-sonnet-5— general (completed)
🟡 2 suggestion(s) | 💬 1 nitpick(s)
1 additional finding(s) omitted (not in diff).
🤖 Prompt for all review comments with AI agents
These findings are from an automated code review. Verify each finding against the current code and only fix it if needed.
In `packages/kotlin-sdk/sdk/src/main/kotlin/org/dashfoundation/dashsdk/persistence/dao/AssetLockDao.kt`:
- [SUGGESTION] packages/kotlin-sdk/sdk/src/main/kotlin/org/dashfoundation/dashsdk/persistence/dao/AssetLockDao.kt:87-91: fundingTypeForTxid doesn't enforce its own hex-only contract before using input as a LIKE pattern
fundingTypeForTxid binds txidHex directly into `outPointHex LIKE :txidHex || ':%'` without validating it is a 64-char pure-hex string, unlike the sibling transactionKindForDisplayTxid which enforces exactly that via displayHexToWireTxid and returns null on malformed input. SQLite treats `%`/`_` in the bound value as wildcards, so `fundingTypeForTxid("%")` matches an arbitrary asset-lock row and 64 `_` chars matches every row — with `LIMIT 1` this silently returns a wrong funding type instead of null. The in-code comment's claim that "a txid is a pure-hex string ... so the LIKE pattern carries no wildcards of its own" is an assumption about callers, not an invariant this public method enforces. Separately, even once the pattern is exact-matched, `LIMIT 1` with no `ORDER BY` over a genuinely one-to-many relationship (DIP-0027 permits multiple asset-lock outputs sharing a txid) makes the returned row scan-order dependent; if all vouts for a txid always share the same funding type this is harmless, but that invariant should be documented or the query made deterministic.
In `packages/kotlin-sdk/sdk/src/main/kotlin/org/dashfoundation/dashsdk/persistence/dao/TransactionDao.kt`:
- [SUGGESTION] packages/kotlin-sdk/sdk/src/main/kotlin/org/dashfoundation/dashsdk/persistence/dao/TransactionDao.kt:42-56: No test coverage for the three new resolver methods
Nothing in the repo invokes transactionKindForTxid, transactionKindForDisplayTxid, or fundingTypeForTxid. The hand-written byte-reversal in displayHexToWireTxid (TransactionDao.kt:129-141) has no pinned regression test — deleting the reversal loop entirely would not fail CI, silently mislabeling every withdraw/unshield transaction on-device. The in-memory Room harness already used by WalletDeletionTest is available for this. Add cases with a non-palindromic txid (to catch endian regressions a symmetric fixture like "ab".repeat(32) would miss), uppercase input, malformed/short hex, no-match, and — for fundingTypeForTxid — multiple asset-lock rows sharing a txid prefix to pin down the LIMIT 1 selection behavior.
| @Query( | ||
| "SELECT fundingTypeRaw FROM asset_locks " + | ||
| "WHERE outPointHex LIKE :txidHex || ':%' LIMIT 1" | ||
| ) | ||
| suspend fun fundingTypeForTxid(txidHex: String): Int? |
There was a problem hiding this comment.
🟡 Suggestion: fundingTypeForTxid doesn't enforce its own hex-only contract before using input as a LIKE pattern
fundingTypeForTxid binds txidHex directly into outPointHex LIKE :txidHex || ':%' without validating it is a 64-char pure-hex string, unlike the sibling transactionKindForDisplayTxid which enforces exactly that via displayHexToWireTxid and returns null on malformed input. SQLite treats %/_ in the bound value as wildcards, so fundingTypeForTxid("%") matches an arbitrary asset-lock row and 64 _ chars matches every row — with LIMIT 1 this silently returns a wrong funding type instead of null. The in-code comment's claim that "a txid is a pure-hex string ... so the LIKE pattern carries no wildcards of its own" is an assumption about callers, not an invariant this public method enforces. Separately, even once the pattern is exact-matched, LIMIT 1 with no ORDER BY over a genuinely one-to-many relationship (DIP-0027 permits multiple asset-lock outputs sharing a txid) makes the returned row scan-order dependent; if all vouts for a txid always share the same funding type this is harmless, but that invariant should be documented or the query made deterministic.
| @Query( | |
| "SELECT fundingTypeRaw FROM asset_locks " + | |
| "WHERE outPointHex LIKE :txidHex || ':%' LIMIT 1" | |
| ) | |
| suspend fun fundingTypeForTxid(txidHex: String): Int? | |
| @Query( | |
| "SELECT fundingTypeRaw FROM asset_locks " + | |
| "WHERE length(:txidHex) = 64 " + | |
| "AND lower(:txidHex) NOT GLOB '*[^0-9a-f]*' " + | |
| "AND substr(outPointHex, 1, 65) = lower(:txidHex) || ':' " + | |
| "LIMIT 1" | |
| ) | |
| suspend fun fundingTypeForTxid(txidHex: String): Int? |
source: ['claude', 'codex']
There was a problem hiding this comment.
Fixed in 176f8ed — adopted the suggested query shape: the SQL now enforces the 64-hex contract itself (length(:txidHex) = 64 + hex-only GLOB, lower()-canonicalized) and compares the exact 65-char <txid>: prefix via substr, so there is no LIKE pattern left for %/_ to exploit — fundingTypeForTxid("%") and 64 underscores now return null instead of an arbitrary row's funding type. The multi-vout LIMIT 1 point is addressed in the KDoc: DIP-0027 rows for one txid come from the same funding flow and share fundingTypeRaw, so the unordered pick is value-stable (and the new test pins that with two vouts sharing a txid).
There was a problem hiding this comment.
Resolved in 176f8ed — fundingTypeForTxid doesn't enforce its own hex-only contract before using input as a LIKE pattern no longer present.
Auto-resolved by the review system based on the latest commit diff. If you believe this was closed in error, reopen the thread.
| @Query("SELECT transactionTypeKind FROM transactions WHERE txid = :txidWire LIMIT 1") | ||
| suspend fun transactionKindForTxid(txidWire: ByteArray): Int? | ||
|
|
||
| /** | ||
| * Convenience over [transactionKindForTxid] keyed by the explorer | ||
| * DISPLAY txid hex (64 lowercase chars, wire order reversed) the | ||
| * resolver already holds — the same display form used by | ||
| * [AssetLockDao.fundingTypeForTxid]. Reverses to wire order before the | ||
| * BLOB PK match. Returns null for malformed hex (not 64 hex chars) or | ||
| * when no such tx is stored. Not a Room query — a plain default method | ||
| * delegating to [transactionKindForTxid]. | ||
| */ | ||
| suspend fun transactionKindForDisplayTxid(txidDisplayHex: String): Int? { | ||
| val wire = displayHexToWireTxid(txidDisplayHex) ?: return null | ||
| return transactionKindForTxid(wire) |
There was a problem hiding this comment.
🟡 Suggestion: No test coverage for the three new resolver methods
Nothing in the repo invokes transactionKindForTxid, transactionKindForDisplayTxid, or fundingTypeForTxid. The hand-written byte-reversal in displayHexToWireTxid (TransactionDao.kt:129-141) has no pinned regression test — deleting the reversal loop entirely would not fail CI, silently mislabeling every withdraw/unshield transaction on-device. The in-memory Room harness already used by WalletDeletionTest is available for this. Add cases with a non-palindromic txid (to catch endian regressions a symmetric fixture like "ab".repeat(32) would miss), uppercase input, malformed/short hex, no-match, and — for fundingTypeForTxid — multiple asset-lock rows sharing a txid prefix to pin down the LIMIT 1 selection behavior.
source: ['claude', 'codex']
There was a problem hiding this comment.
Added in 176f8ed: TransactionLabelResolverDaoTest (in-memory Room via the same Robolectric harness as WalletDeletionTest) covers all three resolvers with a NON-palindromic txid (0x01..0x20), pinning the display→wire byte reversal — the display hex must resolve while the unreversed wire hex must miss, so deleting the reversal loop in displayHexToWireTxid now fails CI. Also covered: uppercase input, malformed/short/empty hex, no-match, wildcard inputs (%, 64 underscores) against fundingTypeForTxid, and multiple asset-lock vouts sharing a txid to pin the LIMIT 1 value-stability.
There was a problem hiding this comment.
Resolved in this update — No test coverage for the three new resolver methods no longer present.
Auto-resolved by the review system based on the latest commit diff. If you believe this was closed in error, reopen the thread.
…lver Room tests Review fixes: - AssetLockDao.fundingTypeForTxid no longer binds the caller's input as a LIKE pattern. The query now enforces the documented 64-hex contract itself (length + hex-only GLOB, lower()-canonicalized) and compares the exact 65-char `<txid>:` outPointHex prefix, so wildcard inputs like "%" or 64 underscores return null instead of an arbitrary row's funding type — matching transactionKindForDisplayTxid's null-on-malformed behavior. The DIP-0027 multi-vout LIMIT 1 selection is documented as value-stable (same-flow rows share fundingTypeRaw). - New TransactionLabelResolverDaoTest pins all three resolver contracts on the in-memory Room harness with a NON-palindromic txid (0x01..0x20), so the display->wire byte reversal in displayHexToWireTxid has a regression test: display hex resolves, the unreversed wire hex misses, uppercase is accepted, malformed/wildcard inputs return null, and multiple vouts sharing a txid stay value-stable. Verified: :sdk:testDebugUnitTest --tests '*TransactionLabelResolverDaoTest*' passes (KSP re-validated the rewritten query). Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
|
Following up on the retracted preliminary blocker: confirmed the
The PR description's stale "DashDatabase v9" claim is also corrected — this branch is on schema v7 with no version bump. Since the stale CHANGES_REQUESTED review could not be dismissed directly, requesting the queued re-review run against this head and supersede it. @coderabbitai review |
|
✅ Action performedReview finished.
|
thepastaclaw
left a comment
thepastaclaw
left a comment
There was a problem hiding this comment.
Final validation — Codex + Sonnet
This cumulative review (base ed4116b..176f8ed, latest delta 715aad2..176f8ed) confirms all three prior findings are fixed at exact head 176f8ed: AssetLockDao.fundingTypeForTxid now enforces its 64-hex/ASCII-only contract via length() + NOT GLOB '*[^0-9a-f]*' and an exact substr(outPointHex,1,65) comparison instead of a raw LIKE pattern (verified directly against the SQL); the new TransactionLabelResolverDaoTest.kt exercises all three resolver methods with a genuinely non-palindromic fixture that pins the display↔wire byte reversal and covers wildcard/malformed/uppercase/multi-vout cases; and the PR description now correctly states the queries reuse DashDatabase v7, matching DashDatabase.kt's version = 7. One new, delta-independent (present since 715aad2 but newly surfaced by Codex) low-severity finding remains: TransactionDao.displayHexToWireTxid's use of Character.digit(char, 16) accepts non-ASCII Unicode digit/letter forms (e.g. fullwidth Latin, Arabic-Indic digits) in addition to ASCII hex, which is inconsistent with the new AssetLockDao ASCII-only GLOB guard — this mirrors a pre-existing, intentionally-copied pattern from decodeOutPointHex in PlatformWalletPersistenceHandler.kt, so a proper fix should consolidate both parsers rather than patch only the new copy. No blocking issues found. Source: reviewers gpt-5.6-sol and claude-sonnet-5 (general); verifier claude-sonnet-5.
Review provenance
- Codex reviewers:
gpt-5.6-sol— general (completed) - Verifier:
claude-sonnet-5— final-verifier - Sonnet reviewers:
claude-sonnet-5— general (completed)
🟡 1 suggestion(s)
🤖 Prompt for all review comments with AI agents
These findings are from an automated code review. Verify each finding against the current code and only fix it if needed.
In `packages/kotlin-sdk/sdk/src/main/kotlin/org/dashfoundation/dashsdk/persistence/dao/TransactionDao.kt`:
- [SUGGESTION] packages/kotlin-sdk/sdk/src/main/kotlin/org/dashfoundation/dashsdk/persistence/dao/TransactionDao.kt:129-141: displayHexToWireTxid accepts non-ASCII Unicode hex digits via Character.digit
Java's `Character.digit(char, 16)` is documented to accept Unicode decimal digits (any script where `isDigit()` is true and the decimal value fits the radix, e.g. Arabic-Indic `١`) and fullwidth Latin letters (`Ａ`-`Ｆ`, `ａ`-`ｆ`) in addition to ASCII `0-9a-fA-F`. A 64-character string built from these non-ASCII equivalents therefore decodes successfully instead of returning null, contradicting the method's documented contract ("Returns null for malformed hex (not 64 hex chars)") and diverging from `AssetLockDao.fundingTypeForTxid`'s new ASCII-only `NOT GLOB '*[^0-9a-f]*'` guard. Practical exposure is limited today since no caller yet exists in the tree, but this is new SDK-public surface. Note this exact `Character.digit` pattern is intentionally mirrored from the pre-existing `decodeOutPointHex` in `PlatformWalletPersistenceHandler.kt` (which the doc comment explicitly cites), so patching only this copy would leave an inconsistent sibling; the two hex-parsing loops (already flagged separately as duplicated) should be consolidated into one ASCII-only helper shared by both.
| private fun displayHexToWireTxid(displayHex: String): ByteArray? { | ||
| if (displayHex.length != 64) return null | ||
| val display = ByteArray(32) | ||
| for (i in 0 until 32) { | ||
| val hi = Character.digit(displayHex[i * 2], 16) | ||
| val lo = Character.digit(displayHex[i * 2 + 1], 16) | ||
| if (hi < 0 || lo < 0) return null | ||
| display[i] = ((hi shl 4) or lo).toByte() | ||
| } | ||
| val wire = ByteArray(32) | ||
| for (i in 0 until 32) wire[i] = display[31 - i] | ||
| return wire | ||
| } |
There was a problem hiding this comment.
🟡 Suggestion: displayHexToWireTxid accepts non-ASCII Unicode hex digits via Character.digit
Java's Character.digit(char, 16) is documented to accept Unicode decimal digits (any script where isDigit() is true and the decimal value fits the radix, e.g. Arabic-Indic ١) and fullwidth Latin letters (Ａ-Ｆ, ａ-ｆ) in addition to ASCII 0-9a-fA-F. A 64-character string built from these non-ASCII equivalents therefore decodes successfully instead of returning null, contradicting the method's documented contract ("Returns null for malformed hex (not 64 hex chars)") and diverging from AssetLockDao.fundingTypeForTxid's new ASCII-only NOT GLOB '*[^0-9a-f]*' guard. Practical exposure is limited today since no caller yet exists in the tree, but this is new SDK-public surface. Note this exact Character.digit pattern is intentionally mirrored from the pre-existing decodeOutPointHex in PlatformWalletPersistenceHandler.kt (which the doc comment explicitly cites), so patching only this copy would leave an inconsistent sibling; the two hex-parsing loops (already flagged separately as duplicated) should be consolidated into one ASCII-only helper shared by both.
| private fun displayHexToWireTxid(displayHex: String): ByteArray? { | |
| if (displayHex.length != 64) return null | |
| val display = ByteArray(32) | |
| for (i in 0 until 32) { | |
| val hi = Character.digit(displayHex[i * 2], 16) | |
| val lo = Character.digit(displayHex[i * 2 + 1], 16) | |
| if (hi < 0 || lo < 0) return null | |
| display[i] = ((hi shl 4) or lo).toByte() | |
| } | |
| val wire = ByteArray(32) | |
| for (i in 0 until 32) wire[i] = display[31 - i] | |
| return wire | |
| } | |
| private fun displayHexToWireTxid(displayHex: String): ByteArray? { | |
| if (displayHex.length != 64) return null | |
| val display = ByteArray(32) | |
| for (i in 0 until 32) { | |
| val hi = asciiHexDigit(displayHex[i * 2]) ?: return null | |
| val lo = asciiHexDigit(displayHex[i * 2 + 1]) ?: return null | |
| display[i] = ((hi shl 4) or lo).toByte() | |
| } | |
| val wire = ByteArray(32) | |
| for (i in 0 until 32) wire[i] = display[31 - i] | |
| return wire | |
| } | |
| private fun asciiHexDigit(char: Char): Int? = when (char) { | |
| in '0'..'9' -> char - '0' | |
| in 'a'..'f' -> char - 'a' + 10 | |
| in 'A'..'F' -> char - 'A' + 10 | |
| else -> null | |
| } |
source: ['claude', 'codex']
3 participants
Adds the two wallet-side transaction-display persistence resolver queries the Android cutover needs:
TransactionDao.transactionKindForDisplayTxid— tx-label (Shielded / Invitation / Unshielded) resolutionAssetLockDao.fundingTypeForTxid— asset-lock funding-type (identity registration / top-up / invitation) resolutionRead-only queries; reuse the existing DashDatabase v7 schema (no migration; this branch does not bump the schema version). Extracted from the QA integration line into its own PR per the feature-branch organization.
Note: the two shielded anchored-note queries the app also uses —
ShieldedDao.getUnspentAnchoredNotesByWalletandminUnspentAnchoredBlockHeight— are not in this PR; they ship with the shielded work in #4204 (they are shielded-note queries and belong there). Together, #4204 + this PR provide the four DAO symbols the app requires to compile against the SDK.🤖 Generated with Claude Code
Summary by CodeRabbit