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 changes (Large) #1024

Merged
merged 28 commits into from
Mar 17, 2018
Merged

Conversation

iFoggz
Copy link
Member

@iFoggz iFoggz commented Mar 15, 2018

I know it is a lot of commits but it was alot of changes and some refactoring as well. merge conflicts now resolved. will still touch this during testing and modify and verify locks.

all list and execute commands are now normal rpc calls.
fDebug4 now logs the rpc call times from inside the rpc. (note calling the daemon to connect to port and call a command can also add time ofcourse.

plan to remove some deprecated commands still but leaving this open for now in case some issues need to be addressed

@iFoggz
Copy link
Member Author

iFoggz commented Mar 15, 2018

some WIP still

@iFoggz
Copy link
Member Author

iFoggz commented Mar 15, 2018

@barton2526 u can start spell checking.

@iFoggz
Copy link
Member Author

iFoggz commented Mar 15, 2018

i will address compile issues as well


// We no longer use the unlocked feature here.
// Bitcoin has removed this option and placed the locks inside the rpc calls to reduce the scope
// Also removes the un needed locking when the end result is a rpc run time error reply over params!
Copy link
Member

Choose a reason for hiding this comment

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

should be "unneeded"

std::string sType = params[2].get_str();
entry.push_back(Pair("Type",sType));
std::string sPass = "";
sPass = (sType=="project" || sType=="projectmapping") ? GetArgu
Copy link
Member

@barton2526 barton2526 Mar 15, 2018

Choose a reason for hiding this comment

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

Spelling error. Should be "Requests". Also should be "quorum"

std::string sType = params[2].get_str();
entry.push_back(Pair("Type",sType));
std::string sPass = "";
sPass = (sType=="project" || sType=="projectmapping") ? GetArgu
Copy link
Member

Choose a reason for hiding this comment

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

Suggest rephrasing to "Displays a report on various versions that have recently staked on the network\n"

std::string sType = params[2].get_str();
entry.push_back(Pair("Type",sType));
std::string sPass = "";
sPass = (sType=="project" || sType=="projectmapping") ? GetArgu
Copy link
Member

Choose a reason for hiding this comment

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

Should be "voters" plural

std::string sType = params[2].get_str();
entry.push_back(Pair("Type",sType));
std::string sPass = "";
sPass = (sType=="project" || sType=="projectmapping") ? GetArgu
Copy link
Member

Choose a reason for hiding this comment

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

Point to Community Tasks Repo

std::string sType = params[2].get_str();
entry.push_back(Pair("Type",sType));
std::string sPass = "";
sPass = (sType=="project" || sType=="projectmapping") ? GetArgu
Copy link
Member

Choose a reason for hiding this comment

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

Needs better description

std::string sType = params[2].get_str();
entry.push_back(Pair("Type",sType));
std::string sPass = "";
sPass = (sType=="project" || sType=="projectmapping") ? GetArgu
Copy link
Member

Choose a reason for hiding this comment

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

Unix time does not have a timezone. Should be rephrased as "Displays current unix time as well as date and time in UTC\n"

std::string sType = params[2].get_str();
entry.push_back(Pair("Type",sType));
std::string sPass = "";
sPass = (sType=="project" || sType=="projectmapping") ? GetArgu
Copy link
Member

Choose a reason for hiding this comment

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

Spelling error. Should say "Phrase"

std::string sType = params[2].get_str();
entry.push_back(Pair("Type",sType));
std::string sPass = "";
sPass = (sType=="project" || sType=="projectmapping") ? GetArgu
Copy link
Member

Choose a reason for hiding this comment

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

Remove "so"

"execute function has been deprecated; run the command as previously done but without execute"

}
return results;

throw JSONRPCError(RPC_DEPRECATED, "list is deprecated; Please run the command the same as previously without list");
Copy link
Member

Choose a reason for hiding this comment

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

Suggest same format as execute deprecate

@@ -23,9 +23,11 @@ Value getconnectioncount(const Array& params, bool fHelp)
if (fHelp || params.size() != 0)
throw runtime_error(
"getconnectioncount\n"
"Returns the number of connections to other nodes.");
"\n"
"Returns the number of connections to other node\n.");
Copy link
Member

@barton2526 barton2526 Mar 15, 2018

Choose a reason for hiding this comment

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

Needs plural. Should be "Returns the number of connections to other nodes\n"

"Returns data about each connected network node.");
"getpeerinfo\n"
"\n"
"Returns data about each connected network node.");
Copy link
Member

Choose a reason for hiding this comment

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

Needs newline

" ]\n"
"2. \"outputs\" (string, required) a json object with outputs\n"
" {\n"
" \"address\": x.xxx (numeric, required) The key is the bitcoin address, the value is the CURRENCY_UNIT amount\n"
Copy link
Member

Choose a reason for hiding this comment

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

Should say Gridcoin address, not bitcoin

"2. minconf (numeric, optional, default=1) Only include transactions confirmed at least this many times.\n"
"3. includeWatchonly (bool, optional, default=false) Also include balance in watchonly addresses (see 'importaddress')\n"
"\nResult:\n"
"amount (numeric) The total amount in btc received for this account.\n"
Copy link
Member

Choose a reason for hiding this comment

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

Change btc to grc

" \"involvesWatchonly\" : \"true\", (bool) Only returned if imported addresses were involved in transaction\n"
" \"address\" : \"receivingaddress\", (string) The receiving address\n"
" \"account\" : \"accountname\", (string) The account of the receiving address. The default account is \"\".\n"
" \"amount\" : x.xxx, (numeric) The total amount in btc received by the address\n"
Copy link
Member

Choose a reason for hiding this comment

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

change btc to grc

" \"address\" : \"receivingaddress\", (string) The receiving address\n"
" \"account\" : \"accountname\", (string) The account of the receiving address. The default account is \"\".\n"
" \"amount\" : x.xxx, (numeric) The total amount in btc received by the address\n"
"\nExamples:\n"
Copy link
Member

Choose a reason for hiding this comment

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

No examples are given

" {\n"
" \"account\":\"accountname\", (string) The account name associated with the transaction. \n"
" It will be \"\" for the default account.\n"
" \"address\":\"bitcoinaddress\", (string) The bitcoin address of the transaction. Not present for \n"
Copy link
Member

Choose a reason for hiding this comment

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

Change "bitcoin" to "gridcoin"

" transaction between accounts, and not associated with an address,\n"
" transaction id or block. 'send' and 'receive' transactions are \n"
" associated with an address, transaction id and block details\n"
" \"amount\": x.xxx, (numeric) The amount in btc. This is negative for the 'send' category, and for the\n"
Copy link
Member

Choose a reason for hiding this comment

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

Change btc to grc

" \"amount\": x.xxx, (numeric) The amount in btc. This is negative for the 'send' category, and for the\n"
" 'move' category for moves outbound. It is positive for the 'receive' category,\n"
" and for the 'move' category for inbound funds.\n"
" \"fee\": x.xxx, (numeric) The amount of the fee in btc. This is negative and only available for the \n"
Copy link
Member

Choose a reason for hiding this comment

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

change btc to grc

"Repair wallet if checkwallet reports any problem.\n");
"repairwallet\n"
"\n"
"Repair wallet if checkwallet reports any problem.\n");
Copy link
Member

Choose a reason for hiding this comment

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

Suggest making problem plural. "Repair wallet if checkwallet reports any problems.\n"

@barton2526
Copy link
Member

@Foggyx420 Initial spellcheck complete. Please let me know if anything I commented is unclear. Apologies to all for the suggestions spam!

"wallet --------> Returns help for blockchain related commands\n"
"mining --------> Returns help for neural network/cpid/beacon related commands\n"
"developer -----> Returns help for developer commands\n"
"network -------> Returns help for network related commands\n");
Copy link

Choose a reason for hiding this comment

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

This returns with (code -1) and in red in the Wallet rpcconsole

Copy link
Member Author

Choose a reason for hiding this comment

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

coudl be i'll check and i wouldnt be surprised as the merge conflicts from a horror movies i had to work out lol.

Copy link
Member Author

Choose a reason for hiding this comment

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

ya categories work i just got to fix the pass through for help from the commands. which was fine before merging recent changes. no problems. will do it later today

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 can confirm that help works help wallet help mining help developer and help network works but help commandshere does not however that will be a easy fix i remember i didnt save the fix for that for some reason.

@tomasbrod
Copy link
Member

Instead of posting all these comments, @barton2526, you could have done the correcting change and submitted a PR to a PR.

@TheCharlatan
Copy link
Contributor

Nice stuff! Can you convert the printf to the new tinyformat LogPrintf?

@denravonska
Copy link
Member

denravonska commented Mar 16, 2018

Merging this after the printf changes and compile issues so we can flesh it out in staging.

@denravonska denravonska merged commit 0c7b622 into gridcoin-community:development Mar 17, 2018
@iFoggz
Copy link
Member Author

iFoggz commented Mar 17, 2018

sorry guy i been busy and sick. will fix the spelling issues etc and post a new pr since this has been accepted. do test away and barton will cprdinate this

@barton2526
Copy link
Member

We have largely finished stress testing this in testnet and are now moving onto testing the individual RPC commands for correct output and errors.

@tomasbrod
Copy link
Member

Please close #960

denravonska added a commit that referenced this pull request May 25, 2018
Fixed
 - Fixes for displaying on high DPI displays, #517 (@skcin).
 - Re-enable unit tests, add unit test to Travis, #769, #808 (@TheCharlatan).
 - Fix empty string in sendalert2 (@tomasbrod).

Added
 - Neural Report RPC command, #1063 (@tomasbrod).
 - GUI wallet redign with new icons and purple native style (@skcin).

Changed
 - Switch to autotools and Depends from Bitcoin, #487 (@TheCharlatan).
 - Clean and update docs for new build system, remove outdated, #828 (@TheCharlatan).
 - Change estimated time to stake calculations to be more accurate, #1084 (@jamescowens).
 - Move logging to tinyformat, #1009 (@TheCharlatan).
 - Improve appcache performance, #734 (@denravonska).
 - Improve block index memory access performance, #679 (@denravonska).
 - NN fixes: clean logging, explain mag single response, move contract to ndata_nresp (@denravonska)
 - Updated translations:
    - Turkish, #771 (@confuest).
    - Chinese, #1012 (@linnaea).
 - RPC refactor: Cleaner locks, better error handling, move execute calls to straght rpc calls, #1024 (@Foggyx420).
 - Change locking primitives from Boost to STL, #1029 (@Foggyx420).

Removed
 - gridcoindiagnostic RPC call (@denravonska).
 - Galaza, #945 (@barton2526).
 - Assertion in SignSignature, #998 (@TheCharlatan).
 - Upgrade menu, #1094 (@jamescowens).
 - Acid test functions, #871 (@tomasbrod).
 - Qt4 support, #801 (@denravonska).
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.

None yet

5 participants