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.
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.
Later tasks will deal with adding new code, introducing new abstractions, and any refactoring that may be required.
cardano-coin-selection repository at https://github.com/input-output-hk/cardano-coin-selection.cardano-wallet that are relevant to coin selection and fee balancing.cardano-wallet that we eventually plan to remove, if removing it at the start would be prohibitively difficult.Cardano.CoinSelection namespace (but other namespaces are permitted).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.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:
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.
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
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
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.
Hi @paweljakubas. Thanks for your feedback on this issue!
I've responded to your points inline:
- I have seen that you imported
Data.Quantitybut 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.
- Probably
Data.Quantityand others should land incardano-auxas 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:
We can always delete or refine things later on, before we make an actual release.
- We are 100% sure that all functions included in
Cardano.Feeneeds 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.
- Would be great to extend
README.mdand 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?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 insrc/Cardano/Unsafe.hsand it is only used in the teststest/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!
Most helpful comment
@jonathanknowles few remarks/questions:
unsafeFromHex- I see there is a helper method that's that's part of the src code insrc/Cardano/Unsafe.hsand it is only used in the teststest/Cardano/TypesSpec.hs... maybe it could be moved there?cardano-walletshould also land here in this new repo.