Restructured blinding + custom messages + keep vectors #218
Conversation
Egge21M
changed the title
more refactoring more refactoring
Egge21M
changed the title
|
Converted back to draft because I came up with the idea to add factories and this needs some refactoring |
|
Edited the original PR message to reflect new changes. Review required and much appreciated! |
There was a problem hiding this comment.
Looks great! Just some minor nits and questions.
I don't fully understand the keepFactory and when it used. Can you elaborate a bit more on the logic behind that? My assumption is that this is used as a way to default how the user wants to store their proofs, but I think my confusion is related to my comment here
| const { send } = await wallet.send(32, testProofs, { | ||
| outputAmounts: { | ||
| sendAmounts: testOutputAmounts, | ||
| keepAmounts: [16, 8, ...Array(8 - fees).fill(1)] |
There was a problem hiding this comment.
can you explain the logic behind this keepAmounts. Is this something the users of cashu-ts will have to do whenever there are fees and they want change?
There was a problem hiding this comment.
When you use outputAmounts you are required to specify amounts that match the keyset and also satisfy the amount constraints exactly. So yes, if your mint takes fees and you want to specify exact output amounts, you need to consider fees.
| expectProofsSecretToEqual(key2Sends, 'key2'); | ||
| }); | ||
| }); | ||
| describe('Keep Vector and Reordering', () => { |
There was a problem hiding this comment.
it doesn't seem like you are testing the "Keep Vector" here, just that send is properly created when keepAmounts is provided
There was a problem hiding this comment.
It does test the vector, but it is hidden behind the scenes. Any outputs in swap-payload ssend to the mint are now ordered by amount, to increase privacy. By testing that the output of receive is in the same order as the keepAmounts specified, we proof that the reordering of proofs works as expected. The code that does this can be found in CashuWallet.ts#305 (reordering) CashuWallet.ts#872 (sorting)
| privkey?: string, | ||
| customOutputData?: { | ||
| keep?: Array<OutputDataLike> | OutputDataFactory; | ||
| send?: Array<OutputDataLike> | OutputDataFactory; |
There was a problem hiding this comment.
should there be a type for Array<OutputDataLike> | OutputDataFactory;? Seems to be repeated a lot, but maybe its more clear to write it out each time
There was a problem hiding this comment.
I decided to keep a union here, because IDEs will then show the union instead of the alias type. I think this is beneficial, because these two types are very different things that change the way the function behaves.
| secretBytes = new TextEncoder().encode(secretHex); | ||
| pubkey?: string, | ||
| outputAmounts?: Array<number>, | ||
| p2pk?: { pubkey: string; locktime?: number; refundKeys?: Array<string> }, |
There was a problem hiding this comment.
are you keeping pubkey for backwards compatibility? Can we remove it to replace with p2pk?
There was a problem hiding this comment.
Yes exactly. pubkey should be removed with a future major update.
| if (outputData) { | ||
| newOutputData = { send: outputData }; | ||
| } else if (this._keepFactory) { | ||
| newOutputData = { send: this._keepFactory }; |
There was a problem hiding this comment.
why are you using the keepFactory for send?
There was a problem hiding this comment.
Good catch. Unfortunately there is a bug in the current version that requires this: #224
Thank you for taking the time to go through this! Using this, you can create a factory that creates some sort of outputs (like deterministic ones) and make the library use is automatically. A simplistic example of a CashuWallet that creates auto-incrementing, deterministic secrets using a keepFactory can be seen here: https://njump.me/nevent1qqsvzxrfqyt5xk0s7f60mm9rlthkdtv70g3qsl8xykrgcvnfdrfpaecpzemhxue69uhhyetvv9ujumn0wd68ytnzv9hxgqg0waehxw309ahx7um5wghx6mmdqgsdmup6e2z6mcpeue6z6kl08he49hcen5xnrc3tnpvw0mdgtjemh0slwxw3x |
Description
This PR addresses a couple of things that were discussed in the past: Sorted outputs when swapping, as well as custom outputs. I always felt like the old
BlindingDatatype was not ergonomic enough, so I restructured some of the blinding types and logic, moved them to their own Class and Interface. These changes are so closely related, that they became a single PR (that got a little out of hand, sorry...).Changes
outputDatathat implements the OutputDataLike Interface. Using this consumers of the library can opt out of the automatic creation of outputs and supply their own logic.PR Tasks
npm run test--> no failing unit testsnpm run format