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

Provide a wallet-specific interface for coin selection. #3109

Merged
merged 2 commits into from
Feb 7, 2022

Conversation

jonathanknowles
Copy link
Member

@jonathanknowles jonathanknowles commented Feb 7, 2022

Issue Number

ADP-1407

Summary

This PR reorganizes the coin selection module hierarchy to have the following structure:

  • Cardano.Wallet.CoinSelection
    provides a stable, wallet-specific interface for coin selection, with wallet-friendly types such as TxIn, TxOut, and UTxO.
  • Cardano.Wallet.CoinSelection.Internal.*
    provides internal functions and types, whose definitions are allowed to deviate from those provided in the public module.

This PR also:

  • adjusts all parts of the wallet that need coin selection functionality to import from Cardano.Wallet.CoinSelection, rather than Cardano.Wallet.CoinSelection.Internal.
  • adjusts the names of some error types and constructors to avoid name collisions.

@jonathanknowles jonathanknowles marked this pull request as draft February 7, 2022 05:22
@jonathanknowles jonathanknowles force-pushed the jonathanknowles/cardano-wallet-coin-selection branch 5 times, most recently from 4859110 to 9c31da8 Compare February 7, 2022 07:16
This commit provides the module `Cardano.Wallet.CoinSelection`, which
provides a wallet-specific interface for coin selection.

Wallet features requiring coin selection functionality should import
from this module.
This commit:

1. Renames `Primitive.CoinSelection` to `CoinSelection.Internal`, making it
   more obvious that this module is an "internal" module.

2. Adds a disclaimer to the top of the module advising people to import
   from the "public" module instead.
@jonathanknowles jonathanknowles force-pushed the jonathanknowles/cardano-wallet-coin-selection branch from 9c31da8 to 72dd574 Compare February 7, 2022 07:20
@jonathanknowles jonathanknowles changed the title WIP: Provide a wallet-specific interface for coin selection. Provide a wallet-specific interface for coin selection. Feb 7, 2022
@jonathanknowles jonathanknowles self-assigned this Feb 7, 2022
@jonathanknowles jonathanknowles marked this pull request as ready for review February 7, 2022 08:12
@jonathanknowles
Copy link
Member Author

bors try

iohk-bors bot added a commit that referenced this pull request Feb 7, 2022
@iohk-bors
Copy link
Contributor

iohk-bors bot commented Feb 7, 2022

try

Build failed:

Failure:

x86_64-linux-nix/Cabal-simple_mPHDZzAJ_3.2.1.0_ghc-8.10.7:
error while loading shared libraries:
libnuma.so.1: cannot open shared object file: No such file or directory

While building package cardano-wallet-core-2022.1.18 (scroll up to its section to see the error) using:

Copy link
Contributor

@HeinrichApfelmus HeinrichApfelmus left a comment

Choose a reason for hiding this comment

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

I like it, looks good to me! Having a single module Cardano.Wallet.CoinSelection that exposes "everything you need to know about coin selection" encourages us to keep the abstraction boundary clean and simple — the "internals that you do not need to know about coin selection" are not exposed.

@jonathanknowles
Copy link
Member Author

bors r+

@iohk-bors
Copy link
Contributor

iohk-bors bot commented Feb 7, 2022

Build succeeded:

@iohk-bors iohk-bors bot merged commit 8bc24b7 into master Feb 7, 2022
@iohk-bors iohk-bors bot deleted the jonathanknowles/cardano-wallet-coin-selection branch February 7, 2022 14:54
WilliamKingNoel-Bot pushed a commit that referenced this pull request Feb 7, 2022
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