-
Notifications
You must be signed in to change notification settings - Fork 213
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
Use monoidal arithmetic in cardano-coin-selection
.
#4110
Use monoidal arithmetic in cardano-coin-selection
.
#4110
Conversation
4e496c7
to
27fbe3d
Compare
(requiredCost <\> excessCoin) | ||
(excessCoin </> requiredCost) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm not sure I'd spot the difference very easily 🤔 Could it make it unnecessarily difficult for other people as well? I guess subtract
and monus
/ truncatedSubtraction
would be clearer? Or even the unicode character ∸
wikipedia lists... Or <->
? (like the ledger)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm not sure I'd spot the difference very easily
Thanks for mentioning this, I do sympathise with your argument.
I'd be very happy to discuss this together with you over a call. Perhaps we can work out a better solution.
One thing to mention is that at least when writing code, it's fairly hard to mix these up, since they do have very different types:
</> :: Reductive m => m -> m -> Maybe m
<\> :: Monus m => m -> m -> m
But as you point out, when reading code, it can be easy to mistake one for the other.
The way I remember <\>
is that it looks visually similar to the set subtraction operator \
, which is indeed the monus operation for sets.
<->? (like the ledger)
The ledger <->
operator is for group subtraction, which only works for things that are actually groups. Recall that a <-> b
is equivalent to a <> invert b
. But none of the types involved in this PR support a group invert
operation, so <->
is not appropriate here.
or even the unicode character ∸
We could use the ∸
operator, as you mention, but that would then deviate from the standard operator used by monoid-subclasses
, so people who are used to that library would have to adjust to this new operator.
Would be happy to discuss our options together over a call!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
lgtm modulo wanted to flag concern about ease of telling <\>
and </>
apart
27fbe3d
to
ba05330
Compare
## Issues - Follow-on from #4110 - ADP-1449 ## Description This PR replaces the use of `Coin.distance` with [`Monus.<\>`](https://hackage.haskell.org/package/monoid-subclasses-1.2.3/docs/Data-Monoid-Monus.html#v:-60--92--62-), which (in the case of `Coin` arguments) performs truncated subtraction of the second argument from the first.
## Issue Follow-on from #4110 ## Description This PR makes the following replacements within the `cardano-balance-tx` library: | Before | After | | -- | -- | | `{TokenBundle,TokenMap,TokenQuantity,Coin}.add` | `Semigroup.<>` | | `{TokenBundle,TokenMap,TokenQuantity,Coin}.subtract` | `Reductive.</>` | | `{TokenBundle,TokenMap,TokenQuantity,Coin}.difference` | `Monus.<\>` | Functions on the LHS of the above table are already just synonyms of class methods on the RHS. This takes us one small step closer to removing the dependency on types defined within `cardano-wallet-primitive`.
Issue
ADP-1449
Description
This PR makes the following replacements within the
cardano-coin-selection
library:{TokenBundle,TokenMap,TokenQuantity,Coin}.add
Semigroup.<>
{TokenBundle,TokenMap,TokenQuantity,Coin}.subtract
Reductive.</>
{TokenBundle,TokenMap,TokenQuantity,Coin}.difference
Monus.<\>
{TokenBundle,TokenMap}.empty
Monoid.mempty
{TokenQuantity,Coin}.zero
Monoid.mempty
Functions on the LHS of the above table are already just synonyms of class methods on the RHS.
This takes us one small step closer to removing the dependency on types defined within
cardano-wallet-primitive
.