Skip to content

Implementing path finding without Horizon#170

Merged
charlie-wasp merged 17 commits intomasterfrom
feature/path-finding
Jun 18, 2019
Merged

Implementing path finding without Horizon#170
charlie-wasp merged 17 commits intomasterfrom
feature/path-finding

Conversation

@charlie-wasp
Copy link
Contributor

@charlie-wasp charlie-wasp commented Jun 6, 2019

This change is Reviewable

@charlie-wasp charlie-wasp marked this pull request as ready for review June 10, 2019 09:56
@charlie-wasp charlie-wasp requested a review from a team as a code owner June 10, 2019 09:56
@charlie-wasp charlie-wasp force-pushed the feature/path-finding branch from 9a11615 to d8f4b43 Compare June 17, 2019 12:32
@nebolsin nebolsin force-pushed the feature/path-finding branch from 11000b5 to 9cf0391 Compare June 17, 2019 17:53
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.

Added a few tini comments, otherwise :lgtm:

Reviewed 7 of 15 files at r1, 10 of 14 files at r2, 4 of 4 files at r3, 7 of 8 files at r4, 6 of 6 files at r5.
Reviewable status: all files reviewed, 2 unresolved discussions (waiting on @charlie-wasp)


src/service/dex/offers_listener.ts, line 31 at r5 (raw file):

      const offers = await getRepository(Offer).find({ where: pair });
      updateOffersGraph(pair.selling, pair.buying, offers);
    });

Shouldn't we use something like asyncForEach here? I think otherwise subscription callback can complete before those pair updates are still working.

// await asyncForEach(assetPairs, async (pair) => { ... })
export async function asyncForEach<T>(array: T[], mapper: (x: T) => Promise<void>): Promise<void[]> {
  return Promise.all(array.map(mapper));
}

src/service/dex/offers_graph.ts, line 123 at r5 (raw file):

    //   return value;
    // }); // cloning

✂️

@charlie-wasp charlie-wasp merged commit f1230cf into master Jun 18, 2019
@charlie-wasp charlie-wasp deleted the feature/path-finding branch June 18, 2019 09:40
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