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

[Dashboard|SDK] Implemented leaders sorting #2904

Merged
merged 29 commits into from
Jan 13, 2025

Conversation

eugenvoronov
Copy link
Contributor

@eugenvoronov eugenvoronov commented Dec 12, 2024

Issue tracking

This issue is being tracked in the following GitHub issue: [Dashboard] Support leaderboard sorting by reputation and fee on BE
#2778

Context behind the change

The changes in this pull request aim to improve how leaderboards are sorted in the application. Previously, sorting was done only on the client-side, leading to performance issues and mismatches with user expectations. Now, the sorting is handled server-side to ensure that the results are accurately sorted according to the selected criteria.

Key changes:

  1. Backend (Details controller and service):

    • Updated /leaders controller to accept sorting parameters (orderBy, orderDirection, first, and skip).
    • Implemented getLeader method in the Details Service to handle sorting by various criteria (e.g., fee, reputation, amountStaked, role).
    • Added pagination and reputation-based filtering in the service layer.
    • Modified the assignReputationsToLeaders method to ensure default reputation values are assigned when reputation data is missing.
  2. SDK updates:

    • Extended the LeaderFilter in both Python and TypeScript SDKs to accept additional parameters such as roles, minAmountStaked, and orderBy. In order to effectively filter data at the subgraph level and request the data we need.
    • Updated the GET_LEADERS_QUERY GraphQL query to support these new parameters.
    • Modified the getLeaders method in OperatorUtils to handle the new filtering and sorting logic.

How has this been tested?

Unit Tests:

  • Implemented unit tests for the Details Service to cover scenarios such as:
    • Fetching and returning leaders with reputations.
    • Handling missing reputation data gracefully.
    • Sorting leaders by reputation.
    • Handling errors when fetching reputations.
    • Supporting pagination when sorting by reputation.

Manual Testing:

  • Backend:
    • Tested the /leaders endpoint to ensure it handles sorting by multiple criteria such as fee, reputation, amount staked, and role.
    • Verified pagination works as expected when sorting.
    • Checked default filtering logic for minAmountStaked and roles.

Release Plan

Frontend

Implement sorting on Dashboard UI side.

Suggestion

It would be good to unify the list of roles, since the spelling varies from service to service.

Identified issue: Multi-chain sorting inaccuracy

During development, I noticed a sorting issue when querying leaders across multiple chain IDs. When retrieving data from multiple networks (e.g. getAvailableNetworks return more than one chainId), the sorting of the aggregated results becomes incorrect. This happens because sorting is performed separately within each subgraph on the SDK side, not across all queried chains simultaneously and together.

Since the subgraph only sorts data within its own chain-specific dataset, fetching results from multiple chain IDs involves sending separate requests. After receiving these sorted lists, we flatten (concatenate) them in the application, causing the global sorting to be inaccurate.

A possible solution is to implement back-end sorting after aggregating results from multiple chain IDs. However, this approach may be inefficient.

@eugenvoronov eugenvoronov self-assigned this Dec 12, 2024
Copy link

vercel bot commented Dec 12, 2024

The latest updates on your projects. Learn more about Vercel for Git ↗︎

Name Status Preview Comments Updated (UTC)
human-app ✅ Ready (Inspect) Visit Preview 💬 Add feedback Jan 10, 2025 4:38pm
human-dashboard-frontend ✅ Ready (Inspect) Visit Preview 💬 Add feedback Jan 10, 2025 4:38pm
2 Skipped Deployments
Name Status Preview Comments Updated (UTC)
faucet-frontend ⬜️ Ignored (Inspect) Visit Preview Jan 10, 2025 4:38pm
faucet-server ⬜️ Ignored (Inspect) Visit Preview Jan 10, 2025 4:38pm

Copy link
Contributor

@dnechay dnechay left a comment

Choose a reason for hiding this comment

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

Few extra questions:

  1. Are these md files updates automatic? Could you please shed some light on how/when we run it?
  2. What about a UI counterpart? This PR focuses on BE side, are we going to implement actual logic to fetch values using "sort criteria" selected by user in a different one?

@eugenvoronov
Copy link
Contributor Author

2. What about a UI counterpart? This PR focuses on BE side, are we going to implement actual logic to fetch values using "sort criteria" selected by user in a different one?

  1. For python SDK:
cd /human-protocol/packages/sdk/python/human-protocol-sdk
make generate-doc-md

For typescript SDK:

cd /human-protocol/packages/sdk/typescript/human-protocol-sdk
yarn build:doc
  1. Yes, as I mentioned in PR description, we need to link the filtering functions in the dashboard to the server part. But for now, the changes are fully compatible with the current version of the client.

@eugenvoronov
Copy link
Contributor Author

eugenvoronov commented Dec 17, 2024

@dnechay @portuu3 comments were resolved, ready for review

I want to point out that when users choose to sort leaders by reputation, leaders with a null reputation are currently not displayed. I haven't added logic in the assignReputationsToLeaders method to include these leaders in the final list yet in order not to complicate this method.. If this behavior is incorrect, please let me know, and I will adjust the logic accordingly.

And I updated reputation oracle GET /reputations endpoint params.

dnechay
dnechay previously approved these changes Jan 10, 2025
Copy link
Contributor

@dnechay dnechay left a comment

Choose a reason for hiding this comment

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

@portuu3 tested locally, seems working as expected

@portuu3 portuu3 merged commit fc709bc into develop Jan 13, 2025
20 checks passed
@portuu3 portuu3 deleted the feature/dashboard/leader-filter branch January 13, 2025 09:01
@dnechay dnechay mentioned this pull request Jan 13, 2025
23 tasks
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.

3 participants