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

Removing force asc ordering on the page #336

Open
wants to merge 2 commits into
base: main
Choose a base branch
from
Open

Conversation

lassemand
Copy link
Contributor

Purpose

The accounts page is expecting the page to be in the same ordering as the querying.

So basically saying this does not adhere GraphQL standards, but we are migrating the C# backend and hence we need to adjust to the expectations of the frontend not GraphQL.

Changes

Removing forced asc

WHEN $4 THEN amount
WHEN $5 THEN num_txs
WHEN $6 THEN delegated_stake
END ASC",
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sorry for not mentioning this earlier when we spoke about it, but I just realized again why this is needed.
This ensures that the above/inner ordering does not leak, since the above/inner is related to whether user is querying first or last elements.
I think the solution is to use DESC instead of ASC on line 685, otherwise clicking next and then previous when paging the account list will change the ordering of the list in the frontend.

Copy link
Contributor Author

@lassemand lassemand Dec 21, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It is a SELECT * FROM (...) ORDER BY CASE (..) ASC

That does nothing but change the order of the inner query. I do not follow how that can prevent a leak?

Could you for example write a test which demonstrates its purpose?

Copy link
Contributor Author

@lassemand lassemand Dec 21, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We cannot desc it either because newest account is asc.

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.

2 participants