Fork the Selection type.
#3111
Merged
Fork the Selection type.
#3111
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
The `SelectionReport` type exists purely in order to provide the wallet with a way to write a report about a selection to the log. It's not required or used by the internal coin selection library, and therefore it can be moved to the wallet-specific coin selection module.
The original `Selection` type was polymorphic in the type of change
outputs. The intention behind this was to allow the wallet to reuse the
`Selection` type in different contexts, both before and after assigning
change outputs with addresses.
However, the internal coin selection library is not concerned with
assigning addresses to change outputs. Assigning change addresses is a
function of the wallet, rather than the coin selection library, and the
coin selection library should not have to be concerned with this detail.
Moreover, requiring the internal coin selection library's `Selection` type
to be polymorphic in the type of change outputs makes the internal
library functions and types more complicated than necessary.
This change forks the `Selection` type into two separate types:
- A wallet-specific type,
located in `Cardano.Wallet.CoinSelection`,
where the type of `change` is kept polymorphic (as it currently is).
- An internal type,
located in `Cardano.Wallet.CoinSelection.Internal`,
where the type of `change` is simply `TokenBundle`.
We also provide a pair of functions `to{External,Internal}Selection`
located in `Cardano.Wallet.CoinSelection` to handle conversion between these
two types.
The forking of this type into two will make it easier to make further
simplifications to the internal type, as we can restrict any breakage
to the `to{External,Internal}Selection` functions.
jonathanknowles
force-pushed
the
jonathanknowles/fork-selection-type
branch
from
6634598 to
144e43c
Compare
HeinrichApfelmus
approved these changes
Feb 8, 2022
Yes, it's currently overestimated! If it turns out to be necessary in future, we could introduce an abstraction that gives the "cost" of a particular input, or the cost of a particular "address", hopefully without the library needing to know about the |
|
bors r+ |
iohk-bors bot
added a commit
that referenced
this pull request
Feb 8, 2022
3111: Fork the `Selection` type. r=jonathanknowles a=jonathanknowles ## Issue Number ADP-1410 ## Background The original `Selection` type was **_polymorphic_** in the type of **_change outputs_**. The intention behind this was to allow the wallet to reuse the `Selection` type in different contexts, _both_ **before** _and_ **after** assigning addresses to change outputs. However, the internal coin selection library is **not** concerned with assigning addresses to change outputs. Assigning change addresses is a function of the **wallet**, rather than the coin selection library, and the coin selection library shouldn't have to be concerned with this detail. Moreover, requiring the internal coin selection library's `Selection` type to be polymorphic in the type of change outputs makes the internal library functions and types more complicated than necessary. ## Primary Changes This PR: - [x] Forks the `Selection` type into two separate types: 1. A wallet-specific type, located in and exported from `Cardano.Wallet.CoinSelection` where the type of `change` is kept polymorphic (as is currently the case). 2. An internal type, located in and exported from `Cardano.Wallet.CoinSelection.Internal` where the type of `change` is just `[TokenBundle]`. - [x] Provides a pair of functions `to{External,Internal}Selection` within `Cardano.Wallet.CoinSelection` to handle conversion between these two types. ## Additional Changes This PR also: - [x] Moves the `SelectionReport` type (and related functions) to the wallet-specific `Cardano.Wallet.CoinSelection` module, as this is only required by the wallet, and not required by the internal coin selection library. ## Motivation - Forking the `Selection` type will make it easier to make further simplifications to the internal type, as we can restrict any breakage to the `to{External,Internal}Selection` functions. - Retaining the original wallet-specific type means that we don’t have to make any changes to wallet code (which relies on `change` being polymorphic) as part of the library extraction process. If we eventually want to change the wallet-specific type to something else, we can change it later on. Co-authored-by: Jonathan Knowles <jonathan.knowles@iohk.io>
|
Build failed: Encountered error while migrating Pantry database:
SQLite3 returned ErrorNotFound while attempting to perform prepare "SELECT sql FROM sqlite_master WHERE type='table' AND name=?": database disk image is malformed
Please report this on https://github.com/commercialhaskell/stack/issues
As a workaround you may delete Pantry database in /build/cardano-wallet.stack/pantry/pantry.sqlite3 triggering its recreation.
error: Command exited with code 1! |
|
bors r+ |
|
Build succeeded: |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Issue Number
ADP-1410
Background
The original
Selectiontype was polymorphic in the type of change outputs. The intention behind this was to allow the wallet to reuse theSelectiontype in different contexts, both before and after assigning addresses to change outputs.However, the internal coin selection library is not concerned with assigning addresses to change outputs. Assigning change addresses is a function of the wallet, rather than the coin selection library, and the coin selection library shouldn't have to be concerned with this detail.
Moreover, requiring the internal coin selection library's
Selectiontype to be polymorphic in the type of change outputs makes the internal library functions and types more complicated than necessary.Primary Changes
This PR:
Selectiontype into two separate types:located in and exported from
Cardano.Wallet.CoinSelectionwhere the type of
changeis kept polymorphic (as is currently the case).located in and exported from
Cardano.Wallet.CoinSelection.Internalwhere the type of
changeis just[TokenBundle].to{External,Internal}SelectionwithinCardano.Wallet.CoinSelectionto handle conversion between these two types.Additional Changes
This PR also:
SelectionReporttype (and related functions) to the wallet-specificCardano.Wallet.CoinSelectionmodule, as this is only required by the wallet, and not required by the internal coin selection library.Motivation
Selectiontype will make it easier to make further simplifications to the internal type, as we can restrict any breakage to theto{External,Internal}Selectionfunctions.changebeing polymorphic) as part of the library extraction process. If we eventually want to change the wallet-specific type to something else, we can change it later on.