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

feature(rpc): add real data to getinfo method #3660

Merged
merged 7 commits into from
Mar 1, 2022
Merged

feature(rpc): add real data to getinfo method #3660

merged 7 commits into from
Mar 1, 2022

Conversation

oxarbitrage
Copy link
Contributor

Motivation

Implement the getinfo RPC method with the fields used by lightwalletd. Close #3142 if merged.

Solution

I had a hard time trying to pass the app version into the method. This approach is with lazy_static. I am open to change this if i find something that works and it is better.

This PR does not have a specific test, i am unsure if the tests we will use will be acceptance or something else for each method. If they are acceptance then the test for this call should be added after #3641 as it is very similar to what we have now.

The code in this pull was tested manually:

$ curl -X POST -H "Content-Type: application/json" -d '{"jsonrpc": "2.0", "method": "getinfo", "params": [], "id":123 }' 127.0.0.1:8832 | jq
{
  "jsonrpc": "2.0",
  "result": {
    "build": "1.0.0-beta.5+22.g45caa63",
    "subversion": "/Zebra:1.0.0-beta.5/"
  },
  "id": 123
}
$ 

The docs for the method are in a discussion at #3638. Open to change the format.

Review

Probably @teor2345 will be the best option if they have time so i can have some feedback for my morning.

Reviewer Checklist

  • Lazy static implementation is acceptable.
  • If so, check the code as a whole.

@oxarbitrage oxarbitrage marked this pull request as draft February 27, 2022 22:57
@teor2345
Copy link
Contributor

This PR does not have a specific test, i am unsure if the tests we will use will be acceptance or something else for each method.

@oxarbitrage this seems like it might be a decision for the whole team. Did you want to put it on the engineering sync agenda for tomorrow?

@codecov
Copy link

codecov bot commented Feb 28, 2022

Codecov Report

Merging #3660 (3c6c7c7) into main (729535c) will decrease coverage by 0.08%.
The diff coverage is 33.33%.

@@            Coverage Diff             @@
##             main    #3660      +/-   ##
==========================================
- Coverage   80.06%   79.98%   -0.09%     
==========================================
  Files         284      288       +4     
  Lines       32740    32755      +15     
==========================================
- Hits        26214    26198      -16     
- Misses       6526     6557      +31     

Copy link
Contributor

@teor2345 teor2345 left a comment

Choose a reason for hiding this comment

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

I haven't tried this myself, but I think it might simplify things.

@oxarbitrage oxarbitrage marked this pull request as ready for review February 28, 2022 15:53
Copy link
Contributor

@teor2345 teor2345 left a comment

Choose a reason for hiding this comment

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

Looks good, let's check what tests we need with the team before we merge.

@oxarbitrage
Copy link
Contributor Author

Lets add some basic test here before we merge. This will also define the test structure for all the methods.

Copy link
Contributor

@teor2345 teor2345 left a comment

Choose a reason for hiding this comment

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

Looks great, thanks!

mergify bot added a commit that referenced this pull request Mar 1, 2022
@mergify mergify bot merged commit b3eb38d into main Mar 1, 2022
@mergify mergify bot deleted the issue3142 branch March 1, 2022 03:32
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.

getinfo JSON-RPC method
2 participants