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

Sorting blindedMessages by amount #100

Open
wants to merge 2 commits into
base: main
Choose a base branch
from

Conversation

Egge21M
Copy link
Collaborator

@Egge21M Egge21M commented Jan 18, 2024

Fixes: #90

Description

Ordered blindedMessages returned by createSplitPayload by amount and added a keepVector

Changes

  • Ordered blindedMessages by amount
  • added keepVector as return value

PR Tasks

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

@Egge21M Egge21M marked this pull request as ready for review January 26, 2024 12:49
@Egge21M
Copy link
Collaborator Author

Egge21M commented Jan 26, 2024

This is now ready for review. As outline above I changed createSplitPayload to return a payload containing a list of proofs ordered by amount, as well as a list of booleans indicating which proof to keep and which to send.
I then changed send to use the list of booleans (keepVector) to /split and keep the right proofs.

@gandlafbtc @callebtc

Copy link
Collaborator

@gandlafbtc gandlafbtc left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good! Only thing missing is a Unit test testing the behaviour.

amount: sendBlindedMessages.amounts[i],
keep: false
}));
const keepVector: Array<boolean> = [];
const blindedMessages: BlindedTransaction = {
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

let's rename this variable to blindedTransaction. blindedMessages seems kinda confusing. What do you think?

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Oh yes absolutely. Will do

blindedMessages: [],
secrets: [],
rs: [],
amounts: []
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

we could add the keep vector here instead

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

True, I was not sure how many lines of code depend on the type BlindedTransaction and wanted to reduce this PRs footprint. But I will check that out in my next iteration.

@Egge21M
Copy link
Collaborator Author

Egge21M commented Jan 29, 2024

Looks good! Only thing missing is a Unit test testing the behaviour.

I was under the assumption that the old units tests for this method would still suffice, but we should probably add one to check wether the amount is actually sorted (will do asap). Or am I missing something?

@Egge21M
Copy link
Collaborator Author

Egge21M commented Jan 30, 2024

Looks good! Only thing missing is a Unit test testing the behaviour.

What would be good way to easily generate secrets and signatures for the Unit tests?

@gandlafbtc
Copy link
Collaborator

If you're only trying to test the sorting, you don't need valid secrets. It doesn't get validated anyways. You should be able to just mock it!

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.

Order outputs by amount and not in buckets to keep and send
2 participants