Skip to content
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

Add framework for converting XDR structures to JSON #249

Merged
merged 68 commits into from
Aug 12, 2024
Merged

Conversation

Shaptic
Copy link
Contributor

@Shaptic Shaptic commented Jul 15, 2024

What

This PR adds support for bsae64-encoded XDR fields to instead be returned as unpacked JSON objects.

New Request Parameter

We introduce a new, optional parameter to all endpoints with XDR outputs:

xdrFormat?: "base64" | "json"

When omitted, the behavior does not change and we encode fields as base64.

New Response Fields

There are new field names for the JSONified versions of XDR structures. Any field with an Xdr suffix (e.g., resultXdr in getTransaction()) will be replaced with one that has a Json suffix (e.g., resultJson) that is a JSON object verbosely and completely describing the XDR structure.

Certain XDR-encoded fields do not have an Xdr suffix, but those also have a *Json equivalent and are listed below:

  • getEvents: topic -> topicJson, value -> valueJson
  • getLedgerEntries: key -> keyJson, xdr -> dataJson
  • getLedgerEntry: xdr -> entryJson
  • simulateTransaction: transactionData, events, results.auth, restorePreamble.transactionData, stateChanges.key|before|after all have a Json suffix, and results.xdr is now results.returnValueJson

Notes for reviewers

  • It is much easier to see the changes when you turn whitespace off.
  • Start with the bindings layer:
    • lib.rs has the Rust code to perform the XDR -> JSON conversion via stellar_xdr.
    • xdr_json.h just has the header definitions to define what is accessible from Go -> Rust.
    • conversion.go has the actual binding invocation:
      • convertAnyBytes is the one that does the low-level Go -> C/FFI -> Rust work
      • the exported members let other packages use it and one is for validation
      • TransactionToJSON is an isolated way to see how the conversion is done for a transaction structure
      • the others are just helper abstractions
  • Then, review one of the endpoints, e.g., get_transaction.go. Each one of these has three sets of changes:
    • Validation for the new request parameter
    • New fields with a Json (serialized) and JSON (Golang) suffix with a 1:1 mapping to their corresponding base64-encoded fields that should always be a json.RawMessage.
    • A switch statement in the handler that uses the new xdr2json package (i.e. conversion.go) to convert XDR structures to JSON.
  • The same changes should be in every endpoint.
  • Most test changes have a lot of variable renames (not serialized fields), so feel free to ignore those entirely.
  • There are also new tests for the JSON behavior.

Why

Closes #124.

Known Limitations

Remaining Items

  • Complete the initial implementation
  • Add support for all endpoints:
    • getTransaction
    • getTransactions
    • getLedgerEntry
    • getLedgerEntries
    • getEvents
    • sendTransaction
    • simulateTransaction
  • Testing for all of the above
  • Polish

Alternative Implementations

Would it be possible to extend xdrgen to actually have .JSON()-like methods attached? This would be a significant lift there, since it'd need to do the Go <-> Rust FFI, but the calling code would be far cleaner and we'd get JSON output "for free" across any Golang project that used stellar/go/xdr...

@Shaptic Shaptic marked this pull request as draft July 15, 2024 22:24
@Shaptic Shaptic force-pushed the json-response-format branch from 3928422 to a143442 Compare July 17, 2024 02:43
@Shaptic Shaptic force-pushed the json-response-format branch from a143442 to 37c8e92 Compare July 17, 2024 02:49
@Shaptic Shaptic force-pushed the json-response-format branch from e8553c4 to 1ef0940 Compare August 5, 2024 22:57
@Shaptic
Copy link
Contributor Author

Shaptic commented Aug 5, 2024

@2opremio sure, that's fine with me! In that case, do you mind approving this to merge into a feature branch (if there are no other critiques) so that we can still isolate the preflight <-> xdr2json separation changes?

Actually, it ended up being a lot easier than I expected. Done in bf70f6f!

@Shaptic Shaptic force-pushed the json-response-format branch from fb4239e to 641c06f Compare August 6, 2024 00:03
@Shaptic Shaptic force-pushed the json-response-format branch 2 times, most recently from 07799d4 to 5ce8937 Compare August 6, 2024 06:46
@Shaptic Shaptic force-pushed the json-response-format branch from c7f2086 to ae2897a Compare August 6, 2024 17:07
@Shaptic Shaptic requested review from 2opremio and tamirms August 6, 2024 17:08
Copy link
Contributor

@tamirms tamirms left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM!

@Shaptic Shaptic force-pushed the json-response-format branch from 11b6aa3 to 2ae24b3 Compare August 12, 2024 20:36
@Shaptic Shaptic enabled auto-merge (squash) August 12, 2024 20:45
@Shaptic Shaptic merged commit b88b421 into main Aug 12, 2024
19 checks passed
@Shaptic Shaptic deleted the json-response-format branch August 12, 2024 20:53
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

JSON Support
5 participants