Skip to content
This repository has been archived by the owner on Sep 3, 2019. It is now read-only.

Leaderboard table #268

Merged
merged 21 commits into from
Jul 11, 2019
Merged

Leaderboard table #268

merged 21 commits into from
Jul 11, 2019

Conversation

BernardHan
Copy link
Contributor

@BernardHan BernardHan commented Jul 3, 2019

#267 #265

  • Create eventLeaderboard and globalLeaderboard tables and models

  • Make db helper methods for both tables

  • In sync, every bet, result set, vote amounts goes into eventLeaderboard investment

  • In sync, when event goes to WITHDRAWING status, update eventLeaderboard winnings and returnRatio

  • In sync, when event goes to WITHDRAWING status, update globalLeaderboard investments, winnings and returnRatio

  • Migration

  • Testings

  • Queries

@BernardHan BernardHan self-assigned this Jul 3, 2019
src/models/event-leaderboard.js Outdated Show resolved Hide resolved
src/models/event-leaderboard.js Outdated Show resolved Hide resolved
src/models/event-leaderboard.js Outdated Show resolved Hide resolved
src/models/global-leaderboard.js Outdated Show resolved Hide resolved
src/models/global-leaderboard.js Outdated Show resolved Hide resolved
src/db/db-helper.js Outdated Show resolved Hide resolved
src/db/index.js Outdated Show resolved Hide resolved
src/db/db-helper.js Outdated Show resolved Hide resolved
src/db/db-helper.js Outdated Show resolved Hide resolved
src/sync/bet-placed.js Show resolved Hide resolved
Copy link
Member

@dwalintukan dwalintukan left a comment

Choose a reason for hiding this comment

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

please make the last changes then can merge. dont forget to rebase.

oh forgot the tests, lets wait on merging until tests are done.

src/models/global-leaderboard.js Outdated Show resolved Hide resolved
src/models/event-leaderboard.js Outdated Show resolved Hide resolved
src/db/index.js Outdated Show resolved Hide resolved
src/sync/bet-placed.js Show resolved Hide resolved
src/sync/event-status-withdrawing-changed.js Outdated Show resolved Hide resolved
src/sync/event-status-withdrawing-changed.js Outdated Show resolved Hide resolved
// calculate winning for this user in this event
let winning = '0';
if (resultIndex === event.currentResultIndex) {
const res = await MultipleResultsEventApi.calculateWinnings({
Copy link
Member

Choose a reason for hiding this comment

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

we would still have the parallel limit issue here since the limit() is only for the outer promise. let me dig into this issue and lets leave this for now.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

can we move the limit to wrap this part so no inner promises?
We query the bets and resultsets first, then start to limit.

Copy link
Member

@dwalintukan dwalintukan Jul 8, 2019

Choose a reason for hiding this comment

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

Let's use https://caolan.github.io/async/v3/docs.html#eachOfSeries to handle the calculateWinnings and keep the outer limit promise. Would prefer not to limit the logic on the inside of an already parallel promise. Not too familiar with the logic of limit to start placing it in the middle of async parallel execution. Not too sure what would happen.

This will be slower since its running each calcWinnings individually but this ensures the concurrency limit is not broken.

src/sync/index.js Outdated Show resolved Hide resolved
src/db/index.js Outdated Show resolved Hide resolved
src/graphql/queries/leaderboard.js Outdated Show resolved Hide resolved
src/graphql/queries/leaderboard.js Show resolved Hide resolved
src/graphql/queries/leaderboard.js Show resolved Hide resolved
src/graphql/schema.js Outdated Show resolved Hide resolved
src/graphql/schema.js Outdated Show resolved Hide resolved
src/graphql/schema.js Outdated Show resolved Hide resolved
// calculate winning for this user in this event
let winning = '0';
if (resultIndex === event.currentResultIndex) {
const res = await MultipleResultsEventApi.calculateWinnings({
Copy link
Member

@dwalintukan dwalintukan Jul 8, 2019

Choose a reason for hiding this comment

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

Let's use https://caolan.github.io/async/v3/docs.html#eachOfSeries to handle the calculateWinnings and keep the outer limit promise. Would prefer not to limit the logic on the inside of an already parallel promise. Not too familiar with the logic of limit to start placing it in the middle of async parallel execution. Not too sure what would happen.

This will be slower since its running each calcWinnings individually but this ensures the concurrency limit is not broken.

@BernardHan BernardHan changed the base branch from dev-062419 to dev-070419 July 9, 2019 01:50
reject(err);
}
}));
async.forEachOf(filtered, (value, key, callback) => {
Copy link
Contributor Author

@BernardHan BernardHan Jul 9, 2019

Choose a reason for hiding this comment

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

forgot to change to use eachOfSeries . Will change and test later.

Copy link
Member

Choose a reason for hiding this comment

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

please change before this gets merged, eachOf still is doing parallel exec.

@@ -418,19 +418,19 @@ type Query {
skip: Int
): PaginatedBiggestWinner!

eventLeaderboard(
filter: LeaderboardFilter
eventLeaderboardEntries(
Copy link
Member

Choose a reason for hiding this comment

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

This should still be eventLeaderboard. Sorry wasn't clear. The eventLeaderboard query returns a list of LeaderboardEntrys. Like a users query will return a list of Users.

The relation is one-to-many. One eventLeaderboard query will return many LeaderboardEntrys.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Think eventLeaderboardEntries works better since we are querying for a set of paginated entries not the whole leaderboard.

In Event store, we only query for one specific entry just to get the winnings.

Copy link
Member

Choose a reason for hiding this comment

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

Fair enough. Actually it does conform to the other query names.


globalLeaderboard(
filter: LeaderboardFilter
globalLeaderboardEntries(
Copy link
Member

Choose a reason for hiding this comment

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

same thing here

reject(err);
}
}));
async.forEachOf(filtered, (value, key, callback) => {
Copy link
Member

Choose a reason for hiding this comment

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

please change before this gets merged, eachOf still is doing parallel exec.

@@ -418,19 +418,19 @@ type Query {
skip: Int
): PaginatedBiggestWinner!

eventLeaderboard(
filter: LeaderboardFilter
eventLeaderboardEntries(
Copy link
Member

Choose a reason for hiding this comment

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

Fair enough. Actually it does conform to the other query names.


if (orderBy && orderBy.length > 0) {
const { field } = orderBy[0];
res.items.sort((a, b) => new BigNumber(b[field]).comparedTo(new BigNumber(a[field]))); // all descending
Copy link
Member

Choose a reason for hiding this comment

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

what if trying to sort by address?

Copy link
Member

Choose a reason for hiding this comment

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

nit: suggest to maybe use BN.js in this case. not sure if the performance impacts with BigNumber would come into play here.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

field here is either investments or winnings, which are all converted from BigNumber instances, think it's better to keep it consistent to use BigNumber?

Copy link
Member

Choose a reason for hiding this comment

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

  1. Need to handle what happens if someone passes in { field: 'eventAddress', direction: 'ASC' } or betterAddress. It will crash here since you're trying to convert an address to a BigNumber.
  2. Was just worried about the performance of it. If you feel the performance is good compared to BN, then let's just keep it BigNumber. But would like to still use BN whenever possible to keep it standardized throughout the code.

Copy link
Contributor Author

@BernardHan BernardHan Jul 11, 2019

Choose a reason for hiding this comment

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

ratio is float type, think it's better keep it consistent to use Bignumber still

I will skip the sorting if it's address

Copy link
Member

Choose a reason for hiding this comment

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

Ah right, ya if dealing w/ floats, then BigNumber all the way.

test/db/index.js Outdated Show resolved Hide resolved
test/db/index.js Show resolved Hide resolved
test/db/index.js Outdated Show resolved Hide resolved
test/db/index.js Outdated Show resolved Hide resolved
test/db/index.js Outdated Show resolved Hide resolved
@BernardHan BernardHan merged commit 21c08ff into dev-070419 Jul 11, 2019
@dwalintukan dwalintukan deleted the leaderboard-table branch July 12, 2019 03:33
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants