feat: add multi-unit support (sat, usd, auth, etc.)#88
feat: add multi-unit support (sat, usd, auth, etc.)#884xvgal wants to merge 4 commits intocashubtc:masterfrom
Conversation
Support multiple currency units per mint by adding unit field to operations and filtering proofs by keyset unit. Backward compatible with 'sat' as default.
|
Egge21M
left a comment
There was a problem hiding this comment.
Thank you so much for taking on this challenge! It would be amazing to have this in for the stable v1 release.
- Add unit parameter to MintQuoteService.createMintQuote (default: 'sat') - Update redeemMintQuote to use quote.unit when getting wallet - Add unit parameter to QuotesApi.createMintQuote - Add unit tests for createMintQuote with different units - Complete multi-unit integration tests (remove TODO placeholders)
| @@ -82,6 +84,7 @@ const operationToParams = (operation: MeltOperation): unknown[] => { | |||
| return [ | |||
| operation.id, | |||
There was a problem hiding this comment.
operationToParams() now includes operation.unit, but the create() INSERT column list still omits unit while passing params. That creates a bindings-count mismatch at runtime when persisting melt operations. Add unit to the INSERT columns/placeholders to match params.
There was a problem hiding this comment.
Fixed. Unit column and placeholder added to the INSERT statement.
Coulumn count 18 matches operationToParams()
| @@ -82,6 +84,7 @@ const operationToParams = (operation: MeltOperation): unknown[] => { | |||
| return [ | |||
| operation.id, | |||
There was a problem hiding this comment.
Same issue as sqlite3 repo: operationToParams() includes unit, but create() INSERT still has the old column list. This will fail with a parameter/placeholder mismatch on melt operation creation. Please include unit in INSERT columns/placeholders.
| @@ -67,6 +69,7 @@ function operationToParams(op: SendOperation): unknown[] { | |||
| return [ | |||
| op.id, | |||
There was a problem hiding this comment.
operationToParams() now returns op.unit, but create() INSERT still omits the unit column. That causes binding mismatch when creating send operations in expo-sqlite. Add unit to the INSERT column list and placeholders.
There was a problem hiding this comment.
unit added to INSERT column list (13 columns, 13 params)
| return results; | ||
| } | ||
|
|
||
| async getAvailableProofs(mintUrl: string): Promise<CoreProof[]> { |
There was a problem hiding this comment.
ProofRepository now requires getAvailableProofs(mintUrl, unit), but this implementation still declares only mintUrl. This breaks assignability/typecheck (and bypasses unit filtering in memory adapter). Please update the signature and filter ready proofs by unit via keyset mapping, consistent with other repositories.
There was a problem hiding this comment.
Matched Decalres
Signature updated to getAvailableProofts(mintUrl, unit). Filters proofs by resolving proof.id -> keyset -> keyset.unit, consistent with SQL JOIN approach in other repositories
ed7a0fb to
8ec98fe
Compare
Egge21M
left a comment
There was a problem hiding this comment.
Great progress on multi-unit support, but there are a few correctness gaps we need to tackle before merging. One is technically not a bug, but a "behavior" issue that I think should be fixed
| // First, get keyset IDs for this unit | ||
| const keysets = (await (this.db as any) | ||
| .table('coco_cashu_keysets') | ||
| .where('[mintUrl+unit]') |
There was a problem hiding this comment.
This compound index is not part of the schema
| if (!unit || unit.trim().length === 0) { | ||
| throw new ProofValidationError('unit is required'); | ||
| } | ||
| const keysetIds = await this.getKeysetIdsForUnit(mintUrl, unit); |
There was a problem hiding this comment.
This turns the getBalance() call into something that relies on networking. Looking at the big picture of supporting as many offline-first use cases as we can, we should avoid network calls when acting on local data.
Balance is the sum of all proofs that we have stored. We should not have any proofs of unknown keysets stored. So I think it would be best to have this act only on local data (even if there is a tiny chance of it being incomplete)
| * Gets balances for all mints by summing ready proof amounts. | ||
| * @returns An object mapping mint URLs to their balances | ||
| */ | ||
| async getBalances(): Promise<{ [mintUrl: string]: number }> { |
There was a problem hiding this comment.
This will mix units and sum them up into one number. I don't think that is desirable. We should probably add a unit param with default sat for backwards compatibility here
Egge21M
left a comment
There was a problem hiding this comment.
I did a more thorough review and noticed that there are still a few places where coco assumes sat as unit and things will break.
We use CDK for our integration tests and it support custom units. I think it would be easy to spot these issues by running the whole integration suite using a custom unit like "USD" against a USD cdk mint once.
Please let me know if you need help adjusting the integration test to do that
| } | ||
| } | ||
|
|
||
| async createOutputsAndIncrementCounters( |
There was a problem hiding this comment.
This method is a critical part of all receive/swap flows and right now it defaults to sat.
| const { wallet } = await this.walletService.getWalletWithActiveKeysetId(mintUrl); | ||
| const unit = quote.unit || 'sat'; | ||
| const { wallet } = await this.walletService.getWalletWithActiveKeysetId(mintUrl, unit); | ||
| const { keep } = await this.proofService.createOutputsAndIncrementCounters(mintUrl, { |
There was a problem hiding this comment.
The created outputs will be for unit: "sat". See: https://github.com/cashubtc/coco/pull/88/changes#r2823058504
| this.logger = logger; | ||
| } | ||
|
|
||
| async receive(token: Token | string): Promise<void> { |
There was a problem hiding this comment.
This method still assumes sat for all token
| return preparedProofs; | ||
| } | ||
|
|
||
| async createBlankOutputs(amount: number, mintUrl: string): Promise<OutputData[]> { |
| * @param serializedOutputData - The serialized output data containing secrets and blinding factors | ||
| * @returns The recovered proofs (only unspent ones) | ||
| */ | ||
| async recoverProofsFromOutputData( |
| const entry: Omit<ReceiveHistoryEntry, 'id'> = { | ||
| type: 'receive', | ||
| createdAt: Date.now(), | ||
| unit: token.unit || 'sat', |
There was a problem hiding this comment.
This will default to sat as TransactionService.receive emits the event without unit
Description
etc.) per mint. Each mint can have multiple keysets with different units, and this implementation enables filtering proofs
and balances by unit.
Key changes:
Notes to the reviewers
reading old database rows without a unit field, it defaults to 'sat'.
Existing data works seamlessly.
proof filtering.
Suggested CHANGELOG Updates
ADDED
MODIFIED
REMOVED