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.

Sign up for GitHub

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

Jump to bottom

Add v0 REST APIs for circulating and total supply #10102

Merged
merged 3 commits into from
May 21, 2020
Merged

Add v0 REST APIs for circulating and total supply #10102

merged 3 commits into from
May 21, 2020

Conversation

mvines
Copy link
Member

@mvines mvines commented May 18, 2020

Some users would like a simple HTTP GET method to fetch the circulating and total supply, and sending an HTTP POST is apparently too much.

The RestApi translation feature of the jsonrpc_http_server looks tempting but is insufficient as it also only responds to HTTP POST requests.

In lieu of stepping into the longer term API rework (cc: ##7866), I went for creating one-off REST APIs at:

$ curl localhost:8899/v0/circulating-supply
683635295413590363
$ curl localhost:8899/v0/total-supply
1000000103431784983

These APIs are:

  1. Intentionally scary looking, /v0/...
  2. Intentionally not advertised in the documentation.

@mvines mvines force-pushed the v0r branch 6 times, most recently from 0ffd43b to 4c141c9 Compare May 19, 2020 06:24
@mvines mvines marked this pull request as ready for review May 19, 2020 06:24
@mvines mvines requested a review from CriesofCarrots May 19, 2020 06:24
mvines
mvines commented May 19, 2020
View reviewed changes
core/src/rpc_service.rs
@@ -239,6 +253,26 @@ impl RequestMiddleware for RpcRequestMiddleware {
}
}

fn process_rest(bank_forks: &Arc<RwLock<BankForks>>, path: &str) -> Option<String> {
match path {
Copy link
Member Author

Choose a reason for hiding this comment

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

I initially plumbed this to JsonRpcRequestProcessor::get_supply() to avoid the duplication, but that caused a circular reference. The duplication is pretty minimal and simple so not too worried.

@mvines mvines added the blocked Unable to proceed label May 19, 2020
@mvines
Copy link
Member Author

mvines commented May 19, 2020

(adding blocked flag because although I'd like to get this PR ready to go, I'm waiting for final confirmation that it'll solve the problem of those who are unable to HTTP POST)

@codecov
Copy link

codecov bot commented May 19, 2020 •
edited
Loading

Codecov Report

Merging #10102 into master will increase coverage by 0.0%.
The diff coverage is 92.1%.

@@           Coverage Diff           @@
##           master   #10102   +/-   ##
=======================================
  Coverage    81.5%    81.6%           
=======================================
  Files         280      280           
  Lines       65705    65757   +52     
=======================================
+ Hits        53607    53666   +59     
+ Misses      12098    12091    -7

CriesofCarrots
CriesofCarrots previously approved these changes May 19, 2020
View reviewed changes
Copy link
Contributor

@CriesofCarrots CriesofCarrots left a comment

Choose a reason for hiding this comment

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

Lgtm, although if we add much more to RequestMiddleware, we might want to reorganize to make the various functionalities more clear.

core/src/rpc_service.rs Outdated Show resolved Hide resolved
@mergify mergify bot dismissed CriesofCarrots’s stale review May 19, 2020 17:44

Pull request has been modified.

@mvines
Copy link
Member Author

mvines commented May 19, 2020

Lgtm, although if we add much more to RequestMiddleware, we might want to reorganize to make the various functionalities more clear.

💯. I'm hoping this won't set a precedent, but if so then yeah rpc totally needs a larger refactor

@mvines mvines removed the blocked Unable to proceed label May 21, 2020
@mvines
Copy link
Member Author

mvines commented May 21, 2020

I just received confirmation that this solution works, :shipit:

@mvines mvines merged commit 324cfd4 into solana-labs:master May 21, 2020
@mvines mvines deleted the v0r branch May 21, 2020 03:04
mergify bot pushed a commit that referenced this pull request May 21, 2020
@mvines
Add v0 REST APIs for circulating and total supply (#10102)
8fcb21c 
(cherry picked from commit 324cfd4)

# Conflicts:
#	core/src/non_circulating_supply.rs
#	core/src/rpc.rs
#	core/src/rpc_service.rs
@mergify mergify bot mentioned this pull request May 21, 2020
Merged
mergify bot pushed a commit that referenced this pull request May 21, 2020
@mvines
Add v0 REST APIs for circulating and total supply (#10102)
12d93f3 
(cherry picked from commit 324cfd4)

# Conflicts:
#	core/src/non_circulating_supply.rs
#	core/src/rpc_service.rs
@mergify mergify bot mentioned this pull request May 21, 2020
Merged
mvines added a commit that referenced this pull request May 21, 2020
@mvines
Add v0 REST APIs for circulating and total supply (#10102)
7f21b03 
(cherry picked from commit 324cfd4)
mvines added a commit that referenced this pull request May 21, 2020
@mvines
Add v0 REST APIs for circulating and total supply (#10102)
29a9fbc 
(cherry picked from commit 324cfd4)
solana-grimes pushed a commit that referenced this pull request May 21, 2020
@mergify
Add v0 REST APIs for circulating and total supply (bp #10102) (#10160)
e29b787 
automerge
solana-grimes pushed a commit that referenced this pull request May 21, 2020
@mergify
Add v0 REST APIs for circulating and total supply (#10102) (#10159)
4334fef 
automerge
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@CriesofCarrots CriesofCarrots CriesofCarrots left review comments

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
2 participants
@mvines @CriesofCarrots