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

Asset holders query #155

Merged
merged 7 commits into from
Apr 23, 2019
Merged

Asset holders query #155

merged 7 commits into from
Apr 23, 2019

Conversation

charlie-wasp
Copy link
Contributor

@charlie-wasp charlie-wasp commented Apr 15, 2019

Here's asset holders query implementation. It somewhat depends on #154 in technical details regarding pagination, but all necessary parts are duplicated, so this PR can be merged first

We may want to add index on assetcode + issuer fields in trustlines table to speed things up


This change is Reviewable

@charlie-wasp charlie-wasp requested a review from a team April 15, 2019 12:26
@charlie-wasp charlie-wasp force-pushed the feature/asset-holders branch from dc6ea74 to fa92ea9 Compare April 15, 2019 12:36
@charlie-wasp charlie-wasp force-pushed the feature/asset-holders branch from fa92ea9 to dca3ddb Compare April 15, 2019 12:40
Copy link
Member

@nebolsin nebolsin left a comment

Choose a reason for hiding this comment

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

Reviewed 12 of 12 files at r1.
Reviewable status: all files reviewed, 4 unresolved discussions (waiting on @charlie-wasp)


src/repo/assets.ts, line 68 at r1 (raw file):

      if (paging.after) {
        queryBuilder.where("balance < ?", balance);

We should use <= balance/>= balance and also introduce strict condition on accountid from the cursor. Currently the pagination will stop as soon as you reach a first zero-balance account, because the next page request will result in where balance < 0.


src/schema/assets.ts, line 18 at r1 (raw file):

    code: AssetCode!
    "All accounts that trust this asset, ordered by balance"
    holders(first: Int, last: Int, after: String, before: String): BalanceConnection

Let's call it balances for better consistency with the result data type and Account side.


src/schema/assets.ts, line 36 at r1 (raw file):

    flags: AccountFlags
    "All accounts that trust this asset, ordered by balance"
    holders(first: Int, last: Int, after: String, before: String): BalanceConnection

I'm not sure we need to introduce holders to both Asset and AssetWithInfo. And by the way, do we still need those two separate types? Looks like the Asset type and AssetID scalar provide exactly the same information.


src/util/paging.ts, line 31 at r1 (raw file):

export function properlyOrdered(records: any[], pagingParams: PagingParams): any[] {
  if (pagingParams.last && pagingParams.before) {

Hmm, I believe that presence of before param should not affect our ordering in any way, should it?

@charlie-wasp charlie-wasp requested a review from nebolsin April 15, 2019 14:41
Copy link
Contributor Author

@charlie-wasp charlie-wasp left a comment

Choose a reason for hiding this comment

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

Reviewable status: 9 of 12 files reviewed, 4 unresolved discussions (waiting on @nebolsin)


src/repo/assets.ts, line 68 at r1 (raw file):

Previously, nebolsin (Sergey Nebolsin) wrote…

We should use <= balance/>= balance and also introduce strict condition on accountid from the cursor. Currently the pagination will stop as soon as you reach a first zero-balance account, because the next page request will result in where balance < 0.

Done.


src/schema/assets.ts, line 18 at r1 (raw file):

Previously, nebolsin (Sergey Nebolsin) wrote…

Let's call it balances for better consistency with the result data type and Account side.

Done.


src/schema/assets.ts, line 36 at r1 (raw file):

Previously, nebolsin (Sergey Nebolsin) wrote…

I'm not sure we need to introduce holders to both Asset and AssetWithInfo. And by the way, do we still need those two separate types? Looks like the Asset type and AssetID scalar provide exactly the same information.

Well, AssetWithInfo is the type for a data, which Horizon returns. It contains additional amount and numAccounts fields. We would be able to reduce them to the single type, when we'll fetch assets from core database.

If we don't want to get a list of assets with corresponding holders, we can drop holders from here


src/util/paging.ts, line 31 at r1 (raw file):

Previously, nebolsin (Sergey Nebolsin) wrote…

Hmm, I believe that presence of before param should not affect our ordering in any way, should it?

Oh my god, this pagination discussion seems to be infinite 😅 Give me a sec... We need to reverse records on backward motion, because we call invertSortOrder, if before is set (look at parseCursorPagination method above). Quite opaque and convoluted logic 😔

@charlie-wasp charlie-wasp force-pushed the feature/asset-holders branch from 94f7135 to 4bc4cc5 Compare April 15, 2019 15:27
@charlie-wasp charlie-wasp force-pushed the feature/asset-holders branch from 805a68c to 9ba34d5 Compare April 15, 2019 20:14
Copy link
Member

@nebolsin nebolsin left a comment

Choose a reason for hiding this comment

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

:lgtm:

Reviewed 2 of 3 files at r2, 2 of 3 files at r3, 1 of 1 files at r4.
Reviewable status: :shipit: complete! all files reviewed, all discussions resolved

@charlie-wasp charlie-wasp merged commit 558b8b3 into master Apr 23, 2019
@nebolsin nebolsin deleted the feature/asset-holders branch November 15, 2019 17:48
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