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

Align naming convention to cursor #118

Closed
janewang opened this issue Mar 28, 2024 · 6 comments · Fixed by #297
Closed

Align naming convention to cursor #118

janewang opened this issue Mar 28, 2024 · 6 comments · Fixed by #297

Comments

@janewang
Copy link
Collaborator

What problem does your feature solve?

There is some confusion for developers about what is cursor. For example in getEvents, we call cursor in the request and pagingToken in the response.

Discord thread: https://discord.com/channels/897514728459468821/966788672164855829/1222799292344045619

Developers are confusing that pagingToken is the cursor. We should rename it to cursor in the response.

@janewang janewang changed the title Align naming convention Align naming convention to cursor Mar 28, 2024
@mollykarcher
Copy link
Contributor

This would be a breaking API change. We could introduce it/duplicate the field in the response and remove it in a later protocol version (maybe 22?)

@janewang
Copy link
Collaborator Author

janewang commented Apr 4, 2024

Agreed, wait after one or two protocol releases before we remove duplicated field is a great idea.

@mollykarcher
Copy link
Contributor

mollykarcher commented Apr 9, 2024

It's also strange that pagingToken is nestled within the individual response objects. As part of the rename, we'd also like to change this formatting to bring the cursor to be a top-level field, at the same level as events. The value of cursor would be the same as the value of the pagingToken from the last element in the list of events. I'd expect this format to be a little more familiar/comfortable for developers, as it tends to be the format that popular/common public APIs use.

Note that this format should apply to all future paginated endpoints, not just events, and will require updates to our Pagination documentation pages. An example of the new proposed response format in an ideal end-state would be:

{
  "jsonrpc": "2.0",
  "id": 8675309,
  "result": {
    "events": [
      {
        "type": "contract",
        "ledger": 2531273,
        "ledgerClosedAt": "2023-11-15T09:20:38Z",
        "contractId": "CDLZFC3SYJYDZT7K67VZ75HPJVIEUVNIXF47ZG2FB2RMQQVU2HHGCYSC",
        "id": "0010871734752280576-0000000004",
        "topic": [
          "AAAADwAAAAh0cmFuc2Zlcg==",
          "AAAAEgAAAAAAAAAA+YQ+FM83vUUwQ6P3gKCMVTyC3/jO+DERXTWJDKEjagU=",
          "AAAAEgAAAAAAAAAAwl0UMLLKYqMEedoowz8VnwbRywjcKEeQegoMmU6C9/0=",
          "AAAADgAAAAZuYXRpdmUAAA=="
        ],
        "value": {
          "xdr": "AAAACgAAAAAAAAAAAAAAAAAAAJY="
        },
        "inSuccessfulContractCall": true
      }
    ],
    "latestLedger": 2539388,
    "cursor": "0010871734752280576-0000000004"
  }
}

@mollykarcher mollykarcher added the breaking-change Breaking change tag label Jul 30, 2024
@mollykarcher mollykarcher added this to the platform sprint 50 milestone Aug 27, 2024
@mollykarcher mollykarcher moved this from Backlog to To Do in Platform Scrum Aug 27, 2024
@aditya1702 aditya1702 assigned aditya1702 and psheth9 and unassigned aditya1702 Sep 6, 2024
@psheth9 psheth9 moved this from To Do to In Progress in Platform Scrum Sep 10, 2024
@psheth9
Copy link
Contributor

psheth9 commented Sep 11, 2024

Instead of removing pagingToken for each events how about we rename pagingToken to eventId. I believe this id might be useful for devs/clients to uniquely identify events on their end.

Cursor will still be present as a top level field for pagination purpose.

@mollykarcher
Copy link
Contributor

Instead of removing pagingToken for each events how about we rename pagingToken to eventId

Isn't this what the id field currently is?

@mollykarcher
Copy link
Contributor

Another recommendation that we can take or leave, given I know we're trying to get this into p22 and I'm not sure how much work it may be. I think we should consider making the cursor completely opaque to the client/end-user and encoding the filter/sort information into it.

For example, the cursor that the API returns could be a base64 encoded string representation of something like the following dictionary:

{
    startLedger: 10,
    endLedger: 20
    event_id: 4000,
    order: desc
}

This would give us a couple advantages:

  • Future proofs, as the cursor is clearly not an event_id, so it prevents clients from relying on it as one, and therefore let's us change it in the future when we need to
  • Removes the need for the client to pass in all of their filter params + sort params on every single request. If they have a live cursor they effectively have a stateful scroll id that they can use to continue their scrolling session
  • If it helps us performance-wise, we could actually effectively change the filter parameters for the next request in the scroll (ex. moving startLedger up)

@psheth9 psheth9 closed this as completed Oct 2, 2024
@github-project-automation github-project-automation bot moved this from Needs Review to Done in Platform Scrum Oct 2, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
Status: Done
Development

Successfully merging a pull request may close this issue.

5 participants