Skip to content

Remove/merge redundant RPC calls #1232

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
Merged
merged 17 commits into from
Aug 25, 2018
Merged

Remove/merge redundant RPC calls #1232

merged 17 commits into from
Aug 25, 2018

Conversation

iFoggz
Copy link
Member

@iFoggz iFoggz commented Jul 27, 2018

Issue #1228

@denravonska denravonska added this to the Camilla milestone Jul 29, 2018
@denravonska denravonska mentioned this pull request Jul 29, 2018
Merged
@denravonska
Copy link
Member

denravonska commented Jul 29, 2018 •
edited
Loading

Regarding cpids and validcpids, here's what I'd like us to do. Merge them into projects. For each project, do:

if is_participating:
   UniValue entry;
   entry.PushKV("CPID", project_cpid);
   entry.PushKV("RAC", rac);
   entry.PushKV("Team", team);
   project.PushKV("Researcher", entry);

So the output should look something like:

....
  {
    "Project": "yafu",
    "URL": "http://yafu.myfirewall.org/yafu/"
  },
  {
    "Project": "yoyo@home",
    "URL": "http://www.rechenkraft.net/yoyo/",
    "Researcher" {
        "CPID": "abcdefgh",
        "RAC": 123,
        "Team": "not_gridcoin"
    }
}

I think that should retain the information people want from the existing commands. After that we can remove cpids and validcpids.

Edit: We might not even need to show the RAC here.

@tomasbrod
Copy link
Member

Agree with @denravonska , that is some nice output.
What is the alternative for list rsa now?

@iFoggz
Copy link
Member Author

iFoggz commented Jul 30, 2018 •
edited
Loading

will redo that*

@tomasbrod tomasbrod mentioned this pull request Jul 31, 2018
Closed
denravonska
denravonska reviewed Aug 4, 2018
View reviewed changes
src/rpcblockchain.cpp Outdated
@@ -2242,6 +1897,28 @@ UniValue projects(const UniValue& params, bool fHelp)
entry.pushKV("Project", sProjectName);
entry.pushKV("URL", sProjectURL);

if (mvCPIDs.size() > 0)
Copy link
Member

@denravonska denravonska Aug 4, 2018 •
edited
Loading

Choose a reason for hiding this comment

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

Some small changes:

  • Change size check to mvCPIDs.empty(). Same when doing HarvestCPIDs.
  • Do a find (like this) on mvCPIDs or else we insert an element if it was missing before.
  • Merge the structcpid.initialized check with the IsResearcher checks below it to remove one depth.

denravonska
denravonska reviewed Aug 4, 2018
View reviewed changes
src/rpcblockchain.cpp
@@ -1422,34 +1241,6 @@ UniValue magnitude(const UniValue& params, bool fHelp)
return results;
}

UniValue mymagnitude(const UniValue& params, bool fHelp)
{
Copy link
Member

@denravonska denravonska Aug 4, 2018 •
edited
Loading

Choose a reason for hiding this comment

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

We should make magnitude default to the user's current CPID now.

Copy link
Member Author

Choose a reason for hiding this comment

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

so rename that to magnitude and simply display the magnitude of the user (current user in msPrimaryCPID?)
and cpid

Copy link
Member

@denravonska denravonska Aug 4, 2018 •
edited
Loading

Choose a reason for hiding this comment

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

We can reuse the current magnitude :) Just make it default to msPrimaryCPID if the user doesn't specify one.

Copy link
Member Author

Choose a reason for hiding this comment

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

lost train on thought of where this one was to be changed since mymagnitude is removed

Copy link
Member Author

Choose a reason for hiding this comment

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

added it to magnitude report. maybe that will satisfy instead of re adding this rpc

Copy link
Member

Choose a reason for hiding this comment

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

I was thinking we change magnitude to default to the user's CPID:

Change

std::string cpid;
if (params.size() > 0)
    cpid = params[0].get_str();

to

const std::string cpid = !params.empty()
        ? params[0]
        : msPrimaryCPID;

if(cpid.empty())
   throw... something

Copy link
Member Author

Choose a reason for hiding this comment

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

oh so revive magnitude to have this information then

Copy link
Member

Choose a reason for hiding this comment

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

magnitude was never axed. We have magnitude which allows you to specify a CPID, and mymagnitude which is exactly the same but works on msPrimaryCPID (?). We can merge this to make magniude work on any specified CPID, or fall back to msPrimaryCPID if none is specified.

denravonska
denravonska reviewed Aug 4, 2018
View reviewed changes
src/rpcblockchain.cpp

res.pushKV("RSA Weight", RSAWEIGHT);
res.pushKV("Magnitude", out_magnitude);
res.pushKV("RSA Owed", out_owed);
Copy link
Member

Choose a reason for hiding this comment

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

Merge the owed field into MagnitudeReport.

Copy link
Member Author

Choose a reason for hiding this comment

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

its untrustedmagnitude here. does the fulfillment in MagnitudeReport not more accurate?
// Fulfillment %
double fulfilled = ((structMag.payments/14) / ((dExpected14/14)+.01)) * 100;
entry.pushKV("Fulfillment %", fulfilled);

Copy link
Member Author

Choose a reason for hiding this comment

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

could figure out difference and name is estimated rsa owed?

Copy link
Member

Choose a reason for hiding this comment

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

Since it's just for display we can use "owed": structMag.owed. I think.

@denravonska denravonska mentioned this pull request Aug 12, 2018
Merged
@tomasbrod
Copy link
Member

Can/Would you please change PR name to something more descriptive?

@iFoggz iFoggz changed the title Issue 1228 Remove redundant RPC calls and modify some to include data from others being removed Aug 21, 2018
@denravonska denravonska changed the title Remove redundant RPC calls and modify some to include data from others being removed Remove/merge redundant RPC calls Aug 25, 2018
@denravonska denravonska merged commit 5d0e29e into gridcoin-community:development Aug 25, 2018
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@denravonska denravonska denravonska left review comments

Assignees
No one assigned
Labels
enhancement
Projects
None yet
Milestone
Camilla
Development

Successfully merging this pull request may close these issues.
3 participants
@iFoggz @denravonska @tomasbrod