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

Disable Automatic Algorithm Fallback #44

Merged
merged 2 commits into from
Apr 15, 2020

Conversation

jonathanknowles
Copy link
Contributor

@jonathanknowles jonathanknowles commented Apr 15, 2020

Related Issue

#30

Summary

The cardano-wallet library currently performs automatic fallback from Random-Improve to Largest-First if the former algorithm is unable to compute a valid coin selection.

However, this automatic fallback might not be appropriate for all users of the coin selection library.

This PR:

  • removes the automatic fallback logic within the implementation of Random-Improve.
  • adds an appropriate error handling function, since Random-Improve can no longer rely on the error handling provided by Largest-First.

The error handling is already tested by our existing test suite.

Disable automatic fallback from Random-Improve to Largest-First.

If the caller needs to perform fallback, they can always manually invoke
algorithms one after the other.
Copy link
Contributor

@paweljakubas paweljakubas left a comment

Choose a reason for hiding this comment

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

if we disable largest first fallback, would be good to show in testing that we can recreate old setup. by invoking random and then rely on largestFirst. And here deliver test showing that we do not have fallback (there were test like that to my best knowledge in cardano-wallet), and when having the composition with fallback it service those cases.

@jonathanknowles
Copy link
Contributor Author

jonathanknowles commented Apr 15, 2020

@paweljakubas wrote:

if we disable largest first fallback, would be good to show in testing that we can recreate old setup. by invoking random and then rely on largestFirst. And here deliver test showing that we do not have fallback (there were test like that to my best knowledge in cardano-wallet), and when having the composition with fallback it service those cases.

Thanks for raising this point!

I agree, it's very important to preserve the existing behaviour of cardano-wallet when we eventually reuse cardano-coin-selection from cardano-wallet, replacing the old code.

I've added an item to the acceptance criteria for this task here, which highlights the need for testing to make sure that fallback continues to happen as expected.

@jonathanknowles jonathanknowles merged commit 544d87f into master Apr 15, 2020
@jonathanknowles jonathanknowles deleted the jonathanknowles/disable-algorithm-fallback branch April 15, 2020 09:38
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

2 participants