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

Update documentation for getTransaction #1093

Closed
wants to merge 2 commits into from
Closed

Conversation

keefertaylor
Copy link
Contributor

The field ledgerVersion is duplicated in the documentation twice. I think that this should be ledgerIndex?

If so, we can add it to the mock response below.

@keefertaylor keefertaylor requested a review from mDuo13 November 13, 2019 16:44
@keefertaylor
Copy link
Contributor Author

Actually, looks like this is just duplicated? There's no ledger index in the response.

@intelliot
Copy link
Collaborator

A few considerations here:

  1. docs/index.md is a generated file and should not be edited directly. Any changes made to it will be lost the next time the docs are generated. Instead, edit the relevant source files in:

(Generate the docs with yarn run docgen)

  1. In this case, the response object for getTransaction is defined by JSON schemas:
  1. The line you have deleted here is the integer variant of ledgerVersion. There is some way we could change the doc generator or schema to make this more clear, but for now, this is what we have. What it intends to communicate is that the ledgerVersion can be an integer if the user wants to specify a particular ledger version by the ledger version number (aka ledger index).

@intelliot
Copy link
Collaborator

@keefertaylor Does the above explanation make sense? Let me know if you have questions or could use more elaboration.

@mDuo13
Copy link
Collaborator

mDuo13 commented Mar 9, 2020

Why does the "output" schema have two options for ledgerVersion, anyway? Does the API actually vary in which one it returns, or is this a case of reusing something that's not specific to the outputs?

@intelliot
Copy link
Collaborator

@mDuo13 The API always returns an unsigned integer because it comes from calling rippled's tx method and setting ledgerVersion: tx.ledger_index. https://github.com/ripple/ripple-lib/blob/develop/src/ledger/parse/utils.ts#L121

You are correct: this is a case of reusing the ledgerVersion schema in both the "input" and "output" sides.

The string type occurs only as "input", when the user specifies 'validated', 'closed', or 'current'.

I think it would be possible to create a new schema for the ledgerVersion that can be returned in responses ("output"). We would change the $ref here: https://github.com/ripple/ripple-lib/blob/develop/src/common/schemas/output/outcome.json#L45

On the other hand, I think people should use request and tx (referring to the rippled API) instead. In this case I would add new helper functions for convenience and to fill in any gaps and bridge any difficulties in the rippled API, per #812.

@intelliot
Copy link
Collaborator

Closing this, as it has had no activity in the past year. Feel free to open a new issue/PR!

@intelliot intelliot closed this May 12, 2021
@mvadari mvadari deleted the Fix-docs branch July 27, 2021 19:15
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.

3 participants