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

transactionHandler's GetLedgerRange is not accurate #208

Closed
2opremio opened this issue Jun 11, 2024 · 4 comments · Fixed by #217
Closed

transactionHandler's GetLedgerRange is not accurate #208

2opremio opened this issue Jun 11, 2024 · 4 comments · Fixed by #217
Assignees
Labels
bug Something isn't working rpc-sdk-scrum

Comments

@2opremio
Copy link
Contributor

2opremio commented Jun 11, 2024

It uses the meta table and not the transaction table. The meta table can have a longer history than the transaction table since it stores the maximum range of the events storage window and the transaction storage window.

Also, GetLedgerRange is untested. A test should be added since the function is pretty complex.

The problem was introduced in #174

@2opremio 2opremio added the bug Something isn't working label Jun 11, 2024
@2opremio 2opremio changed the title transactionHandler's GetLedgerRange is wrong transactionHandler's GetLedgerRange is not accurate Jun 11, 2024
@2opremio
Copy link
Contributor Author

2opremio commented Jun 11, 2024

Also, we will need to revive the old LedgerRangeGetter to include the event range like we used to in getHealth

@2opremio
Copy link
Contributor Author

2opremio commented Jun 11, 2024

Finally GetLedgerRange is overkill if we only want the Latest Ledger sequence, we can use GetLatestLedger for that.

@aditya1702
Copy link
Contributor

Finally GetLedgerRange is overkill if we only want the Latest Ledger sequence, we can use GetLatestLedger for that.

@2opremio We want both latestLedger and oldestLedger along with their details. I think that is why @Shaptic added this method.

@aditya1702 aditya1702 assigned Shaptic and unassigned aditya1702 Jun 11, 2024
@mollykarcher mollykarcher added this to the platform sprint 47 milestone Jun 11, 2024
@mollykarcher mollykarcher moved this from Backlog to To Do in Platform Scrum Jun 11, 2024
@2opremio
Copy link
Contributor Author

@2opremio We want both latestLedger and oldestLedger along with their details. I think that is why @Shaptic added this method.

Not for every endpoint (look at the current uses of GetLedgerRange)

@aditya1702 aditya1702 self-assigned this Jun 17, 2024
@aditya1702 aditya1702 moved this from To Do to In Progress in Platform Scrum Jun 17, 2024
@aditya1702 aditya1702 moved this from In Progress to Needs Review in Platform Scrum Jul 2, 2024
@github-project-automation github-project-automation bot moved this from Needs Review to Done in Platform Scrum Jul 2, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working rpc-sdk-scrum
Projects
Status: Done
Development

Successfully merging a pull request may close this issue.

4 participants