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

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

Details

Reviewers

Fabien

Group Reviewers

Restricted Project

Commits

Summary

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

Test Plan

npm test

Repository

rABC Bitcoin ABC

Branch

herald-vol-sort

Lint

Lint Passed

Unit

No Test Coverage

Build Status

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	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	this is the only code change in the diff the rest is comments and assertions related to using typescript properly

Fabien accepted this revision.Wed, Jan 8, 08:26

Fabien added a subscriber: Fabien.

Fabien added inline comments.

apps/ecash-herald/src/parse.ts
3386	Shouldn't this contain count and volume 0 instead of being undefined ? Sane defaults might be better in this case
3391	Same question
3395	Dito

This revision is now accepted and ready to land.Wed, Jan 8, 08:26

bytesofman marked 3 inline comments as done.Wed, Jan 8, 11:29

bytesofman added inline comments.

apps/ecash-herald/src/parse.ts
3386	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	same answer
3395	yup will be fixed