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.

Sign up for GitHub

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

Jump to bottom

feat: leaderboard endpoints #122

Merged
merged 33 commits into from
Nov 8, 2023
Merged

feat: leaderboard endpoints #122

merged 33 commits into from
Nov 8, 2023

Conversation

ayushtom
Copy link
Contributor

@ayushtom ayushtom commented Nov 1, 2023 •
edited
Loading

closes #98
closes #97

@Th0rgal Th0rgal self-requested a review November 1, 2023 13:01
@ayushtom
Copy link
Contributor Author

ayushtom commented Nov 2, 2023

Hey everyone
I have made the below doc to help with testing. LMK if you face any difficulties
https://app.skiff.com/file/ec0d5b6b-06f9-4eb7-b396-f66e88d1fd4e

Th0rgal
Th0rgal requested changes Nov 2, 2023
View reviewed changes
src/endpoints/leaderboard/get_ranking.rs Outdated Show resolved Hide resolved
src/endpoints/leaderboard/get_ranking.rs Outdated Show resolved Hide resolved
src/endpoints/leaderboard/get_ranking.rs Outdated Show resolved Hide resolved
src/endpoints/leaderboard/get_ranking.rs Outdated Show resolved Hide resolved
src/endpoints/leaderboard/get_static_info.rs Outdated Show resolved Hide resolved
@Th0rgal Th0rgal self-requested a review November 3, 2023 05:26
Th0rgal
Th0rgal reviewed Nov 3, 2023
View reviewed changes
Copy link
Member

@Th0rgal Th0rgal left a comment

Choose a reason for hiding this comment

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

Here are some comments on the pipelines

src/endpoints/leaderboard/get_ranking.rs Show resolved Hide resolved
src/endpoints/leaderboard/get_ranking.rs Show resolved Hide resolved
@ayushtom
Copy link
Contributor Author

ayushtom commented Nov 3, 2023 •
edited
Loading

Regarding this comment

I have put i64 in places because we use multiple math operations to figure out the shifted range. One of them being here and here . This is why i have kept all of them i64 to avoid type conversions while any mathematical conversion.

@ayushtom ayushtom marked this pull request as ready for review November 3, 2023 09:10
@ayushtom ayushtom requested a review from Th0rgal November 3, 2023 11:16
@ayushtom ayushtom added the 🔥 Ready for review This pull request needs a review label Nov 3, 2023
@ayushtom ayushtom force-pushed the ayush/leaderboard-endpoints branch from 4c83f19 to d5bdd43 Compare November 5, 2023 22:53
@ayushtom ayushtom changed the title Ayush/leaderboard endpoints feat: leaderboard endpoints Nov 7, 2023
Th0rgal
Th0rgal approved these changes Nov 8, 2023
View reviewed changes
Copy link
Member

@Th0rgal Th0rgal left a comment

Choose a reason for hiding this comment

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

lgtm

src/endpoints/leaderboard/get_ranking.rs
/*
user address
*/
addr: String,
Copy link
Member

Choose a reason for hiding this comment

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

Please use a FieldElement here. You can then use the to_string() method to convert it to the correct hex decimal string to query the db. This allows us to support both hex and decimal addresses. This is something legacy because in all our other dbs we now use to_hex instead of to_string(), but that's the same concept.

@Th0rgal Th0rgal merged commit 0cbc9a3 into master Nov 8, 2023
1 check passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@Th0rgal Th0rgal Th0rgal approved these changes

Assignees
No one assigned
Labels
🔥 Ready for review This pull request needs a review
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Create leaderboard/get_static_info endpoint Create leaderboard/get_ranking endpoint
2 participants
@ayushtom @Th0rgal