refactor(sql): batch mint quote related data loading by j-kon · Pull Request #2490 · cashubtc/cdk

j-kon · 2026-09-07T15:36:14Z

Description

Eliminates the N+1 query pattern when fetching mint quotes in cdk-sql-common by batch loading related child records (mint_quote_payments and mint_quote_issued) using constant queries instead of looping per quote.

Before: $1 + 2N$ queries for $N$ quotes.
After: $3$ queries constant (1 for parent quotes, 1 for payments belonging to those quotes, 1 for issuances belonging to those quotes).

Key details:

Avoids multi-table JOINs and Cartesian row multiplication ($P \times I$ rows) between payments and issuance tables.
Directly uses the loaded parent quote's CurrencyUnit for child payments and issuances, avoiding redundant joins back to mint_quote.
Groups child records by quote_id in Rust (HashMap<String, Vec<...>>) and attaches them to each MintQuote.
Preserves input order, missing None, and repeated ID semantics in get_mint_quotes_inner.
Preserves single-quote locking semantics: SELECT ... FOR UPDATE acquires row locks on mint_quote first, and relations are read while the lock is held.
Keeps crates/cdk-sqlite/src/async_sqlite.rs unchanged.

Notes to the reviewers

Added comprehensive database integration tests in crates/cdk-common/src/database/mint/test/mint.rs (mint_quotes_load_payments_and_issuance_without_duplicates) covering:
1. Zero payments and zero issuance
2. Multiple payments only
3. Multiple issuance only
4. Multiple payments and multiple issuance (3 payments, 2 issuances)
5. Cross-quote isolation (ensuring quotes do not mix related records)
6. Requested ID ordering preservation
7. Missing IDs returning None
8. Transactional get_mint_quote relations loading with row locking

Suggested CHANGELOG Updates

CHANGED

Batch load mint quote payments and issuance in cdk-sql-common to resolve N+1 queries (Refactor: Use JOIN to get Mint Quotes payments and issuance #2435).

Checklist

I followed the code style guidelines
I ran just quick-check before committing
If the Wallet API was modified (added/removed/changed), I have reflected those changes in the FFI bindings (crates/cdk-ffi)

chatgpt-codex-connector · 2026-09-07T15:36:21Z

You have reached your Codex usage limits for code reviews. You can see your limits in the Codex usage dashboard.
To continue using code reviews, you can upgrade your account or add credits to your account and enable them for code reviews in your settings.

codecov · 2026-09-07T17:04:10Z

Codecov Report

❌ Patch coverage is 98.53659% with 6 lines in your changes missing coverage. Please review.
✅ Project coverage is 79.06%. Comparing base (1c6f683) to head (03785b8).
⚠️ Report is 3 commits behind head on main.

Files with missing lines	Patch %	Lines
crates/cdk-sql-common/src/mint/quotes.rs	96.92%	6 Missing ⚠️

Additional details and impacted files

@@            Coverage Diff             @@
##             main    #2490      +/-   ##
==========================================
+ Coverage   79.05%   79.06%   +0.01%     
==========================================
  Files         387      387              
  Lines      105617   106020     +403     
==========================================
+ Hits        83494    83824     +330     
- Misses      22123    22196      +73

☔ View full report in Codecov by Harness.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:

❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

crodas

Overall looks good, but we cannot panic outside of tests

crodas · 2026-09-10T12:55:43Z

-        let unit = column_as_string!(&row[2], CurrencyUnit::from_str);
        Ok(Issuance::new(
            Amount::from_i64(column_as_number!(row[0].clone()))
                .expect("Is amount when put into db")


Can we throw an error instead of panicking?

Fixed in e0f8789 and retained after the latest sync. The issuance amount conversion now returns ConversionError::InvalidConversion instead of using expect(), so invalid database values are propagated as errors rather than panicking.

crodas · 2026-09-10T12:55:56Z

+                .map(|i| {
+                    Issuance::new(
+                        Amount::from_i64(i.amount)
+                            .expect("Is amount when put into db")


Fixed in e0f8789 as well. The batched issuance reconstruction path now converts the raw value before storing it in RawQuoteIssuance and propagates the same ConversionError on invalid values, so this path no longer panics either.

thesimplekid · 2026-09-10T17:48:01Z

@cdk-bot review

cdk-bot

Verified findings approved for disclosure:

New test's quote4 comment claims "0 payments, 2 issuances" but fixture adds a payment; issuance-only reconstruction case never exercised (low) - see inline comment
Additional locations included in summary:
- crates/cdk-common/src/database/mint/test/mint.rs:2141

cdk-bot · 2026-09-10T18:15:06Z

+        None,
+    );
+
+    // Quote 4: 0 payments, 2 issuances (multiple issuance)


Provenance: lane:persona (persona review lane; the cashu domain skill was requested per the lane methodology but unavailable in this environment).

Root cause (PR-introduced): The new test mint_quotes_load_payments_and_issuance_without_duplicates in crates/cdk-common/src/database/mint/test/mint.rs defines quote4 with a comment claiming "0 payments, 2 issuances" (the issuance-only edge case, line 2063), but the fixture then adds 1 payment to it (lines 2141–2152, add_payment("quote4_pay_1")), so the quote meant to exercise "multiple issuance with no payments" is actually a 1-payment/2-issuance quote. The verifier (verify_q4, lines 2194–2199) matches the actual fixture (q.payments.len() == 1), so the test passes, but the comment at line 2063 and the coverage summary near line 2204 ("Single quote lookups (zero payments/issuance, multiple payments, multiple issuance, multiple of both)") misdescribe what is tested.

Impact: The specific reconstruction case "quote has issuance rows but no payment rows" — where attach_relations_to_quotes in crates/cdk-sql-common/src/mint/quotes.rs must leave quote.payments empty while populating quote.issuance — is never exercised, despite being the case the fixture was named for. The attach logic is symmetric (two independent if let Some(...) branches), so the untested combination is low-risk, but a future regression that, e.g., only attaches payments or cross-links rows when both relations exist would not be caught by this test. The contradictory comment also misleads future maintainers about the fixture's shape.

Fix: Either (a) remove the add_payment block for quote4 (lines ~2141–2152) so it truly has 0 payments and 2 issuances, and update verify_q4 to assert 0 payments — this actually covers the issuance-only case; or (b) keep the fixture as-is and correct the comment at line 2063 to "1 payment, 2 issuances" and adjust the section comment so the claimed coverage matrix matches reality.

Fixed in e7423b2. Quote4 is now a genuine issuance-only fixture with 0 payments and 2 issuances, and verify_q4 asserts that shape. The batch and get-all paths now explicitly exercise relation reconstruction when issuance is present without any payment rows.

j-kon · 2026-09-21T12:36:47Z

Hi @crodas, thanks for the review!

In commit e0f8789, I have addressed both points:

Replaced .expect(...) with error propagation via Amount::from_i64(...).ok_or_else(|| ConversionError::InvalidConversion(...)) in get_mint_quote_issuance and rows_to_issuance_map.
RawQuoteIssuance now holds the validated Amount, making attach_relations_to_quotes completely infallible without panics.
Aligned the test fixture comment on quote4 per cdk-bot's note.

All unit, SQLite, and clippy checks pass cleanly.

refactor(sql): batch mint quote related data loading

2056226

github-project-automation Bot added this to CDK Sep 7, 2026

github-project-automation Bot moved this to Backlog in CDK Sep 7, 2026

refactor(sql): avoid parameter ceiling in get_mint_quotes

03785b8

thesimplekid requested a review from crodas September 9, 2026 15:30

crodas suggested changes Sep 10, 2026

View reviewed changes

github-project-automation Bot moved this from Backlog to In progress in CDK Sep 10, 2026

cdk-bot reviewed Sep 10, 2026

View reviewed changes

fix(sql): propagate conversion error on invalid issuance amount

e0f8789

This was referenced Sep 22, 2026

New Nut: Implement get quotes by pubkeys #1746

Open

Refactor: Use JOIN to get Mint Quotes payments and issuance #2435

Open

thesimplekid requested a review from crodas September 23, 2026 07:13

j-kon added 3 commits September 25, 2026 11:34

test(sql): cover issuance-only mint quotes

e7423b2

Merge branch 'upstream/main' into refactor/mint-quotes-join

f21314f

test(sql): format list in mint quote relations doc comment

0107ea9

Navigation Menu

Repository navigation

refactor(sql): batch mint quote related data loading - #2490

refactor(sql): batch mint quote related data loading#2490
j-kon wants to merge 6 commits into
cashubtc:mainfrom
j-kon:refactor/mint-quotes-join

j-kon commented Sep 7, 2026

Uh oh!

chatgpt-codex-connector Bot commented Sep 7, 2026

Uh oh!

codecov Bot commented Sep 7, 2026

Uh oh!

crodas left a comment

Uh oh!

crodas Sep 10, 2026

Uh oh!

j-kon Sep 25, 2026

Uh oh!

crodas Sep 10, 2026

Uh oh!

j-kon Sep 25, 2026

Uh oh!

thesimplekid commented Sep 10, 2026

Uh oh!

cdk-bot left a comment

Uh oh!

cdk-bot Sep 10, 2026

Uh oh!

j-kon Sep 25, 2026

Uh oh!

j-kon commented Sep 21, 2026

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

Conversation

j-kon commented Sep 7, 2026

Description

Notes to the reviewers

Suggested CHANGELOG Updates

CHANGED

Checklist

Uh oh!

chatgpt-codex-connector Bot commented Sep 7, 2026

Uh oh!

codecov Bot commented Sep 7, 2026

Codecov Report

Uh oh!

crodas left a comment

Choose a reason for hiding this comment

Uh oh!

crodas Sep 10, 2026

Choose a reason for hiding this comment

Uh oh!

j-kon Sep 25, 2026

Choose a reason for hiding this comment

Uh oh!

crodas Sep 10, 2026

Choose a reason for hiding this comment

Uh oh!

j-kon Sep 25, 2026

Choose a reason for hiding this comment

Uh oh!

thesimplekid commented Sep 10, 2026

Uh oh!

cdk-bot left a comment

Choose a reason for hiding this comment

Uh oh!

cdk-bot Sep 10, 2026

Choose a reason for hiding this comment

Uh oh!

j-kon Sep 25, 2026

Choose a reason for hiding this comment

Uh oh!

j-kon commented Sep 21, 2026

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants