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 additional details to getLatestLedger response #337

Closed
wants to merge 4 commits into from

Conversation

aditya1702
Copy link
Contributor

What

Closes #79

  • Add additional information about a ledger to the GetLatestLedger response.
  • Unittests

Why

The current info lacks basic ledger information. This change brings it in sync with the information returned by GetLedgers endpoint - #303 (comment)

Known limitations

NA

@aditya1702 aditya1702 self-assigned this Dec 12, 2024
@2opremio
Copy link
Contributor

2opremio commented Dec 13, 2024

I am not sure why this is needed. You can always obtain the latest ledger and query getLedger with the obtained sequence.

I understand it saves one API call (which can be an advantage), but it also complicates the API. Is there a usecase justiying this?

If we really want to do this, can we at least try to factor out the obtention of LedgerInfo ? (maybe it ends up complicating the code further, but we should at least consider it)

@leighmcculloch
Copy link
Member

I am not sure why this is needed. You can always obtain the latest ledger and query getLedger with the obtained sequence.

The getLatestLedger and getLedger both impl they'll return a "ledger", and yet both return different sets of data. The better developer experience would be to return the same resource. I understand this is an RPC API and not a RESTful API oriented around resources, but I think the concepts are still useful.

@leighmcculloch
Copy link
Member

Is there a usecase justiying this?

There are no critical use cases, as you say you can make two calls, it's just inconvenient, or makes for an inconsistent developer experience.

There are potential issues if operators horizontally scale RPC and return inconsistent results, such as latest refers to a ledger seq that can't be retrieved, but that's just theoretical, I'm not aware of this occurring.

@aditya1702
Copy link
Contributor Author

Based on the discussion we had and @leighmcculloch 's comments, it looks like we dont actually have a strong reason for doing this change right now. Closing this for now but we can always revisit in the future.

@aditya1702 aditya1702 closed this Dec 19, 2024
@leighmcculloch
Copy link
Member

The better developer experience would be to return the same resource.

I think this ☝🏻 is a strong reason.

Based on the discussion we had

Could you link to the discussion?

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.

Add more ledger details to getLedger/getLatestLedger
3 participants