Skip to content
New issue

Have a question about this project? # for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “#”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? # to your account

QoL improvements for accounts_frontiers & accounts_representatives RPC #4223

Merged
merged 3 commits into from
May 15, 2023

Conversation

pwojcikdev
Copy link
Contributor

Issue nanocurrency/nano-docs#665 complained that "[...], if your code expects a valid hash like it always used to return, the RPC now provides an unexpected string for newly created accounts:", which was caused by
#3791 that changed RPC API to return errors in a pretty unexpected, non standard way. This PR changes accounts_frontiers & accounts_representatives RPCs to instead return any errors in an errors subfield, which avoids encoding errors in a field that should always contain valid hash or account number (according to documentation). It should also simplify checking for errors client side, as checking if the account was queried successfully can be accomplished by simple if account in response condition, instead of needing to parse error strings.

accounts_frontiers

Request:

{  
  "action": "accounts_frontiers",  
  "accounts": ["nano_3t6k35gi95xu6tergt6p69ck76ogmitsa8mnijtpxm9fkcm736xtoncuohr3", "nano_36uccgpjzhjsdbj44wm1y5hyz8gefx3wjpp1jircxt84nopxkxti5bzq1rnz", "nano_1hrts7hcoozxccnffoq9hqhngnn9jz783usapejm57ejtqcyz9dpso1bibuy"]  
}

Response:

{
    "frontiers": {
        "nano_3t6k35gi95xu6tergt6p69ck76ogmitsa8mnijtpxm9fkcm736xtoncuohr3": "023B94B7D27B311666C8636954FE17F1FD2EAA97A8BAC27DE5084FBBD5C6B02C"
    },
    "errors": {
        "nano_36uccgpjzhjsdbj44wm1y5hyz8gefx3wjpp1jircxt84nopxkxti5bzq1rnz": "Bad account number",
        "nano_1hrts7hcoozxccnffoq9hqhngnn9jz783usapejm57ejtqcyz9dpso1bibuy": "Account not found"
    }
}

accounts_representatives

Request:

{  
  "action": "accounts_representatives",  
  "accounts": ["nano_3t6k35gi95xu6tergt6p69ck76ogmitsa8mnijtpxm9fkcm736xtoncuohr3", "nano_36uccgpjzhjsdbj44wm1y5hyz8gefx3wjpp1jircxt84nopxkxti5bzq1rnz", "nano_1hrts7hcoozxccnffoq9hqhngnn9jz783usapejm57ejtqcyz9dpso1bibuy"]  
}

Response:

{
    "representatives": {
        "nano_3t6k35gi95xu6tergt6p69ck76ogmitsa8mnijtpxm9fkcm736xtoncuohr3": "nano_3t6k35gi95xu6tergt6p69ck76ogmitsa8mnijtpxm9fkcm736xtoncuohr3"
    },
    "errors": {
        "nano_36uccgpjzhjsdbj44wm1y5hyz8gefx3wjpp1jircxt84nopxkxti5bzq1rnz": "Bad account number",
        "nano_1hrts7hcoozxccnffoq9hqhngnn9jz783usapejm57ejtqcyz9dpso1bibuy": "Account not found"
    }
}

@pwojcikdev pwojcikdev added the documentation This item indicates the need for or supplies updated or expanded documentation label May 3, 2023
@pwojcikdev pwojcikdev requested review from clemahieu and thsfs May 4, 2023 11:00
@keerifox
Copy link

keerifox commented May 4, 2023

would this return an empty object "frontiers": {} / "representatives": {} if each passed account results in an error?

@thsfs thsfs added this to the V25.0 milestone May 5, 2023
@thsfs
Copy link
Contributor

thsfs commented May 9, 2023

would this return an empty object "frontiers": {} / "representatives": {} if each passed account results in an error?

checked this, is is returning an empty string when the frontiers or representatives are empty.

@thsfs thsfs added the rpc Changes related to Remote Procedure Calls label May 9, 2023
@keerifox
Copy link

keerifox commented May 10, 2023

checked this, is is returning an empty string when the frontiers or representatives are empty.

can't check accounts_representatives as most public nodes prohibit that action, but accounts_frontiers seems to return an empty string on V23.3 nodes (pre V24 breaking changes) for valid account addresses that don't have any known to node transactions yet (this is the most common scenario)

{  
    "action": "accounts_frontiers",  
    "accounts": ["nano_1hrts7hcoozxccnffoq9hqhngnn9jz783usapejm57ejtqcyz9dpso1bibuy"]  
}
{
    "frontiers": ""
}

with this in mind, I have no idea which option would be the least breaking for existing implementations. this cursed javascript code works because it iterates characters of the string rather than keys of the set:

const response = {
   frontiers: '',
}

for(const accountID in response.frontiers) {
  console.log(accountID)
}

the one below doesn't seem to produce any errors either, but that might be different in other programming languages, so it depends if the implementations check for the presence of field frontiers and/or its type

const response = {}

for(const accountID in response.frontiers) {
  console.log(accountID)
}

accounts_balances doesn't have an issue of similar importance, as the most common scenario (valid account but no seen transactions) provides balance values of 0. that RPC only provides questionable output if invalid addresses are passed. by questionable I mean the V23.3 response is

{
     "error": "Bad account number"
}

and #4228 proposes changing it to

{
    "errors": {
        "nano_36uccgpjzhjsdbj44wm1y5hyz8gefx3wjpp1jircxt84nopxkxti5bzq1rnz": "Bad account number"
    }
}

which is different, but likely fine for an improper configuration error that shouldn't occur on production code (?)

@@ -989,10 +996,15 @@ void nano::json_handler::accounts_frontiers ()
ec = nano::error_common::account_not_found;
}
}
frontiers.put (account_from_request.second.data (), boost::str (boost::format ("error: %1%") % ec.message ()));
debug_assert (ec);
errors.put (account_from_request.second.data (), ec.message ());
ec = {};
}
response_l.add_child ("frontiers", frontiers);
Copy link
Contributor

Choose a reason for hiding this comment

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

Only add frontiers if it is not empty.

@@ -937,10 +938,15 @@ void nano::json_handler::accounts_representatives ()
continue;
}
}
representatives.put (account_from_request.second.data (), boost::format ("error: %1%") % ec.message ());
debug_assert (ec);
errors.put (account_from_request.second.data (), ec.message ());
ec = {};
}
response_l.add_child ("representatives", representatives);
Copy link
Contributor

Choose a reason for hiding this comment

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

Only add representatives if it is not empty.

ec = {};
}
response_l.add_child ("representatives", representatives);
if (!representatives.empty ())
Copy link
Contributor

Choose a reason for hiding this comment

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

Considering the ptree issue with serialising empty arrays as empty strings, I think this is the less-wrong way to do it. RPC users can test if the array member exists before iterating. In the future when the serialisation is fully conformant, the check could be removed. Otherwise they need to handle different types which is undesirable.

@clemahieu clemahieu merged commit 444f0f2 into nanocurrency:develop May 15, 2023
# for free to join this conversation on GitHub. Already have an account? # to comment
Labels
documentation This item indicates the need for or supplies updated or expanded documentation rpc Changes related to Remote Procedure Calls
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants