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

Add some stats to the Trades view #357

Merged
merged 2 commits into from
Jun 13, 2018
Merged

Add some stats to the Trades view #357

merged 2 commits into from
Jun 13, 2018

Conversation

sindresorhus
Copy link
Contributor

Per discussion in Slack.

screen shot 2018-06-13 at 17 46 30

stats: await appContainer.swapDB.lastMonthStats(),
});
},
});
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Copy link
Member

@lukechilds lukechilds Jun 13, 2018

Choose a reason for hiding this comment

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

Niceeee, btw there is an index on swap.timeStarted so you should be able to query directly on that.

Something like:

this.db.find({
	selector: {timeStarted: {$gt: oneMonthAgo}},
	sort: [{timeStarted: 'desc'}],
});

Copy link
Member

Choose a reason for hiding this comment

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

Also might be nice to abstract it into swapDB.statsSince(timestamp) so we could easily reuse it for all time stats, weekly stats etc.

Copy link
Member

Choose a reason for hiding this comment

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

We could even add it directly into getSwaps().

So getSwaps() gets all swaps and getSwaps(timestamp) returns swaps since the timestamp.

Or maybe it would be better to add a PouchDB query object that gets merged with the default query.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Good idea. Fixed. I made a since and sortoption for convenience, but alsoquery` option that trump those for maximum power!

Copy link
Member

Choose a reason for hiding this comment

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

Awesome!

export default Trades;
export default withState(Trades, {}, {
async componentDidMount() {
/// TODO: This is only here temporarily until we move the swap stuff to the App container
Copy link
Member

Choose a reason for hiding this comment

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

What do you mean by this comment?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I plan to move the swap state to state in the AppContainer instead of being in the Exchange container as it's also needed by the Trades view, and it's awkward import the Exchange container there.

@lukechilds
Copy link
Member

Perfect 👌

@sindresorhus sindresorhus merged commit 7d996b4 into master Jun 13, 2018
@sindresorhus sindresorhus deleted the swap-stats branch June 13, 2018 16:28
@lukechilds
Copy link
Member

@sindresorhus I just noticed two issues with this.

  1. The number of currencies should take into account unique currencies in base and quote. Currently just taking into account quote means if I do trades on BTC/KMD, LTC/KMD and ETH/KMD it will only display as trading 1 currency. It should be 4.

  2. For whatever reason, PouchDB is actually not that fast. swapDB.getSwaps() takes around 250ms for me! But we have all swaps stored in exchangeContainer state. And these are guaranteed to be up to date because it subscribes to change events from Pouch and updates itself. So it would probably be considerably faster to just move the stats aggregation logic into exchangeContainer and run it over the full swap collection.

@sindresorhus
Copy link
Contributor Author

The number of currencies should take into account unique currencies in base and quote. Currently just taking into account quote means if I do trades on BTC/KMD, LTC/KMD and ETH/KMD it will only display as trading 1 currency. It should be 4.

#362

So it would probably be considerably faster to just move the stats aggregation logic into exchangeContainer and run it over the full swap collection.

You're basing this on just a few swaps though. What happens when the user has thousands of swaps? Eventually, we'll also want to move to pagination. So I don't think we should move away from the query interface just because it's a little bit slower on an insignificant amount of data.

@lukechilds
Copy link
Member

Fair point.

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.

None yet

2 participants