Machine-readable shared wallet API errors. #3693
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
jonathanknowles
force-pushed
the
jonathanknowles/shared-wallet-error-types
branch
from
71aaf99
to
a72e41f
Compare
jonathanknowles
changed the title
SharedWalletNoSuchCosigner API error.SharedWalletNoSuchCosigner API error.
jonathanknowles
changed the title
SharedWalletNoSuchCosigner API error.SharedWalletNoSuchCosigner API error machine readable.
jonathanknowles
changed the title
SharedWalletNoSuchCosigner API error machine readable.SharedWalletNoSuchCosigner machine readable.
jonathanknowles
changed the title
SharedWalletNoSuchCosigner machine readable.SharedWalletNoSuchCosigner API error.
jonathanknowles
force-pushed
the
jonathanknowles/shared-wallet-error-types
branch
from
97a692a
to
77961b2
Compare
Strictly speaking, this is completely unrelated to the purpose of this PR, but since this PR will touch the `ApiErrorInfo` type, it makes sense to fix this while we're here.
Though the `QuickCheck` library provides a handy `arbitraryBoundedEnum` function, it currently doesn't provide a `shrinkBoundedEnum` counterpart. Users have asked for a function like this here: nick8325/quickcheck#343 This commit adds our own version, for now. Perhaps, later on, we can submit a PR to the `QuickCheck` repository to add this function, and delete our own version.
Later on, this type will be used within the `ApiErrorInfo` type, so it has to provide an instance of `Data`. Since the underlying `CredentialType` type does not provide an instance of `Data`, we add that too.
This commit parameterises the `SharedWalletNoSuchCosigner` constructor
with a value of type `ApiErrorSharedWalletNoSuchCosigner`.
In addition, we update the OpenAPI specification to be consistent with
the new definition.
This makes it easy for clients (such as our API integration tests) to
inspect the following values programmatically:
- cosigner_index
- credential_type
…sts. This commit replaces a string equality check (using an interpolated string) with an equality check on a concrete error type.
This interpolated string is a duplicate of a similar interpolated string in `Http.Server.Error`.
This is not strictly necessary, since we already have a golden file for
`ApiErrorSharedWalletNoSuchCosigner.json`, but it allows us to
demonstrate the format of a complete error object:
```json
{ "code": "shared_wallet_no_such_cosigner"
, "info":
{ "cosigner_index": 182
, "credential_type": "delegation"
}
, "message": "It looks like you've..."
}
```
jonathanknowles
force-pushed
the
jonathanknowles/shared-wallet-error-types
branch
from
77961b2
to
d1c0f19
Compare
jonathanknowles
changed the title
SharedWalletNoSuchCosigner API error.
jonathanknowles
changed the title
|
bors r+ |
iohk-bors bot
added a commit
that referenced
this pull request
Jan 17, 2023
3693: Machine-readable shared wallet API errors. r=jonathanknowles a=jonathanknowles ## Issue Number Follow-on from review of #3676. ## Summary This PR removes an **interpolated string equality check** from our API integration test suite, and replaces it an equality check that uses **`decodeErrorInfo`** in conjunction with an **API error record**: ```patch lib/wallet/integration/src/Test/Integration/Scenario/API/Shared/Wallets.hs - expectErrorMessage (errMsg403NoSuchCosigner (toText Payment) 7) rPatch5 + decodeErrorInfo rPatch5 `shouldBe` SharedWalletNoSuchCosigner + ApiErrorSharedWalletNoSuchCosigner + { cosignerIndex = ApiCosignerIndex 7 + , credentialType = ApiCredentialType Payment + } ``` ```patch lib/wallet/integration/src/Test/Integration/Framework/TestData.hs - errMsg403NoSuchCosigner :: Text -> Int -> String - errMsg403NoSuchCosigner cred cosigner = unwords - [ "It looks like you've tried to add a cosigner key to a" - , "shared wallet's" - , unpack cred - , "template for a non-existing cosigner index:" - , show cosigner - ] ``` The above definition of `errMsg403NoCosigner` (now removed) was essentially a **duplicate** of an identical interpolated string in `Http.Server.Error`: https://github.com/input-output-hk/cardano-wallet/blob/1263e6ed8c486f31ed75219b59a8e5fc6af577de/lib/wallet/api/http/Cardano/Wallet/Api/Http/Server/Error.hs#L801-L807 ## Advantages 1. If we make any breaking changes to the API error type, the compiler will warn us to update the test suite. 2. Someone browsing the integration tests can easily jump to the definition of the error type, or to the definitions of any of the constructors used. 3. We no longer need to manually keep two copies of an interpolated string function in sync with one another. ## Method To achieve this, we introduce the following structured error type: ```haskell data ApiErrorSharedWalletNoSuchCosigner = ApiErrorSharedWalletNoSuchCosigner { cosignerIndex :: !ApiCosignerIndex , credentialType :: !ApiCredentialType } deriving (Data, Eq, Generic, Show, Typeable) deriving (FromJSON, ToJSON) via DefaultRecord ApiErrorSharedWalletNoSuchCosigner deriving anyclass NFData ``` And add it as a parameter to the relevant constructor of ApiErrorInfo: ```patch lib/wallet/api/http/Cardano/Wallet/Api/Types/Error.hs data ApiErrorInfo = ... | ... - | SharedWalletNoSuchCosigner + | SharedWalletNoSuchCosigner !ApiErrorSharedWalletNoSuchCosigner | ... ``` When errors of this type are encoded as JSON, this results in objects of the following format: ```json { "code": "shared_wallet_no_such_cosigner" , "info": { "cosigner_index": 182 , "credential_type": "delegation" } , "message": "It looks like you've..." } ``` ## Future Work This PR serves as a demonstration of this technique, and only converts one particular error type. We could use this method to remove all duplicate interpolated error messages from our API integration test suite. Co-authored-by: Jonathan Knowles <jonathan.knowles@iohk.io>
|
Build failed: Errors (not caused by this PR): |
|
bors r+ |
jonathanknowles
commented
Jan 17, 2023
| toEnum <$> filter (>= minBoundInt) (shrinkIntegral $ fromEnum a) | ||
| where | ||
| minBoundInt :: Int | ||
| minBoundInt = fromEnum (minBound @a) |
There was a problem hiding this comment.
Later on I turned this into a PR on QuickCheck itself:
nick8325/quickcheck#350
|
Build succeeded: |
WilliamKingNoel-Bot
pushed a commit
that referenced
this pull request
Jan 17, 2023
…jonathanknowles a=jonathanknowles ## Issue Number Follow-on from review of #3676. ## Summary This PR removes an **interpolated string equality check** from our API integration test suite, and replaces it an equality check that uses **`decodeErrorInfo`** in conjunction with an **API error record**: ```patch lib/wallet/integration/src/Test/Integration/Scenario/API/Shared/Wallets.hs - expectErrorMessage (errMsg403NoSuchCosigner (toText Payment) 7) rPatch5 + decodeErrorInfo rPatch5 `shouldBe` SharedWalletNoSuchCosigner + ApiErrorSharedWalletNoSuchCosigner + { cosignerIndex = ApiCosignerIndex 7 + , credentialType = ApiCredentialType Payment + } ``` ```patch lib/wallet/integration/src/Test/Integration/Framework/TestData.hs - errMsg403NoSuchCosigner :: Text -> Int -> String - errMsg403NoSuchCosigner cred cosigner = unwords - [ "It looks like you've tried to add a cosigner key to a" - , "shared wallet's" - , unpack cred - , "template for a non-existing cosigner index:" - , show cosigner - ] ``` The above definition of `errMsg403NoCosigner` (now removed) was essentially a **duplicate** of an identical interpolated string in `Http.Server.Error`: https://github.com/input-output-hk/cardano-wallet/blob/1263e6ed8c486f31ed75219b59a8e5fc6af577de/lib/wallet/api/http/Cardano/Wallet/Api/Http/Server/Error.hs#L801-L807 ## Advantages 1. If we make any breaking changes to the API error type, the compiler will warn us to update the test suite. 2. Someone browsing the integration tests can easily jump to the definition of the error type, or to the definitions of any of the constructors used. 3. We no longer need to manually keep two copies of an interpolated string function in sync with one another. ## Method To achieve this, we introduce the following structured error type: ```haskell data ApiErrorSharedWalletNoSuchCosigner = ApiErrorSharedWalletNoSuchCosigner { cosignerIndex :: !ApiCosignerIndex , credentialType :: !ApiCredentialType } deriving (Data, Eq, Generic, Show, Typeable) deriving (FromJSON, ToJSON) via DefaultRecord ApiErrorSharedWalletNoSuchCosigner deriving anyclass NFData ``` And add it as a parameter to the relevant constructor of ApiErrorInfo: ```patch lib/wallet/api/http/Cardano/Wallet/Api/Types/Error.hs data ApiErrorInfo = ... | ... - | SharedWalletNoSuchCosigner + | SharedWalletNoSuchCosigner !ApiErrorSharedWalletNoSuchCosigner | ... ``` When errors of this type are encoded as JSON, this results in objects of the following format: ```json { "code": "shared_wallet_no_such_cosigner" , "info": { "cosigner_index": 182 , "credential_type": "delegation" } , "message": "It looks like you've..." } ``` ## Future Work This PR serves as a demonstration of this technique, and only converts one particular error type. We could use this method to remove all duplicate interpolated error messages from our API integration test suite. Co-authored-by: Jonathan Knowles <jonathan.knowles@iohk.io> Source commit: 76fdaf9
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Issue Number
Follow-on from review of #3676.
Summary
This PR removes an interpolated string equality check from our API integration test suite, and replaces it an equality check that uses
decodeErrorInfoin conjunction with an API error record:The above definition of
errMsg403NoCosigner(now removed) was essentially a duplicate of an identical interpolated string inHttp.Server.Error:https://github.com/input-output-hk/cardano-wallet/blob/1263e6ed8c486f31ed75219b59a8e5fc6af577de/lib/wallet/api/http/Cardano/Wallet/Api/Http/Server/Error.hs#L801-L807
Advantages
Method
To achieve this, we introduce the following structured error type:
And add it as a parameter to the relevant constructor of ApiErrorInfo:
lib/wallet/api/http/Cardano/Wallet/Api/Types/Error.hs data ApiErrorInfo = ... | ... - | SharedWalletNoSuchCosigner + | SharedWalletNoSuchCosigner !ApiErrorSharedWalletNoSuchCosigner | ...When errors of this type are encoded as JSON, this results in objects of the following format:
{ "code": "shared_wallet_no_such_cosigner" , "info": { "cosigner_index": 182 , "credential_type": "delegation" } , "message": "It looks like you've..." }Future Work
This PR serves as a demonstration of this technique, and only converts one particular error type. We could use this method to remove all duplicate interpolated error messages from our API integration test suite.