Skip to content

Commit

Permalink
Merge #2258
Browse files Browse the repository at this point in the history
2258: ADP-291: Document endpoints error codes in the API documentation r=hasufell a=hasufell

# Issue Number

ADP-291

# Implementation approach

This is all best-effort. There are no static checks to ensure we didn't miss anything. There's ongoing work at servant to make this possible: haskell-servant/servant#841
But even that would require a major refactor.

Testing all possible errors is also an option, but seems quite excessive.

The workflow for figuring out the error codes is roughly:

1. follow the entry point to the the `Handler ()` function e.g.
```hs
listTransactions
    :: forall ctx s t k n. (ctx ~ ApiLayer s t k)
    => ctx
...
    -> Handler [ApiTransaction n]
```
2. check all `liftHandler` calls, which have functions with `ExceptT` as argument, e.g.:

```hs
listTransactions
    :: forall ctx s k t.
        ( HasDBLayer s k ctx
        , HasNetworkLayer t ctx
        )
    => ctx
...
    -> ExceptT ErrListTransactions IO [TransactionInfo]
```

3. find all the LiftHandler instance, e.g.

```hs
instance LiftHandler ErrListTransactions where
    handler = \case
        ErrListTransactionsNoSuchWallet e -> handler e
        ErrListTransactionsStartTimeLaterThanEndTime e -> handler e
        ErrListTransactionsMinWithdrawalWrong ->
            apiError err400 MinWithdrawalWrong
            "The minimum withdrawal value must be at least 1 Lovelace."
        ErrListTransactionsPastHorizonException e -> handler e
```

4. follow the recursive handlers to resolve all errors and add client error types to `swagger.yaml`. The error code is the 3rd `ApiErrorCode` argument to `apiError`.
5. Some errors are implicit, e.g. failed parameter parsing etc. These are also in `ApiErrorCode`. Also see the special instance `instance LiftHandler (Request, ServerError)`
6. repeat until exhaustion

# Overview

- [x] Wallets
- [x] Addresses
- [x] Coin Selections
- [x] Transactions
- [x] Migrations
- [x] Stake Pools
- [x] Utils
- [x] Network
- [x] Proxy
- [x] Settings
- [ ] Byron-specific endpoints

# Remarks

1. I did not double check the byron endpoints. Many of their endpoints share the same types in `swagger.yaml`, so I assumed they have the same behavior wrt error codes.
2. This will generally be hard to maintain, since yaml anchors aren't enough to express the overlaps of error codes and group them sensibly. It would need something like [dhall](https://github.com/dhall-lang/dhall-lang) to do that.
3. I removed 405 errors, because the HTTP method is part of the very spec. If you follow the spec, you can't get 405.

<!-- 
Don't forget to:

 ✓ Self-review your changes to make sure nothing unexpected slipped through
 ✓ Assign yourself to the PR
 ✓ Assign one or several reviewer(s)
 ✓ Once created, link this PR to its corresponding ticket
 ✓ Assign the PR to a corresponding milestone
 ✓ Acknowledge any changes required to the Wiki
-->


Co-authored-by: Julian Ospald <julian.ospald@iohk.io>
Co-authored-by: KtorZ <matthias.benkort@gmail.com>
  • Loading branch information
3 people authored Oct 30, 2020
2 parents 2a052d0 + 7225066 commit 366c007
Show file tree
Hide file tree
Showing 6 changed files with 794 additions and 126 deletions.
Original file line number Diff line number Diff line change
Expand Up @@ -68,7 +68,6 @@ module Test.Integration.Framework.TestData
, errMsg500
, errMsg400NumberOfWords
, errMsgNotInDictionary
, errMsg403RejectedTip
, errMsg400MinWithdrawalWrong
, errMsg403WithdrawalNotWorth
, errMsg403NotAShelleyWallet
Expand Down Expand Up @@ -427,12 +426,6 @@ errMsgNotInDictionary = "Found an unknown word not present in the pre-defined\
errMsg400NumberOfWords :: String
errMsg400NumberOfWords = "Invalid number of words:"

errMsg403RejectedTip :: String
errMsg403RejectedTip =
"I am sorry but I refuse to rollback to the given point. \
\Notwithstanding I'll willingly rollback to the genesis point (0, 0) \
\should you demand it."

errMsg403WithdrawalNotWorth :: String
errMsg403WithdrawalNotWorth =
"I've noticed that you're requesting a withdrawal from an account that is \
Expand Down
3 changes: 0 additions & 3 deletions lib/core/src/Cardano/Wallet.hs
Original file line number Diff line number Diff line change
Expand Up @@ -2451,7 +2451,6 @@ data ErrStartTimeLaterThanEndTime = ErrStartTimeLaterThanEndTime
data ErrSelectForDelegation
= ErrSelectForDelegationNoSuchWallet ErrNoSuchWallet
| ErrSelectForDelegationFee ErrAdjustForFee
| ErrSelectForDelegationUnableToAssignInputs ErrNoSuchWallet
deriving (Show, Eq)

-- | Errors that can occur when signing a delegation certificate.
Expand All @@ -2468,7 +2467,6 @@ data ErrJoinStakePool
| ErrJoinStakePoolSignDelegation ErrSignDelegation
| ErrJoinStakePoolSubmitTx ErrSubmitTx
| ErrJoinStakePoolCannotJoin ErrCannotJoin
| ErrJoinStakePoolUnableToAssignInputs CoinSelection
deriving (Generic, Eq, Show)

data ErrQuitStakePool
Expand All @@ -2477,7 +2475,6 @@ data ErrQuitStakePool
| ErrQuitStakePoolSignDelegation ErrSignDelegation
| ErrQuitStakePoolSubmitTx ErrSubmitTx
| ErrQuitStakePoolCannotQuit ErrCannotQuit
| ErrQuitStakePoolUnableToAssignInputs CoinSelection
deriving (Generic, Eq, Show)

-- | Errors that can occur when fetching the reward balance of a wallet
Expand Down
21 changes: 0 additions & 21 deletions lib/core/src/Cardano/Wallet/Api/Server.hs
Original file line number Diff line number Diff line change
Expand Up @@ -2239,9 +2239,6 @@ data ErrCreateWallet
-- ^ Somehow, we couldn't create a worker or open a db connection
deriving (Eq, Show)

newtype ErrRejectedTip = ErrRejectedTip ApiSlotReference
deriving (Eq, Show)

-- | Small helper to easy show things to Text
showT :: Show a => a -> Text
showT = T.pack . show
Expand All @@ -2262,15 +2259,6 @@ instance LiftHandler ErrUnexpectedPoolIdPlaceholder where
where
Left msg = fromText @PoolId "INVALID"

instance LiftHandler ErrRejectedTip where
handler = \case
ErrRejectedTip {} ->
apiError err403 RejectedTip $ mconcat
[ "I am sorry but I refuse to rollback to the given point. "
, "Notwithstanding I'll willingly rollback to the genesis point "
, "(0, 0) should you demand it."
]

instance LiftHandler ErrSelectForMigration where
handler = \case
ErrSelectForMigrationNoSuchWallet e -> handler e
Expand Down Expand Up @@ -2621,7 +2609,6 @@ instance LiftHandler ErrSelectForDelegation where
[ "I'm unable to select enough coins to pay for a "
, "delegation certificate. I need: ", showT cost, " Lovelace."
]
ErrSelectForDelegationUnableToAssignInputs e -> handler e

instance LiftHandler ErrSignDelegation where
handler = \case
Expand Down Expand Up @@ -2656,10 +2643,6 @@ instance LiftHandler ErrJoinStakePool where
[ "I couldn't find any stake pool with the given id: "
, toText pid
]
ErrJoinStakePoolUnableToAssignInputs e ->
apiError err500 UnableToAssignInputOutput $ mconcat
[ "I'm unable to assign inputs from coin selection: "
, pretty e]

instance LiftHandler ErrFetchRewards where
handler = \case
Expand Down Expand Up @@ -2696,10 +2679,6 @@ instance LiftHandler ErrQuitStakePool where
, "account! Make sure to withdraw your ", pretty rewards
, " lovelace first."
]
ErrQuitStakePoolUnableToAssignInputs e ->
apiError err500 UnableToAssignInputOutput $ mconcat
[ "I'm unable to assign inputs from coin selection: "
, pretty e]

instance LiftHandler ErrCreateRandomAddress where
handler = \case
Expand Down
15 changes: 8 additions & 7 deletions lib/core/src/Cardano/Wallet/Api/Types.hs
Original file line number Diff line number Diff line change
@@ -1,5 +1,6 @@
{-# LANGUAGE AllowAmbiguousTypes #-}
{-# LANGUAGE DataKinds #-}
{-# LANGUAGE DeriveDataTypeable #-}
{-# LANGUAGE DeriveFunctor #-}
{-# LANGUAGE DeriveGeneric #-}
{-# LANGUAGE DerivingStrategies #-}
Expand Down Expand Up @@ -245,6 +246,8 @@ import Data.ByteArray.Encoding
( Base (Base16), convertFromBase, convertToBase )
import Data.ByteString
( ByteString )
import Data.Data
( Data )
import Data.Either.Extra
( maybeToEither )
import Data.Function
Expand Down Expand Up @@ -277,6 +280,8 @@ import Data.Time.Clock
( NominalDiffTime, UTCTime )
import Data.Time.Text
( iso8601, iso8601ExtendedUtc, utcTimeFromText, utcTimeToText )
import Data.Typeable
( Typeable )
import Data.Word
( Word16, Word32, Word64 )
import Data.Word.Odd
Expand Down Expand Up @@ -778,7 +783,6 @@ data ApiErrorCode
| MalformedTxPayload
| KeyNotFoundForAddress
| NotEnoughMoney
| UtxoNotEnoughFragmented
| TransactionIsTooBig
| InputsDepleted
| CannotCoverFee
Expand All @@ -792,18 +796,15 @@ data ApiErrorCode
| NotFound
| MethodNotAllowed
| NotAcceptable
| StartTimeLaterThanEndTime
| UnableToDetermineCurrentEpoch
| UnsupportedMediaType
| UnexpectedError
| StartTimeLaterThanEndTime
| UnableToDetermineCurrentEpoch
| NotSynced
| NothingToMigrate
| NoSuchPool
| PoolAlreadyJoined
| NotDelegatingTo
| InvalidRestorationParameters
| RejectedTip
| InvalidDelegationDiscovery
| NotImplemented
| WalletNotResponding
| AddressAlreadyExists
Expand All @@ -817,7 +818,7 @@ data ApiErrorCode
| PastHorizon
| UnableToAssignInputOutput
| SoftDerivationRequired
deriving (Eq, Generic, Show)
deriving (Eq, Generic, Show, Data, Typeable)

-- | Defines a point in time that can be formatted as and parsed from an
-- ISO 8601-compliant string.
Expand Down
54 changes: 52 additions & 2 deletions lib/core/test/unit/Cardano/Wallet/Api/TypesSpec.hs
Original file line number Diff line number Diff line change
@@ -1,5 +1,6 @@
{-# LANGUAGE AllowAmbiguousTypes #-}
{-# LANGUAGE DataKinds #-}
{-# LANGUAGE DeriveGeneric #-}
{-# LANGUAGE DerivingStrategies #-}
{-# LANGUAGE DuplicateRecordFields #-}
{-# LANGUAGE FlexibleContexts #-}
Expand All @@ -8,6 +9,7 @@
{-# LANGUAGE NoMonomorphismRestriction #-}
{-# LANGUAGE NumericUnderscores #-}
{-# LANGUAGE OverloadedLabels #-}
{-# LANGUAGE PolyKinds #-}
{-# LANGUAGE QuasiQuotes #-}
{-# LANGUAGE RankNTypes #-}
{-# LANGUAGE ScopedTypeVariables #-}
Expand Down Expand Up @@ -54,6 +56,7 @@ import Cardano.Wallet.Api.Types
, ApiCoinSelectionInput (..)
, ApiCoinSelectionOutput (..)
, ApiEpochInfo (..)
, ApiErrorCode (..)
, ApiFee (..)
, ApiMnemonicT (..)
, ApiNetworkClock (..)
Expand Down Expand Up @@ -182,17 +185,28 @@ import Cardano.Wallet.Unsafe
import Control.Lens
( at, (.~), (?~), (^.) )
import Control.Monad
( forM_, replicateM )
( forM, forM_, replicateM )
import Control.Monad.IO.Class
( liftIO )
import Crypto.Hash
( hash )
import Data.Aeson
( FromJSON (..), ToJSON (..), (.=) )
( FromJSON (..)
, Result (..)
, ToJSON (..)
, fromJSON
, withObject
, (.:?)
, (.=)
)
import Data.Aeson.QQ
( aesonQQ )
import Data.Char
( toLower )
import Data.Data
( dataTypeConstrs, dataTypeOf, showConstr )
import Data.Either
( lefts )
import Data.FileEmbed
( embedFile, makeRelativeToProject )
import Data.Function
Expand Down Expand Up @@ -272,6 +286,7 @@ import Test.QuickCheck
, arbitraryPrintableChar
, arbitrarySizedBoundedIntegral
, choose
, counterexample
, elements
, frequency
, oneof
Expand Down Expand Up @@ -929,6 +944,41 @@ spec = do
in
x' === x .&&. show x' === show x

describe "Api Errors" $ do
it "Every constructor from ApiErrorCode has a corresponding type in the schema" $
let res = fromJSON @SchemaApiErrorCode specification
errStr = case res of
Error s -> s
_ -> ""
in counterexample errStr $ res == Success SchemaApiErrorCode

{-------------------------------------------------------------------------------
Error type Encoding
-------------------------------------------------------------------------------}

-- | We use this empty data type to define a custom
-- JSON instance that checks ApiErrorCode has corresponding
-- constructors in the schema file.
data SchemaApiErrorCode = SchemaApiErrorCode
deriving (Show, Eq)

instance FromJSON SchemaApiErrorCode where
parseJSON = withObject "SchemaApiErrorCode" $ \o -> do
vals <- forM (fmap showConstr $ dataTypeConstrs $ dataTypeOf NoSuchWallet)
$ \n -> do
(r :: Maybe Yaml.Value) <- o .:? T.pack (toSchemaName n)
pure $ maybe (Left n) Right r
case lefts vals of
[] -> pure SchemaApiErrorCode
xs -> fail ("Missing ApiErrorCode constructors for: "
<> show xs
<> "\nEach of these need a corresponding swagger type of the form: "
<> "x-errConstructorName")
where
toSchemaName :: String -> String
toSchemaName [] = []
toSchemaName xs = "x-err" <> xs

{-------------------------------------------------------------------------------
Address Encoding
-------------------------------------------------------------------------------}
Expand Down
Loading

0 comments on commit 366c007

Please sign in to comment.