Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

NUT-20 Signed Mint Payloads #229

Open
wants to merge 15 commits into
base: development
Choose a base branch
from

Conversation

lollerfirst
Copy link
Contributor

@lollerfirst lollerfirst commented Dec 21, 2024

Description

NUT-20

This PR adds options to add a public key to the payload of a mint quote request and
signing the payload of mint requests with the respective private key.

Note

Since it is my understanding that cashu-crypto-ts is to be wound down, I've created a new directory crypto in which now sits nut-20.ts.

Changes

  • createLockedMintQuote requests a mint quote locked to a specific public key. Errors if the Mint does not support NUT-20.
  • mintProofs now accepts either a string or a LockedMintQuote as the quote parameter. If it's a LockedMintQuote produces a signature on the mint payload.
  • LockedMintQuote is a type containing the quote ID and the secret key to unlock the quote. The user can pass this as:
await mintProofs(10, { id: quoteId, privkey: privateKey }, options);
  • crypto/nut-20.ts contains the necessary logic to sign and verify mint quotes.
  • tests with test vectors

PR Tasks

  • Open PR
  • run npm run test --> no failing unit tests
  • run npm run format

@Egge21M
Copy link
Collaborator

Egge21M commented Dec 22, 2024

Thank you for taking this on! I started a PR a couple of weeks ago, but was not able to finish it, so I am very glad you did.

When I started working on this, I added conditional typing to make sure the returned MintQuotrReponse always has a pubkey, when the request had one. This allows consumers to access response.pubkey without having to guard against it possibly being undefined.

However, thinking about this a little more, having pubkey as an optional makes more sense, because that way consumers are forced to guard against mint responses where nut-20 is unsupported.

I will do a full review of this later today.

@lollerfirst lollerfirst marked this pull request as ready for review December 22, 2024 10:20
@Egge21M
Copy link
Collaborator

Egge21M commented Dec 23, 2024

I wonder if the creation of signed mints should have a separate set of methods.

This would allow us to

a) error if the mint does not respond with a nut-20 compatible reponse (this is great for wallets that want to rely on this)

b) have clear typing that enforces a privkey on mintProofs

It also avoids "option-bloat", which we are currently trying to tackle.

What do you think?
@gandlafbtc please add your 2 sats here as well

@gandlafbtc
Copy link
Collaborator

I think having it as an option fits the current style of the lib. It should however be more clear what happens if the mint doesn't support nut-20. IMO it should error from the start, when trying to create a quote

@Egge21M
Copy link
Collaborator

Egge21M commented Dec 23, 2024

I think having it as an option fits the current style of the lib.

I agree, but I think it is up for debate wether that is a good thing. Methods become increasingly indented, with lots of code paths. This is neither readable nor scaleable IMHO.

It should however be more clear what happens if the mint doesn't support nut-20. IMO it should error from the start, when trying to create a quote.

Aligned. If the consumer clearly signals they want a signed quote while the mint does not support it, we should throw

@lollerfirst
Copy link
Contributor Author

I wonder if the creation of signed mints should have a separate set of methods.

This would allow us to

a) error if the mint does not respond with a nut-20 compatible reponse (this is great for wallets that want to rely on this)

b) have clear typing that enforces a privkey on mintProofs

It also avoids "option-bloat", which we are currently trying to tackle.

What do you think? @gandlafbtc please add your 2 sats here as well

I think that in order to do a separate method we would have to duplicate the logic of mintProofs or just partially abstract it into a common method, because you can only sign once the payload once you know what the payload is.

I agree on the throwing thing. I'll adjust the PR.

@Egge21M
Copy link
Collaborator

Egge21M commented Dec 23, 2024

I think that in order to do a separate method we would have to duplicate the logic of mintProofs or just partially abstract it into a common method, because you can only sign once the payload once you know what the payload is.

I was thinking about something along the lines of

createSignedMintQuote(amount: number, pubkey: Uint8Array): MintQuoteResponse {};

signMintPayload(quote: MintQuoteReponse, privkey: Uint8Array): SignedMintQuoteReponse {};

mintProofs(amount: number, quote: string | SignedMintQuoteReponse): Array<Proof> {};

This clearly separates regular mints from signed ones. It also allows strict types with less optionals. Signing and minting are decoupled. However mintProofs needs a bit of adjustment to handle quote as string | SignedMintQuoteResponse. Once we start working on v3 I would like to get rid of string in this case anyway to make this more in line with meltProofs (which takes in the whole reponse)

@lollerfirst
Copy link
Contributor Author

lollerfirst commented Dec 23, 2024

I think that in order to do a separate method we would have to duplicate the logic of mintProofs or just partially abstract it into a common method, because you can only sign once the payload once you know what the payload is.

I was thinking about something along the lines of

createSignedMintQuote(amount: number, pubkey: Uint8Array): MintQuoteResponse {};

signMintPayload(quote: MintQuoteReponse, privkey: Uint8Array): SignedMintQuoteReponse {};

mintProofs(amount: number, quote: string | SignedMintQuoteReponse): Array<Proof> {};

This clearly separates regular mints from signed ones. It also allows strict types with less optionals. Signing and minting are decoupled. However mintProofs needs a bit of adjustment to handle quote as string | SignedMintQuoteResponse. Once we start working on v3 I would like to get rid of string in this case anyway to make this more in line with meltProofs (which takes in the whole reponse)

I don't follow. Shouldn't signMintPayload also take the actual outputs of the MintPayload? Because that's how the spec works: you sign the quote string together with the blinded messages. And why does it return a SignedMintQuoteResponse as opposed to a SignedMintPayload?

@Egge21M
Copy link
Collaborator

Egge21M commented Dec 23, 2024

I don't follow. Shouldn't signMintPayload also take the actual outputs of the MintPayload? Because that's how the spec works: you sign the quote string together with the blinded messages. And why does it return a SignedMintQuoteResponse as opposed to a SignedMintPayload?

You are right. Please apologise the confusion, I wrote this from the top of my head. But the example still illustrates the flow. The point is that signing is somewhat decoupled from minting. I think this way we can extend the functionality without adding too much complexity to each of the methods.

@gandlafbtc
Copy link
Collaborator

We can break up methods at a level inside the lib. I still feel like it is less overwhelming for an implementor to just have a single method, and then options & parameters to customize.

If you guys disagree with this design, we can move away from it, but I think it's out of scope for this PR. IMO it would affect the lib as a whole since it is a fundamental design choice, and it should be done in the same way everywhere (not just have this one method as an exception)

@Egge21M
Copy link
Collaborator

Egge21M commented Dec 26, 2024

We can break up methods at a level inside the lib. I still feel like it is less overwhelming for an implementor to just have a single method, and then options & parameters to customize.

I STRONGLY disagree. IMHO it is much more overwhelming to dig through a bunch of options and understand what each one of them does.

Especially because the semantic links are missing (e.g. the param "pubkey" creating a signed mint).

If methods are self describing, like "createdSignedMintQuote" implementers don't even need to read the docs to get it. It also allows stricter typing and self documenting parameters.

If you guys disagree with this design, we can move away from it, but I think it's out of scope for this PR. IMO it would affect the lib as a whole since it is a fundamental design choice, and it should be done in the same way everywhere (not just have this one method as an exception)

While you are right about this being out of scope, I don't want to knowingly merge stuff that changes the public API in a non-desirable way. Especially because fixing it will require a major version bump.

Postponing this debate does more harm and increases tech-debt.

@lollerfirst
Copy link
Contributor Author

lollerfirst commented Dec 26, 2024

@Egge21M We would still need to supply mintProofs with the private key somehow, because that's where the blinded outputs are generated and they go into the signature (NUT-20). That's kind of unavoidable, unless you want to supply the MintPayload directly to mintProofs and abstract the blinded secret generation away as well.
We would have something like this:

async createLockedMintQuote(amount: number, description?: string, pubkey?: string): Promise<MintQuoteResponse> {...}
type LockedMintQuote {
    quote: MintQuoteResponse | string;
    privkey: string;
};
async mintProofs(amount: number, quote: string | LockedMintQuote, options?: MintProofOptions): Promise<Array<Proof>> {...} 

Alternatively, supply the private key to createLockedMintQuote, which will derive the corresponding public key to lock the quote to. This allows createLockedMintQuote to return a promise with the LockedMintQuote struct directly.

async createLockedMintQuote(amount: number, description?: string, privkey?: string): Promise<LockedMintQuoteResponse> {...}

Although I am neutral to the option redesign argument.

@Egge21M
Copy link
Collaborator

Egge21M commented Dec 26, 2024

Alternatively, supply the private key to createLockedMintQuote, which will derive the corresponding public key to lock the quote to. This allows createLockedMintQuote to return a promise with the LockedMintQuote object directly.

That was the first design I had in mind, however there might be use cases where consumers lock to keys they do no own. So we should make createLockedMintQuote work without knowing the secret.

@Egge21M We would still need to supply mintProofs with the private key somehow, because that's where the blinded outputs are generated and they go into the signature (NUT-20). That's kind of unavoidable, unless you want to supply the MintPayload directly to mintProofs and abstract the blinded secret generation away as well. We would have something like this:

That is a very good point. In this case we can not get around adding parameters to mintProofs. I would in this case still prefer an overload over an option. That way TypeScript would at least complain if mintProofs was called with a LockedMintQuote, but without a secret key.

mintProofs(amount: number, quote: LockedMintQuote, privateKey: Uint8Array, options?: MintProofOptions): Promise<Array<Proof>>;
mintProofs(amount: number, quote: string | LockedMintQuote, options?: MintProofOptions): Promise<Array<Proof>> {...} 

I think that is a good middle path, no?

@Egge21M
Copy link
Collaborator

Egge21M commented Dec 27, 2024

Here is what I had in mind: lollerfirst/cashu-ts@sign-mint-quotes...cashubtc:cashu-ts:typesafe-adjustments

This makes sure that TypeScript will not let me mint a quote created with createLockedMintQuote without adding a privkey to the options.

const lockedQuote = await wallet.createLockedMintQuote(21, 'a pubkey');
const proofsError = await wallet.mintProofs(21, lockedQuote); // TypeScripts errors on this
const lockedProofsNoError = await wallet.mintProofs(21, lockedQuote, { privateKey: '123' }); // This works

const unlockedMint = await wallet.createMintQuote(21);
const unlockedProofsNoError = await wallet.mintProofs(21, unlockedMint.quote) // This works

@lollerfirst let me know if I should add these changes to your branch

@lollerfirst
Copy link
Contributor Author

Here is what I had in mind: lollerfirst/[email protected]:cashu-ts:typesafe-adjustments

This makes sure that TypeScript will not let me mint a quote created with createLockedMintQuote without adding a privkey to the options.

const lockedQuote = await wallet.createLockedMintQuote(21, 'a pubkey');
const proofsError = await wallet.mintProofs(21, lockedQuote); // TypeScripts errors on this
const lockedProofsNoError = await wallet.mintProofs(21, lockedQuote, { privateKey: '123' }); // This works

const unlockedMint = await wallet.createMintQuote(21);
const unlockedProofsNoError = await wallet.mintProofs(21, unlockedMint.quote) // This works

@lollerfirst let me know if I should add these changes to your branch

This is nice but doesn't it already do that? LockedMintQuote consists of a privkey and a quote id, if I don't specify a privkey doesn't it error?

@lollerfirst
Copy link
Contributor Author

lollerfirst commented Dec 27, 2024

The way it works right now:

const quoteResponse = await createLockedMintQuote(21, "02...");
const lockedProofsNoError = await mintProofs(21, { id: quoteResponse.quote, privkey: "3bca..." });

...Or should work. Haven't tested it yet.

@lollerfirst
Copy link
Contributor Author

No OK I get it: you could still specify a quote ID as a normal quote. Your solution is better.

@lollerfirst
Copy link
Contributor Author

Tests failing because Nutshell 0.16.3 release doesn't yet have NUT-20. Once the new release comes should be good.

@lollerfirst lollerfirst changed the title NUT-XX Signed Mint Payloads NUT-20 Signed Mint Payloads Dec 30, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants