feat(core): add protocol and secure BOLT11 foundations - #400
Conversation
🦋 Changeset detectedLatest commit: 3671483 The changes in this PR will be included in the next version bump. This PR includes changesets to release 8 packages
Not sure what this means? Click here to learn what changesets are. Click here if you're a maintainer who wants to add another changeset to this PR |
Egge21M
left a comment
There was a problem hiding this comment.
GM! thank you for turning the massive PR around so quickly. This is just in time, as this overlaps a bit with #398. But that is fine, we will get this merged first and i'll stack my follow up on yours, streamlining claimability a bit more.
However there are some things that need adjustments. One thing is the effort to not log quote IDs. IIrc coco does not sanitize logs as of right now. Secrets and quote ids might get logged many places. So I don't think these changes should be in here and if we decide to sanitize logs we should do it in all places.
|
|
||
| ctx.logger?.debug('Melt quote created', { | ||
| operationId, | ||
| quoteId: quote.quote, |
There was a problem hiding this comment.
Why did you choose t remove the quote ID from the logs?
| } | ||
|
|
||
| if (ctx.operation.quoteId !== quote.quote) { | ||
| throw new Error( |
|
|
||
| async execute(ctx: ExecuteContext<'bolt11'>): Promise<MintExecutionResult> { | ||
| const outputData = deserializeOutputData(ctx.operation.outputData); | ||
| const mintConfig = await this.getMintConfig(ctx.operation.pubkey); |
There was a problem hiding this comment.
This name is quite ambigious. It sounds like getting some config from the mint, not getting the required private key
| state: MintMethodRemoteState, | ||
| observedAt = Date.now(), | ||
| ): Promise<void> { | ||
| await this.db.run( |
There was a problem hiding this comment.
This changes setMintQuoteState from being atomic to possible races. Concurrency is an ongoing issue in coco, but unless necessary we should not regress here
| } | ||
| const lockPubkey = | ||
| shouldLock && !pubkey | ||
| ? (await this.keyRingService.generateMintQuoteKeyPair()).publicKeyHex |
There was a problem hiding this comment.
This call is not safe concurrently and can lead to key reuse. Concurrency is an ongoing issue, but maybe add a TODO here, so we can fix it in the upstream service
|
Hello @Egge21M thanks for the review, I'll attend to all you comments asap. |
7bacc48 to
a19b487
Compare
|
@codex kindly review |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: a19b487dae
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| return { | ||
| observedRemoteState: remoteState, | ||
| observedRemoteStateAt, | ||
| quoteSnapshot: quote, |
There was a problem hiding this comment.
Validate polled snapshots before persisting them
When a paid BOLT11 operation is polled, this raw snapshot is passed by observePendingOperation directly to recordMintQuoteSnapshot; unlike the batch polling path, that path never verifies the request, unit, amount, or NUT-20 pubkey against the existing canonical quote, and resolveMintQuoteObservation accepts changes to those fields as meaningful. A malformed or malicious response can therefore overwrite locked-quote ownership or the fixed amount after the invoice has been paid, causing issuance to fail or recovery to mark the operation terminal. Validate the immutable fields against the operation/canonical quote before returning or persisting this snapshot.
AGENTS.md reference: AGENTS.md:L160-L165
Useful? React with 👍 / 👎.
| if (payload.amount_paid === undefined || payload.amount_issued === undefined) return false; | ||
| const amount = Amount.from(payload.amount); | ||
| const amountIssued = Amount.from(payload.amount_issued); | ||
| return amountIssued.greaterThanOrEqual(amount); |
There was a problem hiding this comment.
Keep watching after invalid issuance accounting
If a subscription sends malformed accounting such as amount_issued >= amount while amount_issued > amount_paid, recordMintQuoteSnapshot rejects or ignores the observation, but this predicate still returns true and handleSubscriptionPayload immediately unsubscribes. The canonical quote remains pending and receives no later valid update, so an operation can stall until another explicit poll occurs. Check the accounting invariant here before treating the quote as terminal.
AGENTS.md reference: AGENTS.md:L146-L152
Useful? React with 👍 / 👎.
| throw new MintOperationError(err.code, message); | ||
| } | ||
| throw new Error(message); |
There was a problem hiding this comment.
Preserve the original locked-mint error as cause
When locked issuance fails, both wrapper branches replace the original exception with a new error containing only its code/message, discarding the original stack and any transport or protocol metadata. This makes production failures substantially harder to diagnose specifically for the newly added locked flow; attach err as cause (or otherwise preserve the original object) when adding the contextual message.
AGENTS.md reference: AGENTS.md:L146-L152
Useful? React with 👍 / 👎.
| .table('coco_cashu_canonical_mint_quotes') | ||
| .get([normalizeMintUrl(mintUrl), method, quoteId])) as MintQuoteRow | undefined; | ||
| if (!existing) return; | ||
| await (this.db as any).table('coco_cashu_canonical_mint_quotes').put({ | ||
| ...existing, | ||
| state, | ||
| amountPaid: state === 'UNPAID' ? '0' : existing.amount, | ||
| amountIssued: state === 'ISSUED' ? existing.amount : '0', | ||
| updatedAt: observedAt, | ||
| } as MintQuoteRow); | ||
| const quote = rowToMintQuote(existing); | ||
| if (!isStatefulMintQuote(quote)) return; | ||
| await this.upsertMintQuote(applyBolt11MintQuoteStateFallback(quote, state, observedAt)); |
There was a problem hiding this comment.
Make the IndexedDB legacy fallback atomic
This IndexedDB path reads a row, computes the fallback, and later performs an independent upsert without a Dexie transaction. If a watcher or poll persists newer ordered accounting between the read and the final write, this stale quote overwrites the newer amountPaid, amountIssued, and remoteUpdatedAt, whereas the SQL implementation's conditional update correctly makes the fallback a no-op. Perform the read/check/write in one transaction or use a conditional atomic mutation.
AGENTS.md reference: AGENTS.md:L160-L163
Useful? React with 👍 / 👎.
a19b487 to
3671483
Compare
Context
Slice 1 of #364. This pull request resolves #365 and establishes the protocol/security boundary required by later Mint Swap slices without exposing a parent swap model or runtime.
Problem
Mint Swap needs authoritative destination quote accounting and recoverable locked-quote ownership before orchestration can safely move value. Existing compatibility
statevalues must not override ordered NUT-04 accounting, and quote identifiers should not be copied into broad structured logs.Summary
amount_paid,amount_issued, andupdated_atauthoritative while retainingstateas a deprecated compatibility projection.Compatibility
Existing unlocked BOLT11 quote creation remains the default. Legacy quote
stateremains available for compatibility but no longer overrides canonical accounting.Verification
bun run typecheckbun run --filter='@cashu/coco-core' test:unitbun --cwd packages/sqlite-bun test src/test/contract.test.tsbun --cwd packages/expo-sqlite test src/test/contract.test.tsbun run --filter='@cashu/coco-sqlite' test -- src/test/contract.test.tsbun run --filter='@cashu/coco-indexeddb' test:browser -- src/test/contract.test.tsChangeset
.changeset/secure-bolt11-foundations.md