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

Price API #354

Closed
wants to merge 5 commits into from
Closed

Price API #354

wants to merge 5 commits into from

Conversation

adityapk00
Copy link
Contributor

Stub implementation for the Prices API. Mainly to review the API

@adityapk00
Copy link
Contributor Author

Updated with full implementation, both for current and historical prices.

  • Historical prices are cached and stored locally
  • Current price is fetched from 3 sources, outliers discarded, then the median is used

@pacu pacu closed this Jul 1, 2021
@gmale gmale reopened this Jul 16, 2021
@gmale
Copy link
Contributor

gmale commented Jul 16, 2021

Accidentally closed inside zenhub. Fixed.

@gmale
Copy link
Contributor

gmale commented Jul 29, 2021

Update: in the next sprint, @LarryRuane would like to add a few tests, complete a review and merge this work.

@nighthawk24
Copy link

Any update on this? @LarryRuane
@adityapk00 do you have some bandwidth to resolve the merge conflicts?

@LarryRuane
Copy link
Collaborator

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 gofmt on the new file, common/prices.go, (which my editor automatically does) had the effect of changing every line. This is because every line ended with a control-M character. So this file had a DOS format, instead of Unix. We really need all code (except the stuff in vendor) to stay up-to-date formatted with gofmt; I hope this doesn't cause you any trouble. (If you diff with the ignore-whitespace in effect, such as git diff -b or similar, you'll see that there are no code changes.)

@adityapk00
Copy link
Contributor Author

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

Copy link
Collaborator

@defuse defuse left a 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.

common/prices.go Outdated Show resolved Hide resolved
@nighthawk24
Copy link

Bump. Please consider including this in the next major release.

@LarryRuane
Copy link
Collaborator

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.

walletrpc/service.proto Outdated Show resolved Hide resolved
@LarryRuane
Copy link
Collaborator

LarryRuane commented May 18, 2022

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:

  • undo some test-only code that will make this easier for reviewers to test and experiment with (search for "XXX testing")
  • remove some of the logging (but it's nice for manual testing)

@LarryRuane LarryRuane force-pushed the prices_stub branch 2 times, most recently from baf3e7c to a7bd774 Compare May 18, 2022 19:04
@LarryRuane
Copy link
Collaborator

Force-pushed (twice, first, second) to make these changes:

  • A call to GetCurrentPrice gRPC soon after startup returns a confusing error, "price too old"; instead return "starting up, prices not available yet"
  • Don't hold the mutex during price fetching. This allows GetCurrentPrice to return immediately if our price oracles are slow to respond.

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 GetCurrentPrice. (A historical price could be fetched if it had been fetched and cached before.)

@LarryRuane
Copy link
Collaborator

LarryRuane commented May 18, 2022

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 --zcash-conf-path ~/.zcash/zcash.conf --no-tls-very-insecure

$ grpcurl -plaintext  localhost:9067 cash.z.wallet.sdk.rpc.CompactTxStreamer/GetCurrentZECPrice
{
  "timestamp": "1652900224",
  "currency": "USD",
  "price": 102.69499999999998
}
$ grpcurl -plaintext -d '{"timestamp":1652895450, "currency":"USD"}' localhost:9067 cash.z.wallet.sdk.rpc.CompactTxStreamer/GetZECPrice
{
  "timestamp": "1652873850",
  "currency": "USD",
  "price": 114.12326794636196
}
$

You can obtain the current "Unix" time (seconds since 1970) this way:

$ date '+%s'
1652901832
$ 

You can find the Unix time for a given date like this (the date string is mostly free-format human readable):

$ date '+%s' --date='jan 15 2020'
1579071600
$

@LarryRuane
Copy link
Collaborator

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
patch -R -p1 <<EOF
--- a/common/prices.go
+++ b/common/prices.go
@@ -235,19 +235,14 @@ func GetCurrentPrice() (float64, error) {
 
 // return the time in YYYY-MM-DD string format
 func day(t time.Time) string {
-	// XXX testing: pretend every second is a new day:
-	return t.Format("2006-01-02-03-04-05")
-
-	// XXX testing: before merge, restore this:
-	//return t.Format("2006-01-02")
+	return t.Format("2006-01-02")
 }
 
 // GetHistoricalPrice returns the price for the given day, but only
 // accurate to day granularity.
 func GetHistoricalPrice(ts time.Time) (float64, *time.Time, error) {
 	dt := day(ts)
-	// XXX testing (change back to "2006-01-02" before merging)
-	canonicalTime, err := time.Parse("2006-01-02-03-04-05", dt)
+	canonicalTime, err := time.Parse("2006-01-02", dt)
 	if err != nil {
 		return -1, nil, err
 	}
@@ -344,12 +339,8 @@ func StartPriceFetcher(dbPath string, chainName string) {
 				latestPriceTime = time.Now()
 				mutex.Unlock()
 			}
-
-			// XXX testing: run each minute
-			time.Sleep(1 * time.Minute)
-
-			// XXX testing: before merge, restore this:
-			//time.Sleep(15 * time.Minute)
+			// price data 15 minutes out of date is probably okay
+			time.Sleep(15 * time.Minute)
 		}
 	}()
 }
EOF

@LarryRuane
Copy link
Collaborator

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:

  • docs/rtd/index.html
  • walletrpc/service.pb.go
  • walletrpc/service_grpc.pb.go

@@ -272,6 +272,10 @@ func startServer(opts *common.Options) error {
walletrpc.RegisterDarksideStreamerServer(server, service)
}

if !opts.Darkside {
common.StartPriceFetcher(dbPath, chainName)
Copy link
Contributor

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?

Copy link
Collaborator

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?

Copy link
Contributor

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

@LarryRuane LarryRuane changed the title Price API stub implementation Price API Jun 23, 2022
@LarryRuane
Copy link
Collaborator

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).

common/prices.go Outdated Show resolved Hide resolved
@LarryRuane
Copy link
Collaborator

Force-pushed to address @daira's suggestion, now require at least 3 oracles. I also added a fourth oracle, coingecko (which we're already using for historical data, but it can also provide current data), because otherwise all oracles would need to be available. This way, one of them can be unreachable and we can still establish a current price.

Copy link
Contributor

@pacu pacu left a 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.

frontend/service.go Outdated Show resolved Hide resolved
@LarryRuane
Copy link
Collaborator

Force-pushed to resolve review suggestion, thanks, @pacu!

adityapk00 and others added 5 commits July 21, 2022 11:32
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.
@LarryRuane
Copy link
Collaborator

Force-pushed rebased

Copy link
Contributor

@pacu pacu left a 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

walletrpc/service.proto Show resolved Hide resolved
@defuse
Copy link
Collaborator

defuse commented Feb 13, 2023

The binance URL will need to be updated to ZECBUSD, see: adityapk00#12

@LarryRuane
Copy link
Collaborator

Closing, I don't think we're going to be doing this, please reopen if needed, thanks.

@LarryRuane LarryRuane closed this Feb 1, 2024
@str4d
Copy link
Collaborator

str4d commented Sep 12, 2024

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 zcash_client_backend 0.13 Rust crate adds APIs for doing effectively the same querying as this PR, but light clients themselves query the exchanges so they get authenticated exchange rate data (and do so over Tor for IP hiding).

@sourcesolver sourcesolver mentioned this pull request Oct 10, 2024
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.