Skip to content

Unify request validation methods for getLedgers and getTransactions#407

Merged
Shaptic merged 9 commits intoprotocol-23from
better-cursor-validation
Apr 17, 2025
Merged

Unify request validation methods for getLedgers and getTransactions#407
Shaptic merged 9 commits intoprotocol-23from
better-cursor-validation

Conversation

@Shaptic
Copy link
Contributor

@Shaptic Shaptic commented Apr 15, 2025

What

Use a single method to ensure request parameters are correct. Specifically, follow the documented set of rules:

  • If pagination is set:
    • If the cursor is set, the startLedger is not set.
    • If the limit is set, it does not exceed the maxLimit.
    • If the cursor is not set, the startLedger is in the RPC's range of known ledgers.
  • Otherwise,
    • The startLedger is set and is in the RPC's range of known ledgers.

This also improves the error messages in failure cases by, for example, including the startLedger that was provided in the message itself.

Why

Several issues have cropped up related to this.

For example, #391 is caused by a lack of startLedger validation due to the presence of pagination. A recent Slack thread by @fnando also highlights problematic validation.

Known limitations

n/a

@Shaptic Shaptic requested review from a team and 2opremio April 15, 2025 21:43
@urvisavla
Copy link
Contributor

urvisavla commented Apr 15, 2025

It looks like getEvents has its own checks for pagination and ledger range. Should that updated it to use this as well for consistent error handling?

@Shaptic
Copy link
Contributor Author

Shaptic commented Apr 15, 2025

@urvisavla getEvents has a more complicated series of checks (e.g. it has endLedger, needs to validate topics, etc.) and uses a different cursor (*Cursor vs. string), so it has to have its own validation. It could, I think, be unified in theory, but I didn't want to make this a massive PR so I figured this would be a quick n' easy win.

leighmcculloch
leighmcculloch approved these changes Apr 15, 2025
View reviewed changes
@urvisavla
Copy link
Contributor

urvisavla commented Apr 16, 2025

@urvisavla getEvents has a more complicated series of checks (e.g. it has endLedger, needs to validate topics, etc.) and uses a different cursor (*Cursor vs. string), so it has to have its own validation. It could, I think, be unified in theory, but I didn't want to make this a massive PR so I figured this would be a quick n' easy win.

I missed that GetEvents also includes the endLedger. I agree there’s no need to expand the scope of this PR. The changes look good overall. There are some failing integration tests that you'll want to fix.

leighmcculloch
leighmcculloch approved these changes Apr 17, 2025
View reviewed changes
@Shaptic Shaptic merged commit 4e9e9e5 into protocol-23 Apr 17, 2025
9 of 13 checks passed
@Shaptic Shaptic deleted the better-cursor-validation branch April 17, 2025 04:17
@Shaptic Shaptic mentioned this pull request Apr 17, 2025
Merged
@Shaptic Shaptic mentioned this pull request Jun 10, 2025
Merged
7 tasks
@sreuland sreuland mentioned this pull request Jun 10, 2025
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@leighmcculloch leighmcculloch leighmcculloch approved these changes

@2opremio 2opremio Awaiting requested review from 2opremio

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

3 participants

@Shaptic @urvisavla @leighmcculloch