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

Fix coin selection max number of inputs not diminishing as expected #523

Merged
merged 2 commits into from
Jul 10, 2019

Conversation

KtorZ
Copy link
Member

@KtorZ KtorZ commented Jul 10, 2019

Issue Number

#522

Overview

  • I have added regression tests to both largestFirst and random selection.
  • I have moved the CoinSelectionOption to be part of the fold accumulator, so that it can be updated between each output
  • I have slightly adjusted the improve part so that it will also behave accordingly.

Comments

@KtorZ KtorZ requested review from paweljakubas and piotr-iohk July 10, 2019 14:32
@KtorZ KtorZ self-assigned this Jul 10, 2019
@paweljakubas paweljakubas force-pushed the KtorZ/522/coin-selection-options branch from 51ae115 to f96bec8 Compare July 10, 2019 16:44
KtorZ added 2 commits July 10, 2019 18:48
Seems that we only respect the 'maximum number of inputs' for each output independantly, but not globally.

See: #522
…prove algorithm

This is because inputs are 'consumed' as we process outputs, so we gotta keep the number of allowed inputs in the loop and not only
look at it as a global value.
@paweljakubas paweljakubas merged commit d6b10f6 into master Jul 10, 2019
@paweljakubas paweljakubas deleted the KtorZ/522/coin-selection-options branch July 10, 2019 17:12
rvl added a commit that referenced this pull request Jul 11, 2019
piotr-iohk pushed a commit that referenced this pull request Jul 11, 2019
piotr-iohk pushed a commit that referenced this pull request Jul 12, 2019
@KtorZ KtorZ added this to the Bugs & Debts - Sprint 27-28 milestone Jul 24, 2019
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.

2 participants