Merge pull request #3429 from input-output-hk/Squad1/CBR-26/improve-error-handling

[CBR-26] Error Handling Improvements

Author: KtorZ

CHANGELOG.md

@@ -14,12 +14,22 @@
 
 [PR 3365]: https://github.com/input-output-hk/cardano-sl/pull/3365
 
+- Improve type safety (and as a consequence, API documentation) of account indexes (CBR-306)
+
 ### Improvements
 
+- Friendly error mistakes from deserializing invalid addresses instead of brutal 500 (CBR-283)
+
+- **[API BREAKING CHANGE]** Add `walletId` to `WalletAlreadyExists` WalletLayerError (CBR-254)
+
+- Small refactor of wallet Errors implementation to be more maintainable (CBR-26)
+
 ### Specifications
 
 ### Documentation
 
+- Make an inventory of existing wallet errors and exceptions (CBR-307)
+
 
 ## Cardano SL 1.3.0
 

pkgs/default.nix

@@ -17806,7 +17806,6 @@ license = stdenv.lib.licenses.mit;
 , http-client-tls
 , http-types
 , ixset-typed
-, json-sop
 , lens
 , log-warper
 , memory
@@ -17912,7 +17911,6 @@ http-client
 http-client-tls
 http-types
 ixset-typed
-json-sop
 lens
 log-warper
 memory

wallet-new/cardano-sl-wallet-new.cabal

@@ -44,6 +44,7 @@ library
                        Cardano.Wallet.API.V1.Handlers.Addresses
                        Cardano.Wallet.API.V1.Handlers.Transactions
                        Cardano.Wallet.API.V1.Handlers.Wallets
+                       Cardano.Wallet.API.V1.Headers
                        Cardano.Wallet.API.V1.Info
                        Cardano.Wallet.API.V1.LegacyHandlers
                        Cardano.Wallet.API.V1.LegacyHandlers.Accounts
@@ -183,7 +184,6 @@ library
                      , http-client-tls
                      , http-types
                      , ixset-typed
-                     , json-sop
                      , lens
                      , log-warper
                      , memory
@@ -530,7 +530,6 @@ test-suite wallet-new-specs
                     RequestSpec
                     WalletHandlersSpec
                     WalletNewJson
-                    WalletNewGen
                     Cardano.Wallet.WalletLayer.QuickCheck
 
   default-language: Haskell2010

wallet-new/integration/TransactionSpecs.hs

@@ -5,14 +5,14 @@ module TransactionSpecs (transactionSpecs) where
 
 import           Universum
 
-import           Cardano.Wallet.API.V1.Errors hiding (describe)
 import           Cardano.Wallet.Client.Http
+import           Control.Concurrent (threadDelay)
 import           Control.Lens
-import qualified Pos.Core as Core
 import           Test.Hspec
-
-import           Control.Concurrent (threadDelay)
 import           Text.Show.Pretty (ppShow)
+
+import qualified Pos.Core as Core
+
 import           Util
 
 {-# ANN module ("HLint: ignore Reduce duplication" :: Text) #-}

wallet-new/integration/WalletSpecs.hs

@@ -5,8 +5,6 @@ module WalletSpecs (walletSpecs) where
 
 import           Universum
 
-import           Cardano.Wallet.API.V1.Errors
-                     (WalletError (WalletAlreadyExists))
 import           Cardano.Wallet.Client.Http
 import           Control.Lens
 import           Test.Hspec
@@ -63,8 +61,10 @@ walletSpecs _ wc = do
                         }
             -- First wallet creation/restoration should succeed
             result <- postWallet wc newWallet1
-            void $ result `shouldPrism` _Right
+            wallet <- fmap wrData (result `shouldPrism` _Right)
             -- Second wallet creation/restoration should rise WalletAlreadyExists
             eresp <- postWallet wc newWallet2
             clientError <- eresp `shouldPrism` _Left
-            clientError `shouldBe` ClientWalletError WalletAlreadyExists
+            clientError
+                `shouldBe`
+                    ClientWalletError (WalletAlreadyExists (walId wallet))

wallet-new/src/Cardano/Wallet/API/Response.hs

@@ -1,10 +1,12 @@
 {-# LANGUAGE DeriveFunctor   #-}
 {-# LANGUAGE DeriveGeneric   #-}
+{-# LANGUAGE LambdaCase      #-}
 {-# LANGUAGE OverloadedLists #-}
 module Cardano.Wallet.API.Response (
     Metadata (..)
   , ResponseStatus(..)
   , WalletResponse(..)
+  , JSONValidationError(..)
   -- * Generating responses for collections
   , respondWith
   , fromSlice
@@ -18,18 +20,22 @@ module Cardano.Wallet.API.Response (
   ) where
 
 import           Prelude
-import           Universum (Buildable, decodeUtf8, toText, (<>))
+import           Universum (Buildable, Exception, Text, decodeUtf8, toText,
+                     (<>))
 
-import           Cardano.Wallet.API.Response.JSend (ResponseStatus (..))
-import           Cardano.Wallet.API.V1.Swagger.Example (Example, example)
 import           Control.Lens
-import           Data.Aeson
+import           Data.Aeson (FromJSON (..), ToJSON (..), eitherDecode, encode)
 import           Data.Aeson.Encode.Pretty (encodePretty)
+import qualified Data.Aeson.Options as Serokell
 import           Data.Aeson.TH
+import qualified Data.Char as Char
 import           Data.Swagger as S hiding (Example, example)
 import           Data.Typeable
 import           Formatting (bprint, build, (%))
+import qualified Formatting.Buildable
+import           Generics.SOP.TH (deriveGeneric)
 import           GHC.Generics (Generic)
+import           Servant (err400)
 import           Servant.API.ContentTypes (Accept (..), JSON, MimeRender (..),
                      MimeUnrender (..), OctetStream)
 import           Test.QuickCheck
@@ -42,14 +48,13 @@ import           Cardano.Wallet.API.Request.Pagination (Page (..),
                      PerPage (..))
 import           Cardano.Wallet.API.Request.Sort (SortOperations (..))
 import           Cardano.Wallet.API.Response.Filter.IxSet as FilterBackend
+import           Cardano.Wallet.API.Response.JSend (HasDiagnostic (..),
+                     ResponseStatus (..))
 import           Cardano.Wallet.API.Response.Sort.IxSet as SortBackend
-import           Cardano.Wallet.API.V1.Errors
-                     (WalletError (JSONValidationFailed))
-
-import qualified Data.Aeson.Options as Serokell
-import qualified Data.Char as Char
-import qualified Formatting.Buildable
-
+import           Cardano.Wallet.API.V1.Errors (ToServantError (..))
+import           Cardano.Wallet.API.V1.Generic (jsendErrorGenericParseJSON,
+                     jsendErrorGenericToJSON)
+import           Cardano.Wallet.API.V1.Swagger.Example (Example, example)
 
 -- | Extra information associated with an HTTP response.
 data Metadata = Metadata
@@ -166,7 +171,7 @@ respondWith :: (Monad m, Indexable' a)
             -> m (WalletResponse [a])
 respondWith RequestParams{..} fops sorts generator = do
     (theData, paginationMetadata) <- paginate rpPaginationParams . sortData sorts . applyFilters fops <$> generator
-    return $ WalletResponse {
+    return WalletResponse {
              wrData = theData
            , wrStatus = SuccessStatus
            , wrMeta = Metadata paginationMetadata
@@ -225,3 +230,40 @@ instance Accept ValidJSON where
 
 instance ToJSON a => MimeRender ValidJSON a where
     mimeRender _ = mimeRender (Proxy @ JSON)
+
+
+--
+-- Error from parsing / validating JSON inputs
+--
+
+newtype JSONValidationError
+    = JSONValidationFailed Text
+    deriving (Eq, Show, Generic)
+
+deriveGeneric ''JSONValidationError
+
+instance ToJSON JSONValidationError where
+    toJSON =
+        jsendErrorGenericToJSON
+
+instance FromJSON JSONValidationError where
+    parseJSON =
+        jsendErrorGenericParseJSON
+
+instance Exception JSONValidationError
+
+instance Arbitrary JSONValidationError where
+    arbitrary =
+        pure (JSONValidationFailed "JSON validation failed.")
+
+instance Buildable JSONValidationError where
+    build _ =
+        bprint "Couldn't decode a JSON input."
+
+instance HasDiagnostic JSONValidationError where
+    getDiagnosticKey _ =
+        "validationError"
+
+instance ToServantError JSONValidationError where
+    declareServantError _ =
+        err400

wallet-new/src/Cardano/Wallet/API/Response/JSend.hs

@@ -17,6 +17,13 @@ data ResponseStatus =
 
 deriveJSON defaultOptions { Data.Aeson.TH.constructorTagModifier = map Char.toLower . reverse . drop 6 . reverse } ''ResponseStatus
 
+class HasDiagnostic a where
+    getDiagnosticKey :: a -> Text
+
+noDiagnosticKey :: Text
+noDiagnosticKey =
+    error "Contructor has declared no diagnostic key but apparently requires one! Have a look at HasDiagnostic instances!"
+
 instance Arbitrary ResponseStatus where
     arbitrary = elements [minBound .. maxBound]