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

Amount Preferences Fixes #140

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

Conversation

lollerfirst
Copy link

@lollerfirst lollerfirst commented Jun 18, 2024

Fixes: #129

Changes

  • getPreferences checks input amouts against the amounts of the keys
  • keepPreferences and sendPreferences as different optional parameters in createSplitPayload.
  • modify splitAmount behaviour -in case of no specified preference- to create amounts not from powers of 2
    but from the amounts of the keyset instead.
  • splitAmount reorders its outputs by descending/ascending order.

PR Tasks

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

@lollerfirst lollerfirst marked this pull request as ready for review June 20, 2024 13:39
@Egge21M
Copy link
Collaborator

Egge21M commented Jul 2, 2024

This looks good to me, great job! However we should wait for development to be cleaned up (staging and dev got out of sync a bit), so that its a bit clearer which parts of the library this PR touched. Once that is done I can do a final review.

@lollerfirst
Copy link
Author

Now the changes are isolated.

Comment on lines +559 to +562
export type Preferences = {
sendPreference: Array<AmountPreference>;
keepPreference?: Array<AmountPreference>;
};
Copy link
Contributor

Choose a reason for hiding this comment

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

This is an awesome PR!

If we do introduce breaking changed is on how the preference is passed, what do you guys think of getting rid of the (rather cumbersome?) format for the preference as sendPreference: [{ amount: 1, count: 3 }] and instead use a pure array of numbers a la sendPreference: [1, 1, 1]? I think I would prefer that.

Copy link
Author

Choose a reason for hiding this comment

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

Probably a good idea. I think the original motive behind it was noble (not having an array with a billion useless 1s in it) but this is more practical.

Copy link
Contributor

@callebtc callebtc left a comment

Choose a reason for hiding this comment

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

Really like this PR. We have to decide wether this should be a breaking change in the API or not. We could also deprecate the old method for setting the amount preference (see my comment).

Comment on lines -217 to +219
preference?: Array<AmountPreference>;
preference?: Preferences;
Copy link
Contributor

Choose a reason for hiding this comment

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

How about we deprecate preference, do the new thing in a new option called outputAmounts, and write a thin translation method that maps preference to outputAmounts, and issue a deprecation warning?

Copy link
Author

Choose a reason for hiding this comment

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

I second this

@callebtc
Copy link
Contributor

callebtc commented Jul 25, 2024

Maybe this is out of scope for this PR but we should also get rid of this behavior of the lib where it overwrites amount if preference is set. We should throw an error instead of overwriting things and force the implementor to use the library properly.

Example:

if (options?.preference) {
    amount = options?.preference?.reduce((acc, curr) => acc + curr.amount * curr.count, 0);
}

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.

None yet

3 participants