Cardano-wallet: Create buildable `cardano-coin-selection` repository.

Created on 24 Feb 2020  路  7Comments  路  Source: input-output-hk/cardano-wallet

Context

As part of the work to create a separate coin selection and fee balancing library, we need to create a repository that will ultimately hold this library.

Decision

Create a repository that is based on a stripped-down version of the cardano-wallet repository, but with the history _filtered_ so that it only those commits that are relevant to coin selection and fee balancing are retained.

Non Goals

  • Introducing any new code.
  • Introducing any new abstractions.
  • Performing any refactoring apart from renaming modules.
  • Making any releases, either to Hackage or to anywhere else.

Later tasks will deal with adding new code, introducing new abstractions, and any refactoring that may be required.

Acceptance Criteria

  • [x] There _must_ be a cardano-coin-selection repository at https://github.com/input-output-hk/cardano-coin-selection.
  • [x] The repository _must_ contain all commits, code, and tests from cardano-wallet that are relevant to coin selection and fee balancing.
  • [x] The repository _may_ (to begin with) contain code and tests from cardano-wallet that we eventually plan to remove, if removing it at the start would be prohibitively difficult.
  • [x] Haskell modules _should_ be located within a Cardano.CoinSelection namespace (but other namespaces are permitted).
  • [x] The repository _must_ be buildable.
  • [x] The tests _must_ all pass.

Development

  • [x] Remove the ToText and FromText classes, and all instances. Within cardano-wallet, these are mostly use to support serialization and deserialization of values for transport across our REST API.
  • [x] Remove all tags from the repository.

Filtering History

To produce the cardano-coin-selection repository, we'll start with a _clone_ of the cardano-wallet repository, and then _filter out_ all the commits that are unrelated to coin selection.

However, producing a filtered version of the history is non-trivial.

One issue is that some files have been _renamed_ several times over the course of their history, and other files are composed of content from _multiple ancestor files_.

If we naively just filter for commits that affect the current set of files, we'll lose those commits that affect older versions of files that existed at different paths.

For example, the module Cardano.Wallet.Primitive.Types has content derived historically from all of the following paths:

lib/core/src/Cardano/Wallet/Primitive/Types.hs
src/Cardano/Wallet/Primitive.hs
src/Cardano/Wallet/Primitive/Types.hs

We'd ideally like to keep commits relating to _all_ of those paths.

In general, we'd like to keep commits for both:

  • all files of interest; and
  • all ancestors of files that are of interest.

Step 1: Remove all the files that are definitely _not_ relevant.

We start with a fresh clone of cardano-wallet.

First, we make a commit to the master branch that removes all unwanted files from the repository. The only files remaining should be those related to _coin selection_. Note that we need to be conservative here, and retain any modules that define functions used by coin selection in some way.

See here for a branch that does this.

Step 2: Identify the historic names of files that we want to keep.

Next, we generate a list of path names for all current files, as well as path names for all historical ancestor files, using the following script (find-paths.sh):

for x in $(git ls-tree -r master --name-only) 
do
  git log --follow --name-status -- $x | egrep R[0-9]+ | awk '{print $2; print $3}' | sort -u 
done

Run the script from the root of the repository, as follows:

find-paths.sh | sort -u > files-to-keep

Step 3: Filter out unwanted commits.

Finally, we filter out all commits that don't affect the list of files identified in step 2.

git filter-repo --paths-from-file files-to-keep

Step 4: Verify that nothing has been lost.

Re-run the find-paths script from the root of the repository, as follows:

find-paths.sh | sort -u > files-kept

The content of files-kept should be identical to files-to-keep.

Most helpful comment

@jonathanknowles few remarks/questions:

  • unsafeFromHex - I see there is a helper method that's that's part of the src code in src/Cardano/Unsafe.hs and it is only used in the tests test/Cardano/TypesSpec.hs... maybe it could be moved there?
  • I'm sure that you have this already planned in further tasks but to back up @paweljakubas comment about extending README, I think that it would be nice that readme also had:

    • badges: for CI (which also isn't set up yet I think), latest release, code coverage (when it is set up ofc)

    • I think that especially having code coverage would be helpful in assessing whether any tests from cardano-wallet should also land here in this new repo.

All 7 comments

Hi @paweljakubas. Thanks for your feedback on this issue!

I've responded to your points inline:

  1. I have seen that you imported Data.Quantity but didn't add corresponding test for it : https://github.com/input-output-hk/cardano-wallet/blob/master/lib/core/test/unit/Data/QuantitySpec.hs

Well spotted! That's actually intentional.

The existing QuantitySpec only has roundtrip tests for ToText/FromText instances. But these type classes (and all instances) have been _completely removed_ from the cardano-coin-selection codebase.

We decided to remove them because the textual serialization format is, in most cases, specific to the cardano-wallet API wire format. That format should be almost certainly be defined within cardano-wallet. Users cardano-coin-selection may wish to make different choices about how to serialize values of these types, and not depend on our particular choices.

In future, when we update cardano-wallet to depend on cardano-coin-selection, we may decide to define wrapper types for ToText and FromText instances. However, one other possibility (strongly hinted at in the original specification) is that we update cardano-coin-selection to use more abstract types, meaning that wrapper types would be rendered unnecessary.

  1. Probably Data.Quantity and others should land in cardano-aux as they are widely reused throughout code base

Another good point. In the future, if there is a cardano-aux repository, we can always delete it from cardano-coin-selection.

Another possibility is that we remove the Quantity type altogether, and make this abstract.

However, the current goals are merely to:

  • ensure that we haven't deleted anything essential to the purpose of the library.
  • ensure that we haven't deleted history of things we will later need.

We can always delete or refine things later on, before we make an actual release.

  1. We are 100% sure that all functions included in Cardano.Fee needs to be in this repo?

I've removed computeCertFee, as this seems to be unrelated to the original requirements. See here: https://github.com/input-output-hk/cardano-coin-selection/commit/9e27594ef106574dba4c774671170d14a4bc48b9

Is there anything else that you think should definitely be removed?

As mentioned before, we can always delete more stuff later, if we need to.

  1. Would be great to extend README.md and show link to the wallet spec proper chapter and link blog (https://iohk.io/blog/self-organisation-in-coin-selection/), also basic usage, and for example small usage session in repl.

Good points!

I will add these tasks to another issue, as this PR is mainly concerned with filtering the cardano-wallet history to create a buildable cardano-coin-selection repository.

@jonathanknowles few remarks/questions:

  • unsafeFromHex - I see there is a helper method that's that's part of the src code in src/Cardano/Unsafe.hs and it is only used in the tests test/Cardano/TypesSpec.hs... maybe it could be moved there?
  • I'm sure that you have this already planned in further tasks but to back up @paweljakubas comment about extending README, I think that it would be nice that readme also had:

    • badges: for CI (which also isn't set up yet I think), latest release, code coverage (when it is set up ofc)

    • I think that especially having code coverage would be helpful in assessing whether any tests from cardano-wallet should also land here in this new repo.

@piotr-iohk wrote:

unsafeFromHex - I see there is a helper method that's that's part of the src code in src/Cardano/Unsafe.hs and it is only used in the tests test/Cardano/TypesSpec.hs... maybe it could be moved there?

Well spotted! I've added this to https://github.com/input-output-hk/cardano-wallet/issues/1380.

@piotr-iohk wrote:

to back up @paweljakubas comment about extending README

Thanks! Added to https://github.com/input-output-hk/cardano-wallet/issues/1382

badges: for CI (which also isn't set up yet I think), latest release

Thanks! Added to the following tickets:

code coverage (when it is set up ofc)

Added here: https://github.com/input-output-hk/cardano-wallet/issues/1390

LGTM. Closing, all remarks addressed in furhter tasks.

Thanks @piotr-iohk!

Was this page helpful?
0 / 5 - 0 ratings