Page MenuHomePhabricator

[ecash-herald] Sort agora tokens by volume instead of buy count
ClosedPublic

Authored by bytesofman on Wed, Jan 8, 00:32.

Details

Summary

Now that we have some tokens with regular volume, this is the better metric

Test Plan

npm test

Diff Detail

Repository
rABC Bitcoin ABC
Lint
Lint Not Applicable
Unit
Tests Not Applicable

Event Timeline

clean up comments, back out unrelated changes

bytesofman published this revision for review.Wed, Jan 8, 00:43
bytesofman added inline comments.
apps/ecash-herald/src/parse.ts
2384 ↗(On Diff #52034)

I didn't really understand typescript that well when I implemented it in ecash-herald

Define this type so we get ts benefits for this change

3402–3407 ↗(On Diff #52034)

this is the only code change in the diff

the rest is comments and assertions related to using typescript properly

Fabien added a subscriber: Fabien.
Fabien added inline comments.
apps/ecash-herald/src/parse.ts
3386 ↗(On Diff #52034)

Shouldn't this contain count and volume 0 instead of being undefined ? Sane defaults might be better in this case

3391 ↗(On Diff #52034)

Same question

3395 ↗(On Diff #52034)

Dito

This revision is now accepted and ready to land.Wed, Jan 8, 08:26
bytesofman added inline comments.
apps/ecash-herald/src/parse.ts
3386 ↗(On Diff #52034)

maybe -- there is an issue here, yes

the original design of "the key doesn't exist if this action never happened" is sane enough. it complicates sorting a map by this key though.

two options

1 - update the sort function so that we recognize "key is undefined" and do not need a default of 0
2 - update the type so the key is always defined.

option 1 is probably the way to go here, since we may have 100s of tokens and very few of them are expected to have all the possible actions.

I think it should be handled in another diff. what happened here is that identifying the typescript issue (to support the implementation of this change) ended up flagging this separate (already existing) issue. Because this diff doesn't change anything about how the issue is currently handled, but does add useful framework for supporting the fix, would be convenient to build on top of it.

3391 ↗(On Diff #52034)

same answer

3395 ↗(On Diff #52034)

yup will be fixed