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

Refactor population of transaction JSON #4801

Open
Bronek opened this issue Nov 3, 2023 · 2 comments
Open

Refactor population of transaction JSON #4801

Bronek opened this issue Nov 3, 2023 · 2 comments
Labels
Feature Request Used to indicate requests to add new features

Comments

@Bronek
Copy link
Collaborator

Bronek commented Nov 3, 2023

Currently we have multiple locations performing the same task of populating JSON with transaction data, taking into account client-selected API version. As we add more API versions in the future, all these locations will accumulate complexity, while essentially doing the same task. Ideally we should improve code cohesion / reduce duplication by pulling all this logic into single location e.g. STTx::populateJson(Json::Value& target, ReadView const& ledger, unsigned apiVersion ... etc.) (not necessarily inside STTx , just an example).

There are two challenges that need to be solved:

  • first challenge is pulling the necessary data while managing dependencies (levelization) on ledger, master ledger, context etc. These data are (using JSON names):
    • validated
    • close_time_iso / date
    • ledger_index / inLedger
    • ledger_hash
  • second challenge is related to how we pre-populate transaction JSON for any API version inNetworkOPs.cpp and then adjust it accordingly for a specific version:
    • perhaps want to populate JSON for a specific API version,
    • then create function that will transform this JSON from one API version to another ?
@Bronek Bronek added the Feature Request Used to indicate requests to add new features label Nov 3, 2023
@scottschurr
Copy link
Collaborator

I'll mention that clio has exactly the same problems and the added difficulty that it wants to remain in lock step with what rippled supports. You might check in with the clio folks to see if there's some way these changes can be available to both rippled and clio?

@Bronek
Copy link
Collaborator Author

Bronek commented Nov 3, 2023

Also, by "currently" I actually mean since #4775

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Feature Request Used to indicate requests to add new features
Projects
None yet
Development

No branches or pull requests

2 participants