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

Generalize types within cardano-coin-selection library. #17

Closed
9 tasks done
jonathanknowles opened this issue Feb 26, 2020 · 3 comments
Closed
9 tasks done

Generalize types within cardano-coin-selection library. #17

jonathanknowles opened this issue Feb 26, 2020 · 3 comments
Assignees

Comments

@jonathanknowles
Copy link
Contributor

jonathanknowles commented Feb 26, 2020

Context

The cardano-coin-selection library currently duplicates some of the types that are defined within the cardano-wallet library.

It would be desirable to replace these duplicated types with abstract types or interfaces, allowing consumers of the library (including cardano-wallet) to use their own concrete types.

Decision

Revise the types used in the cardano-coin-selection library, replacing concrete types with abstract types (or interfaces) where it makes sense.

Types to Generalize

  • Cardano.CoinSelection.CoinSelectionLimit
    This was originally defined in terms of Word8. It has been widened to Word16.

  • TxIn (used in the inputs of a CoinSelection)
    Since coin selection algorithms are agnostic to the type of input identifier, this has been replaced with the type variable i.

  • Address (used in the outputs of a CoinSelection)
    Since coin selection algorithms are agnostic to the type of output identifier, this has been replaced with a type variable o.

  • UTxO
    This has been replaced with CoinMap i, where i refers to an input identifier type.

  • [TxOut]
    This has been replaced with CoinMap o, where o refers to an output identifier type.

  • Hash
    This has been removed, as it was part of TxIn which has also been removed.

  • Coin
    This was originally defined in terms of Word64, and has now been redefined in terms of Natural.

  • Fee
    This was originally defined in terms of Word64, and has now been redefined in terms of Coin (which is defined in terms of Natural).

  • DustThreshold
    This was originally defined in terms of Word64, and has now been redefined in terms of Coin (which is defined in terms of Natural).

@jonathanknowles jonathanknowles changed the title Generalize types within cardano-coin-selection library. Generalize cardano-coin-selection library types. Feb 26, 2020
@jonathanknowles jonathanknowles changed the title Generalize cardano-coin-selection library types. Generalize types within cardano-coin-selection library. Feb 26, 2020
@jonathanknowles jonathanknowles self-assigned this Feb 27, 2020
@KtorZ KtorZ transferred this issue from cardano-foundation/cardano-wallet Mar 20, 2020
@jonathanknowles
Copy link
Contributor Author

jonathanknowles commented Apr 23, 2020

@KtorZ A previous code review identified one type that could potentially be widened or generalized: https://github.com/input-output-hk/cardano-coin-selection/blob/0dfd846c68669d60fdff4f77dc8ae19ae25bf0e1/src/library/Cardano/CoinSelection.hs#L278-L283

The justification being that some blockchains might accept more than 255 inputs in a transaction.

I did experiment with widening this type, and was successfully able to widen it to Word16 without any issue (see this branch).

Of course, this value can probably be no higher than maxBound :: Int, as that is the maximum size of a Data.Map.

However, widening it to Int (or Word) causes the test for idealBatchSize to run for an excessively long time (due to the way that fixPoint increments by 1 each iteration).

What should we do here?

  1. Leave it as Word8.
  2. Change it to Word16 by making a PR out of this branch.
  3. Something else?

@jonathanknowles
Copy link
Contributor Author

Just for the record, we decided to widen the type of CoinSelectionLimit to Word16. See #17 (comment).

@piotr-iohk
Copy link
Contributor

lgtm.

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

No branches or pull requests

2 participants