[CO-413] Removing wallet-new/src/Cardano/Wallet/Orphans*

[paweljakubas]

Description

Removing :

1. wallet-new/src/Cardano/Wallet/Orphans.hs
2. wallet-new/src/Cardano/Wallet/Orphans/Aeson.hs
3. wallet-new/src/Cardano/Wallet/Orphans/Arbitrary.hs
4. wallet-new/src/Cardano/Wallet/Orphans/Bi.hs
   and adapting the rest code to work as before.

Linked issue

https://iohk.myjetbrains.com/youtrack/issue/CO-413

Type of change

• 🐞 Bug fix (non-breaking change which fixes an issue)
• 🛠 New feature (non-breaking change which adds functionality)
• ⚠️ Breaking change (fix or feature that would cause existing functionality to change)
• 🏭 Refactoring that does not change existing functionality but does improve things like code readability, structure etc
• 🔨 New or improved tests for existing code
• ⛑ git-flow chore (backport, hotfix, etc)

Developer checklist

• I have read the style guide document, and my code follows the code style of this project.
• If my code deals with exceptions, it follows the guidelines.
• I have updated any documentation accordingly, if needed. Documentation changes can be reflected in opening a PR on cardanodocs.com, amending the inline Haddock comments, any relevant README file or one of the document listed in the docs directory.
• CHANGELOG entry has been added and is linked to the correct PR on GitHub.

Testing checklist

• I have added tests to cover my changes.
• All new and existing tests passed.

QA Steps

Screenshots (if available)

[paweljakubas]

In the logs of integration tests (https://hydra.iohk.io/build/267893/log) I have:

  fails if you spend more money than your available balance
    +++ OK, passed 1 tests.
  fails if you can't cover fee with a transaction FAILED [1]
  posted transactions gives rise to nonempty Utxo histogram
    +++ OK, passed 1 tests.
Servant API Properties
  V0 API follows best practices & is RESTful abiding
  V1 API follows best practices & is RESTful abiding

Failures:

  integration/Util.hs:129:5: 
  1) Transactions fails if you can't cover fee with a transaction
       Falsifiable (after 1 test):
       
       predicate failed on: Right (Transaction {txId = AbstractHash 614306d0d39574a32fac99c4ffbd002202c0fc770c1ca9ca44f434904893b994, txConfirmations = 0, txAmount = Coin {getCoin = 42}, txInputs = PaymentDistribution {pdAddress = Address {addrRoot = AbstractHash 383bd170e15aad904538e99e105fb699d24c6420e5421308fa0eca6d, addrAttributes = Attributes { data: AddrAttributes {aaPkDerivationPath = Just (HDAddressPayload {getHDAddressPayload = "\235\159\155\STX\DC3Y\174\DLEU\EOT\233\228\221\235\246\205\NAK\146\201\vKv\FS\NAK\154e\151\DC3"}), aaStakeDistribution = BootstrapEraDistr} }, addrType = ATPubKey}, pdAmount = Coin {getCoin = 42}} :| [], txOutputs = PaymentDistribution {pdAddress = Address {addrRoot = AbstractHash cdc975e0526c75e9ec8d86cfbe5895651c18c72da50ba122f128f4ab, addrAttributes = Attributes { data: AddrAttributes {aaPkDerivationPath = Just (HDAddressPayload {getHDAddressPayload = "\150[*9\221\152\221\190m\177\ACK\135\US\251+\STX\ENQ\181\NUL\190g\162k\SUBd\152\DC1\135"}), aaStakeDistribution = BootstrapEraDistr} }, addrType = ATPubKey}, pdAmount = Coin {getCoin = 42}} :| [], txType = ForeignTransaction, txDirection = OutgoingTransaction, txCreationTime = 1538646407869634, txStatus = Applying})

Randomized with seed 421818342

Finished in 589.6528 seconds
21 examples, 1 failure

Maybe it is because we do not have this included: #3704 ???

[KtorZ]

🤔 Where does that come from? Why using the underlying Show instance here, that feels wrong.
Plus, it's another wild orphan as we are supposed to removed them 😅

[paweljakubas]

Frankly speaking I got it from:

wallet/src/Pos/Wallet/Web/ClientTypes/Types.hs
675:instance Buildable Version where

[paweljakubas]

shown is from Formatting(shown)

[KtorZ]

Yup, this I understand ^.^ ... My question was more like: Why do we need this instance in a first place? Where is this Version used?

If it's only in:

-- | The @static@ settings for this wallet node. In particular, we could group
-- here protocol-related settings like the slot duration, the transaction max size,
-- the current software version running on the node, etc.
data NodeSettings = NodeSettings {
     setSlotDuration   :: !SlotDuration
   , setSoftwareInfo   :: !(V1 Core.SoftwareVersion)
   , setProjectVersion :: !Version
   , setGitRevision    :: !Text
   } deriving (Show, Eq, Generic)

I'd rather stick a V1 Version here and define a non-orphan instance for V1 Version. This shouldn't break much thing and following the compiler steps will be easy.

[KtorZ]

We gotta need a roundtrip Aeson tests for that one 👍

[paweljakubas]

certainly

[paweljakubas]

there is already in wallet-new/test/MarshallingSpec.hs line 95:

httpApiDataRoundtripProp @Core.Coin Proxy

but we are missing this one (although have aesonRoundtripProp @(V1 Core.Coin) Proxy):

aesonRoundtripProp @Core.Coin Proxy

But if we test aeson roundtrips of V1 Something do we need to check aeson roundtrips of Something ?

[KtorZ]

Sorry, I meant the httpAPIData round trip of course :/ ...

I didn't realize at first this was also (again) an orphan instance. I think we want instances to be on V1 Core.Coin here (like the others above). In such case (as for aeson instances, there's no point of doing a rountrip test on the non-wrapped type (Core.Coin) itself, because we do want to test and use the wrapped instance. And chances are that there's not even an instance defined for the unwrapped type, which is good.

May you do this change 🙏 ?

[paweljakubas]

will be addressed in https://iohk.myjetbrains.com/youtrack/issue/CO-422 for reasons specified therein

[KtorZ]

Meh, still one orphan exchanged for another 😞 ...
Would it be too much work to go for a newtype here, or at leat pin-point the place where it's actually needed ?

[paweljakubas]

[7 of 9] SwaggerSpec

/home/pawel/IOHK/cardano-sl/wallet-new/test/SwaggerSpec.hs:57:13: error:
    • No instance for (Arbitrary NoContent)
        arising from a use of ‘validateEveryToJSON'’
    • In the second argument of ‘($)’, namely
        ‘validateEveryToJSON' (Proxy @V1.API)’
      In the second argument of ‘($)’, namely
        ‘describe "(V1) ToJSON matches ToSchema"
           $ validateEveryToJSON' (Proxy @V1.API)’
      In a stmt of a 'do' block:
        parallel
          $ describe "(V1) ToJSON matches ToSchema"
              $ validateEveryToJSON' (Proxy @V1.API)
   |
57 |             validateEveryToJSON' (Proxy @ V1.API)
   |             ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^

[parsonsmatt]

Arbitrary instances should live with their type definitions IMO.

Otherwise, orphan arbitrary instances are one of the few justifiable ones, even to the Anti Orphan Squad

[KtorZ]

Yup. It seems to be actually the "accepted pattern" here (in IOHK) to have orphan Arbitrary instances defined in test code only (even though, I tend to prefer having everything about a type defined next to the type itself).

In that specific case, since we need this Arbitrary only in the SwaggerSpec, let's make it an orphan instance there and not in V1.Types.

[paweljakubas]

Summary:

1. Adopting V1 Version everywhere instead of Version
2. Relocated NoContent instance of Arbitrary to SwaggerSpecs (there is already ToSchema instance there)
3. As Core.Coin is needed in LegacyHandlers another ticket was created that will deal with it when LegacyHandlers are cleaned

[KtorZ]

What's with the reverse 😕 ?

   parseJSON (String v) = case readP_to_S parseVersion (T.unpack v) of
        [(ver, _)] -> pure (V1 ver)
        _          -> mempty

Isn't that enough ?

[paweljakubas]

Prelude> import Data.Version
Prelude Data.Version> let v = makeVersion [1,2,3]
Prelude Data.Version> v
Version {versionBranch = [1,2,3], versionTags = []}
Prelude Data.Version> import Text.ParserCombinators.ReadP
Prelude Data.Version Text.ParserCombinators.ReadP> let out = readP_to_S parseVersion (showVersion v)
Prelude Data.Version Text.ParserCombinators.ReadP> out
[(Version {versionBranch = [1], versionTags = []},".2.3"),(Version {versionBranch = [1,2], versionTags = []},".3"),(Version {versionBranch = [1,2,3], versionTags = []},"")]

I need the last element. And I can have also [1,2] - hence the need to pattern match against the last element in the list

[KtorZ]

Arf. Okay.

[KtorZ]

Where does the Arbitrary Version comes from ?

[paweljakubas]

http://hackage.haskell.org/package/QuickCheck-2.12.6.1/docs/src/Test.QuickCheck.Arbitrary.html#line-927

[KtorZ]

Ho :o !
That's a surprise. Great 🎉

[KtorZ]

Approving 👍
Though I believe the ViewPattern is overkill here 😅 ... We could get away with a simple pattern-match!