-
Notifications
You must be signed in to change notification settings - Fork 87
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
Price API #354
Price API #354
Conversation
Updated with full implementation, both for current and historical prices.
|
Accidentally closed inside zenhub. Fixed. |
Update: in the next sprint, @LarryRuane would like to add a few tests, complete a review and merge this work. |
Any update on this? @LarryRuane |
Hi @nighthawk24, thanks for the reminder, sorry for the delay. I rebased this branch onto latest master (also resolving the merge conflicts), and pushed it to a new branch of my fork: https://github.com/LarryRuane/lightwalletd/tree/adityapk00-prices_stub Aditya, if that looks okay, I can force-push those commits onto your branch here (if I have permission, which I think I do). I don't like to force-push onto someone else's branch without asking first. I did some basic testing (on my branch), and your changes seem to work. If you and Adi are happy with it, I'd like to do just a little more review and then I'll push to master. I really don't think we need to spend a lot of effort adding tests, because this is kind of an add-on, not part of the critical core of lightwalletd. One problem I ran into when rebasing onto latest master is that running |
Looks good to me. Sorry about the whitespace thing - I use a windows machine and I think it does that by default. No problem from my side to reformat |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Adding partial review comments. I have yet to review the historical price functionality and the higher-up-the-call-stack code.
Bump. Please consider including this in the next major release. |
Force-pushed rebase to latest master branch, fixed a bug in the average price calculation, and also disabled this when running in darkside wallet test mode. I still need to address @defuse's comment, and see if I can clean up the code a bit. But this should be close to being able to merge. |
Force-pushed to fix some things (see commit message), I think this is very close to being able to merge, if the API is acceptable (@adityapk00 @nighthawk24 @pacu @ccjernigan). I would appreciate also if @defuse could take another look. I added @steven-ecc but don't feel any obligation. The only things I'm aware of that are needed before merging are:
|
baf3e7c
to
a7bd774
Compare
Force-pushed (twice, first, second) to make these changes:
I also did some manual testing: I turned off my internet, and the price fetches timed out within about 10 seconds. But during this time, the currrent price (which is cached every 15 minutes) can still be fetched using |
In case this is helpful to reviewers, you can test without an actual wallet as follows. This assumes you've started a local zcashd and a lightwalletd using this branch using the command-line arguments
You can obtain the current "Unix" time (seconds since 1970) this way:
You can find the Unix time for a given date like this (the date string is mostly free-format human readable):
|
Force-pushed again to remove test code. If you want to restore the test code for your own testing, run this patch command on the latest commit: Click to expand patch command
|
Force-pushed to rebase onto master, ping @steven-ecc @ccjernigan @defuse for review, please note that the following files are generated and don't need review:
|
@@ -272,6 +272,10 @@ func startServer(opts *common.Options) error { | |||
walletrpc.RegisterDarksideStreamerServer(server, service) | |||
} | |||
|
|||
if !opts.Darkside { | |||
common.StartPriceFetcher(dbPath, chainName) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
How will price be supported for darksidewalletd?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hmm, now that you mention it, I may have added that conditional in haste, incorrectly. I was thinking well of course we don't want lightwalletd to go out and ping these websites in dark mode, since it's supposed to be self-contained (no zcashd
) testing only. But maybe it's okay to do that.
OR, we could stub out the two price gRPCs and just return hardcoded fake data. That way internet access wouldn't be needed to do (full) darkside testing. Which do you think would be better?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
we should return stub data so integration tests can be reliable and deterministic
Rebased onto latest master, and add commit to support darksidewallet testing - return fixed price data in darkside mode. I think this should be ready to merge now (pending review). |
Force-pushed to address @daira's suggestion, now require at least 3 oracles. I also added a fourth oracle, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
utACK. Left some minor recommendation regarding an error message.
Force-pushed to resolve review suggestion, thanks, @pacu! |
Simplified locking; there is no need for a read-write mutex, this code is not performance-critical. Removed some of the go routines because they're not needed and the locking and concurrency are easier to reason about without them. NOTE: there is some test code left in here, search for "XXX testing" and remove before committing! This test code makes things happen faster: Instead of fetching prices every 15 minutes, do it every minute. Also, write historical data every minute, instead of once per day. Another change needed is to remove some of the logging. It's good for now during testing, but it's too much for production.
Force-pushed rebased |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM!
Just a doubt on API usage
The binance URL will need to be updated to |
Closing, I don't think we're going to be doing this, please reopen if needed, thanks. |
Indeed we don't want to do this; there is no authentication of the price data, so it would enable lightwalletd operators to manipulate the exchange rates being served to light clients. The |
Stub implementation for the Prices API. Mainly to review the API