feat: use Uuid as mint and melt quote ids #469

tdelabro · 2024-11-22T14:24:30Z

Description

This PR changes the type of mint and melt quote Ids to Uuid rather than String.
It required changes in some field types along with some trait signatures.

It will improve the code security by manipulating check Uuid, instead of string that can be anything.
It will also improve performances as Uuid fit on less memory [u8; 16] than its string representation. Same for database.
It also avoids some memory allocation by being Copy. So no need to clone.
It also avoids mixing different types into String. Eg. In nut17 there was a part where both PublicKeys and Uuid were represented as the same type String and fed to the same method. This is really error prone and should be avoided.

Notes to the reviewers

I did not write the migration to modify the Ids of the tables in the different database from string to uuid/bytes or whatever is the underlying representation.
Can you do it as you are more familiar with the multiple database supported and the way to write and test those migrations.

Suggested CHANGELOG Updates

CHANGED: use uuid as identifier for mint and melt quotes

Checklist

I followed the code style guidelines
I ran just final-check before committing

thesimplekid · 2024-11-23T08:48:39Z

Thanks for the PR.

Since the quote id being a uuid is an implementation choice and not something that is defined in the NUTs we cannot enforce or expect a mint to return and a uuid in the wallet. However, maybe for some of the reasons you stated above it would be worth using the uuid type internally for the Mint and enforcing it there. Internal types like MintQuote, MeltQuote could use the uuid type and then types like MeltQuoteBolt11Response would have to stay string.

What do you think?

tdelabro · 2024-11-29T21:33:36Z

@thesimplekid can you find time to review this this week?

thesimplekid · 2024-11-30T22:27:50Z

Yes sorry for the slow review. Will be done tomorrow.

thesimplekid

Thank you for this PR I think its a good improvement and one to enforce early in the project.

Just a few notes on the REDB migrations but other wise LGTM.

I'm happy to migrate the sqlite but that can be a separate PR.

crates/cdk-redb/src/mint/migrations.rs

thesimplekid · 2024-12-01T10:02:52Z

crates/cdk/src/nuts/nut17/mod.rs

+impl From<Params> for Vec<Index<Notification>> {
    fn from(val: Params) -> Self {
        let sub_id: SubscriptionGlobalId = Default::default();
        val.filters
-            .iter()
-            .map(|filter| Index::from(((filter.clone(), val.kind), val.id.clone(), sub_id)))
-            .collect()
+            .into_iter()
+            .map(|filter| {
+                let idx = match val.kind {
+                    Kind::Bolt11MeltQuote => {
+                        Notification::MeltQuoteBolt11(Uuid::from_str(&filter)?)
+                    }
+                    Kind::Bolt11MintQuote => {
+                        Notification::MintQuoteBolt11(Uuid::from_str(&filter)?)
+                    }
+                    Kind::ProofState => Notification::ProofState(PublicKey::from_str(&filter)?),
+                };
+
+                Ok(Index::from((idx, val.id.clone(), sub_id)))
+            })
+            .collect::<Result<_, anyhow::Error>>()
+            .unwrap()
+        // TODO don't unwrap, move to try from


@crodas I think we should merge this and then rebase your PR #473 on this. Could you make this a TryFrom there or is it still relevant I haven't reviewed that PR yet but know it changes a good bit in these files. What do you think?

Can I keep the unwrap into the code for new?

I think moving to try_from will imply some changes in our custom pubsub lib, which may require rethinking part it's design, which I don't have context about and is slightly out of the scope of this PR

Yes, lets do that separately. I meant that comment for @crodas

We should not unwrap anything; we can either bubble up the error or exit logging it if it is unexpected (a malformed user-given input is not an error). Otherwise, it could be an attack vector that reboots the mint to flood the log with garage entries.

Having a Uuid is fine, but it shouldn't be enforced, instead hinted, as it should be backward compatible. Perhaps the ID is an enum of Uuid(Uuid), Legacy(String). It shouldn't be enforced unless there is a standard/NUT that enforces quotes ID to be UUIDs.

We should not unwrap anything; we can either bubble up the error or exit logging it if it is unexpected (a malformed user-given input is not an error). Otherwise, it could be an attack vector that reboots the mint to flood the log with garage entries.

I agree we should not have unwraps in the codebase. Its not a question of do we keep the unwrap in the codebase its a question of do we fix this here in this PR or in your immediate follow up PR related to wallet WS #473. As changing it here may cause a lot of merge conflicts for you.

It shouldn't be enforced unless there is a standard/NUT that enforces quotes ID to be UUIDs.

This is why we don't and can't enforce it for the wallet. However for the mint it is an implementation choice that we are free to make and it has always been UUID in cdk just stored as a string and treated as a string so there are no legacy IDs.

Having a Uuid is fine, but it shouldn't be enforced, instead hinted, as it should be backward compatible. Perhaps the ID is an enum of Uuid(Uuid), Legacy(String).

I think using an enum makes it more complicated and gives flexibility we don't need to the mint as we would always use the Uuid variant of the enum. I believe it better to take an opinionated stance for the reasons @tdelabro pointed to in the description. Supporting backwards compatibility should only be done in cases where it is absolutely needed as it leads to significant tech debt that should be avoided where possible.

I agree we should not have unwraps in the codebase. Its not a question of do we keep the unwrap in the codebase its a question of do we fix this here in this PR or in your immediate follow up PR related to wallet WS #473. As changing it here may cause a lot of merge conflicts for you.

This is not a blocked and I can improve this bit on my PR, no worries.

This is why we don't and can't enforce it for the wallet. However for the mint it is an implementation choice that we are free to make and it has always been UUID in cdk just stored as a string and treated as a string so there are no legacy IDs.

Yes, having a strict well defined type is better. String could be a 4GB video, as long as it is a valid UTF-8 string. I like this approach. I can improve the unwraps that I don't like and add some logging in my PR.

tdelabro · 2024-12-01T12:43:30Z

@thesimplekid thanks for the review! I will get back to it on Monday

tdelabro · 2024-12-02T10:08:32Z

@thesimplekid ready for another review

thesimplekid

ACK f101a0a

Thanks again. I'll just hold off on merging and give @crodas a chance to comment.

crodas · 2024-12-02T20:12:42Z

crates/cdk/src/nuts/nut04.rs

    /// Quote Id
-    pub quote: String,
+    pub quote: Q,


Instead of leaking the type as generic, isn't it better to have an ID type? And have that ID be a String or an UUID instead? /cc @thesimplekid @tdelabro

Having MeltId wrap an String or an UUID based on a cfg.

Having MeltId wrap an String or an UUID based on a cfg.

I don't think we can do it based on cfg as features are additive so both the mint and wallet feature could be and are by default enabled at the same time.

Perhaps instead of leaking then, we can have a type MintQuoteBolt11Response<Q> we can have a type type MintQuoteBolt11Response = nut04::MintQuoteBolt11Response<Uuid> in the mint crate.

Other than that I like this PR.

We could also keep it as a string in the Response as to be inline with the nut. And then use the uuid in only the mint mod.

Yes, that could also be it, but I also like the idea of abstract generic types in a crate, then specific ones inside mint for instance, and have the webserver and future RPC use those types, but have the wallet use the generic one using a String instead.

We could also keep it as a string in the Response as to be inline with the nut. And then use the uuid in only the mint mod.

Doesn't really make sense IMO.
It happens that the protocol specifies that the id can be anything that can be serialized into a string.
It happens that in some part of the codebase, we know for sure it is a Uuid, and in some other parts we don't know so we go for the smaller common denominator, a String.

But going from bytes to String to Uuid would add an intermediate, non-necessary step in the serialization.

I don't really see any way to avoid a generic here

What is the issue with having a generic? What are you trying to avoid here?

There is nothing wrong, but I think it could be best and save some time at usage time to have an alias that people can reference directly.

Is readability your only issue?

Yes, pretty much yes. It was not a blocker but a minor suggestion.

Let's merge it that way and see if it becomes a problem for users.
I'm usually in favor of being explicit until it becomes an overcharge of information.
Do you agree?

I'm happy with it as is.

thesimplekid · 2024-12-04T16:00:27Z

ACK f101a0a

tdelabro force-pushed the use-uuid-for-quote-id branch 3 times, most recently from 42def59 to 42209ce Compare November 25, 2024 18:56

thesimplekid requested changes Dec 1, 2024

View reviewed changes

tdelabro added 7 commits December 2, 2024 10:16

feat: use Uuid as mint and melt quote ids

f65eab0

chore: clippy

a172d0a

chore: fmt

e8d5ba6

fix: keep using String as QuoteId for wallet

bfd0c1b

fix: import only for feature mint

8f8d2e9

fix: migrate redb + keep using hyphenated string for sqlite

5073095

fix: redb migration were bugged

f101a0a

tdelabro force-pushed the use-uuid-for-quote-id branch from 42209ce to f101a0a Compare December 2, 2024 09:40

thesimplekid approved these changes Dec 2, 2024

View reviewed changes

crodas reviewed Dec 2, 2024

View reviewed changes

thesimplekid requested a review from crodas December 4, 2024 16:00

thesimplekid merged commit 7d87c48 into cashubtc:main Dec 5, 2024
42 checks passed

vnprc pushed a commit to vnprc/cdk that referenced this pull request Dec 18, 2024

feat: use Uuid as mint and melt quote ids (cashubtc#469)

c0df5f1

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat: use Uuid as mint and melt quote ids #469

feat: use Uuid as mint and melt quote ids #469

tdelabro commented Nov 22, 2024 •

edited

Loading

thesimplekid commented Nov 23, 2024

tdelabro commented Nov 29, 2024

thesimplekid commented Nov 30, 2024

thesimplekid left a comment

thesimplekid Dec 1, 2024 •

edited

Loading

tdelabro Dec 2, 2024

thesimplekid Dec 2, 2024 •

edited

Loading

crodas Dec 2, 2024

thesimplekid Dec 2, 2024

crodas Dec 2, 2024

tdelabro commented Dec 1, 2024

tdelabro commented Dec 2, 2024

thesimplekid left a comment

crodas Dec 2, 2024

thesimplekid Dec 2, 2024

crodas Dec 2, 2024

thesimplekid Dec 2, 2024

crodas Dec 2, 2024

tdelabro Dec 3, 2024

tdelabro Dec 3, 2024

crodas Dec 3, 2024

tdelabro Dec 4, 2024

thesimplekid Dec 4, 2024

thesimplekid commented Dec 4, 2024

feat: use Uuid as mint and melt quote ids #469

feat: use Uuid as mint and melt quote ids #469

Conversation

tdelabro commented Nov 22, 2024 • edited Loading

Description

Notes to the reviewers

Suggested CHANGELOG Updates

CHANGED: use uuid as identifier for mint and melt quotes

Checklist

thesimplekid commented Nov 23, 2024

tdelabro commented Nov 29, 2024

thesimplekid commented Nov 30, 2024

thesimplekid left a comment

Choose a reason for hiding this comment

thesimplekid Dec 1, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

thesimplekid Dec 2, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

tdelabro commented Dec 1, 2024

tdelabro commented Dec 2, 2024

thesimplekid left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

thesimplekid commented Dec 4, 2024

tdelabro commented Nov 22, 2024 •

edited

Loading

thesimplekid Dec 1, 2024 •

edited

Loading

thesimplekid Dec 2, 2024 •

edited

Loading