-
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 more HTTP API errors machine-readable. #4622
[ADP-3306] Make more HTTP API errors machine-readable. #4622
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.
Thanks for making this PR!
I have two general comments:
-
Suggestion:
Can we convert the error strings into constants (i.e., stop building interpolated error strings)? Now that we have JSON objects to carry the variable parts of the error, I think there's no need to repeat this information within the error strings.Example: [ADP-3306] Make more HTTP API errors machine-readable. #4622 (comment)
-
Request:
Within API error record types, can we replace the use of low-level types likeText
andNatural
with specialised types such asApiT TokenPolicyId
,ApiT TokenQuantity
,ApiAnyAddress
? The existing API error types all use specialised types.Example: [ADP-3306] Make more HTTP API errors machine-readable. #4622 (comment)
@@ -293,6 +301,34 @@ data ApiErrorTxOutputLovelaceInsufficient = ApiErrorTxOutputLovelaceInsufficient | |||
via DefaultRecord ApiErrorTxOutputLovelaceInsufficient | |||
deriving anyclass NFData | |||
|
|||
data ApiErrorOutputTokenQuantityExceedsLimit = ApiErrorOutputTokenQuantityExceedsLimit | |||
{ address | |||
:: !Text |
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.
Can we replace this with ApiAnyAddress
?
:: !Text | |
:: !ApiAnyAddress |
lib/integration/scenarios/Test/Integration/Scenario/API/Shelley/CoinSelections.hs
Outdated
Show resolved
Hide resolved
lib/integration/scenarios/Test/Integration/Scenario/API/Shelley/CoinSelections.hs
Show resolved
Hide resolved
In this commit, we wrap excessively-long lines to fit within the line length limit (80 characters).
The `bundle_size` field value can never be negative.
The `bundle_size` field value can never be negative.
We can no longer use invalid values for `policy_id` and `asset_name`.
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! 👍🏻
I think it makes sense to defer making a decision on using a more dedicated type for the address
fields for now. Switching to ApiAddress n
, where n
is a NetworkDiscriminant
would require quite a few invasive changes.
…- [x] Convert to machine-readable error: `AssetNotPresent` - [x] Convert to machine-readable error: `OutputTokenBundleSizeExceedsLimit` - [x] Convert to machine-readable error: `OutputTokenQuantityExceedsLimit` - [x] Convert to machine-readable error: `TransactionAlreadyInLedger` - [x] Convert to machine-readable error: `WrongEncryptionPassphrase` ### Issue Number ADP-3306 Source commit: b5b9f00
AssetNotPresent
OutputTokenBundleSizeExceedsLimit
OutputTokenQuantityExceedsLimit
TransactionAlreadyInLedger
WrongEncryptionPassphrase
Issue Number
ADP-3306