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

RPC Overhaul (WIP) #960

Closed
iFoggz opened this issue Feb 20, 2018 · 8 comments
Closed

RPC Overhaul (WIP) #960

iFoggz opened this issue Feb 20, 2018 · 8 comments
Assignees

Comments

@iFoggz
Copy link
Member

iFoggz commented Feb 20, 2018

This is a WIP and thought i'd post an issue so people can chime in and know what will be happening over the next while as i'm in between bug diagnostics and fixes as well and improvements overall. some will increase performance as well. I been in bitcoin code learning about all the improvements etc they have made in this field.

Improvements:

  • Remove the LOCK for cs_main pwalletMain->cs_wallet from the RPC caller itself
  • Removing these locks from there and placing the locks in the rpc calls the same way bitcoin did improved performance there and eliminate unnecessary locking when incorrect amount of parameters were set thus throwing the json runtime. Its not necessary and counterproductive to lock to throw the help when incorrect amount of parameters are entered. Currently if there was a delay waiting for a lock you could wait for the runtime error reply this should not be the case.
  • Added categories to help rpc command to sort all the rpc functions into appropriate categories.
  • Fixed and add proper ConvertTo for parameter types and missing conversions as well. (ex listtransactions was missing bool etc)
  • Adding locks to the rpc functions itself also allows better control and shorten the scopes of the locks in turn freeing them sooner then later.
  • Also improves from locking both cs_main and pwalletMain->cs_wallet when only one of the locks are required. (Keep in mind gridcoin has some differences in commands and i will keep both locks when necessary)
  • Move the execute and list functions to RPC calls without the need for list and execute.
  • If any locks are required in the previous execute or list functions they will be added
  • If any conversions are needed they from execute and list they will be added properly to use the RPC as it was intended.
  • Help command will display help information with categories so user can go help wallet etc
  • All original RPC functions will not be affected they will still be entered the same
  • Fixing some broken RPC calls and eliminating any duplicates
  • Adding debug flag for rpc response times for dev testing

This is WIP and im working on it when i can thou been tied up lately. I'm about 40% complete and its running smoothly and looks nice. Will update this and let everyone know of more information when I have time and available.

If there are any requests as to additions etc do comment and make me aware.

@denravonska thought I should post this as some were asking what kind of improvements they will see. Please add the enhancement tag and if u feel the need to add bug fix for the missing conversions and broken rpc calls. this will be a good improvement for the wallet RPC.

@tomasbrod
Copy link
Member

Why is this an issue, not a PR?
You are great, bu picking up on this.
To be clear, I am strongly against including into the next release, maybe the one after.

@iFoggz
Copy link
Member Author

iFoggz commented Feb 23, 2018

Wouldn't be an enhancement tag etc. Issue is also to allow input as well as requests that could be helpful for users and rpc.

@tomasbrod
Copy link
Member

Now I think i get it. This is summary of what you are going to do.

@iFoggz
Copy link
Member Author

iFoggz commented Feb 24, 2018

yes summary of work in progress i'm partially done just been on hold
XD

@tomasbrod
Copy link
Member

Does this update #527 in any way?

@iFoggz
Copy link
Member Author

iFoggz commented Apr 30, 2018

no clue i plan some clean up in how data is shown and grouped.

@denravonska
Copy link
Member

All/most of these are done now, right?

@iFoggz
Copy link
Member Author

iFoggz commented May 28, 2018

yes we are and @TheCharlatan took on the other parts. closing

@iFoggz iFoggz closed this as completed May 28, 2018
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

No branches or pull requests

3 participants