-
Notifications
You must be signed in to change notification settings - Fork 217
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
[ADP-3306] Make additional API errors machine-readable. #4590
[ADP-3306] Make additional API errors machine-readable. #4590
Conversation
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.
This looks great!
Before merging though, it's still missing a couple of things, including Swagger definitions for the new info
fields, and JSON golden tests.
I've added the missing things in: #4592
This is necessary to avoid failures of the following type: ``` src/Test/Aeson/Internal/GoldenSpecs.hs:77:5: 1) Cardano.Wallet.Api.Types, JSON golden roundtrip, JSON encoding of ApiError, produces the same JSON as is found in /home/jsk/projects/input-output-hk/cardano-wallet-0/lib/unit/test/data/Cardano/Wallet/Api/ApiError.json uncaught exception: AesonDecodeError AesonDecodeError "Error in $.samples[14]: parsing Cardano.Wallet.Api.Types.Error.ApiErrorInfo(MissingWitnessesInTransaction) failed, key \"info\" not found" To rerun use: --match "/Cardano.Wallet.Api.Types/JSON golden roundtrip/JSON encoding of ApiError/produces the same JSON as is found in /home/jsk/projects/input-output-hk/cardano-wallet-0/lib/unit/test/data/Cardano/Wallet/Api/ApiError.json/" --seed 1949888530 Randomized with seed 1949888530 ``` The above error appears because we recently added a non-optional `info` field to the `errMissingWitnessesInTransaction` error.
72066e0
to
e9cd7df
Compare
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! 🎆
errMsg403InvalidConstructTx = | ||
"It looks like I've created an empty transaction that does not have \ | ||
\any payments, withdrawals, delegations, metadata nor minting. \ | ||
\Include at least one of them." |
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.
🍾
… This PR makes the following API errors machine-readable: - `ApiErrorNoSuchPool` - `ApiErrorMissingWitnessesInTransaction` In addition, we adjust the integration test suite to express expectations in terms of rich error objects instead of interpolated error message strings. ### Issue ADP-3306 Source commit: bfbb5b8
…4594) This PR follows on from #4590, and: - Extracts out witness count validation into a common function `validateWitnessCounts`. - Replaces `fromIntegral` with `intCastMaybe` and `fromJustNote` with a unique and identifiable error message. - Changes the types of fields `{expected,detected}NumberOfKeyWits` to `Natural`. - Changes the name of `ErrSubmitTransactionPartiallySignedOrNoSignedTx` to `ErrSubmitTransactionMissingWitnesses`. ## Issue ADP-3306
…4594) This PR follows on from #4590, and: - Extracts out witness count validation into a common function `validateWitnessCounts`. - Replaces `fromIntegral` with `intCastMaybe` and `fromJustNote` with a unique and identifiable error message. - Changes the types of fields `{expected,detected}NumberOfKeyWits` to `Natural`. - Changes the name of `ErrSubmitTransactionPartiallySignedOrNoSignedTx` to `ErrSubmitTransactionMissingWitnesses`. ## Issue ADP-3306
This PR makes the following API errors machine-readable:
ApiErrorNoSuchPool
ApiErrorMissingWitnessesInTransaction
In addition, we adjust the integration test suite to express expectations in terms of rich error objects instead of interpolated error message strings.
Issue
ADP-3306