fix: type correctness issues, error body preservation, and get_did_document implementation#29
Open
Onyx2406 wants to merge 2 commits into
Open
Conversation
Fix several bugs found by auditing the SDK against the Open Payments
specification (v1.3.0):
1. OutgoingPayment: grant_spent_debit_amount and
grant_spent_receive_amount are now Option<Amount>. The spec only
includes these in the "with-spent-amounts" variant (POST response),
not in GET/LIST responses. Previously, deserializing a GET response
would fail at runtime because the server wouldn't include these
fields.
2. IncomingPaymentWithMethods: removed #[serde(flatten)] on
IncomingPayment to fix duplicate "methods" field in serialized
output. IncomingPayment already had an optional methods field,
causing undefined serde behavior with duplicate JSON keys.
3. AccessToken.access: changed from Option<Vec<AccessItem>> to
Vec<AccessItem> to match the spec which marks "access" as required.
4. HTTP error responses: the response body is now read and included
in the error. Previously, 4xx/5xx responses discarded the body
entirely, making it impossible to see server error details.
Structured JSON error bodies are parsed into the error's details
field.
5. get_did_document: implemented the previously unimplemented public
API method that would panic at runtime. Now correctly fetches
the DID document from {walletAddress}/did.json.
- Add test for OutgoingPayment deserialization without grant spent amounts (the core bug this PR fixes — GET responses omit these) - Add test for OutgoingPayment deserialization WITH grant spent amounts (POST responses include them) - Add test verifying AccessToken requires the access field (deserialization fails when absent, matching the spec)
This file contains hidden or 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
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
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.
Summary
Fixes five issues found by auditing the SDK against the Open Payments specification v1.3.0:
1. OutgoingPayment deserialization failure (bug)
grant_spent_debit_amountandgrant_spent_receive_amountwere required fields, but the spec only includes them in theoutgoing-payment-with-spent-amountsvariant (POST response). GET/LIST responses from a spec-compliant server would fail to deserialize because the server doesn't include these fields.Fix: Changed both fields to
Option<Amount>.2. IncomingPaymentWithMethods duplicate fields (bug)
The struct used
#[serde(flatten)]onIncomingPayment, butIncomingPaymentalready had anOption<Vec<PaymentMethod>>methods field. This produced duplicatemethodskeys in serialized JSON — undefined behavior in serde.Fix: Replaced flatten with explicit fields.
3. AccessToken.access incorrectly optional (spec deviation)
The spec marks
accessas required onaccess_token, but the SDK made itOption<Vec<AccessItem>>.Fix: Changed to
Vec<AccessItem>.4. HTTP error response body discarded (bug)
When the server returned 4xx/5xx, the response body was completely discarded. The Open Payments spec defines structured error responses with error codes and descriptions that are critical for debugging.
Fix: Read the response body, include it in the error description, and parse JSON error bodies into the
detailsfield.5.
get_did_documentpanics at runtime (bug)The public method contained
unimplemented!()which would crash the program when called.Fix: Implemented as a GET request to
{walletAddress}/did.json, returningserde_json::Value.Test plan