`accounts_representatives` is failing when one of the account is not opened

[tombertrand]

Summary

Original PR: #3409

When using the accounts_representatives RPC if one of the account in the list is not opened, it returns an error message instead of setting the representative response value to "" or null or undefined for that account.

curl -d '{
  "action": "accounts_representatives",
  "accounts": ["nano_1openedaccount", "nano_3unopenedaccount"]
}' http://127.0.0.1:7076

Node version

V23

Build details

Production build

OS and version

unrelated

Steps to reproduce the behavior

Query the accounts_representative with opened and unopened accounts.

Expected behavior

{
  "representatives": {
    "nano_1openedaccount": "nano_1repaccount",
    "nano_3unopenedaccount": "" or undefined or null
  }
}

Actual behavior

{
    "error": "Account not found"
}

Possible solution

include the unopened account in the response but mark the rep as an empty string "" (preferred solution since it would be easier to read the response with missing reps than trying to diff the accounts sent in parameters vs the response) or exclude the unopened account from the response

Supporting files

No response

[dsiganos]

This is the code that handles the RPC:

void nano::json_handler::accounts_representatives ()
{
        boost::property_tree::ptree representatives;
        for (auto & accounts : request.get_child ("accounts"))
        {
                auto account (account_impl (accounts.second.data ()));
                auto transaction (node.store.tx_begin_read ());
                auto info (account_info_impl (transaction, account));

                if (!ec)
                {
                        boost::property_tree::ptree entry;
                        entry.put ("", info.representative.to_account ());
                        representatives.push_back (std::make_pair (accounts.second.data (), entry));
                }
        }
        response_l.add_child ("representatives", representatives);
        response_errors ();
}

The only way I can explain what is happening is, the variable ec must be getting set as a side effect by something (probably by account_impl) and then somehow that causes the entire response to be ignore and a general error to be returned.

[dsiganos]

Yeap, just as I thought. If ec is set then response_l is ignored and response_error is returned instead.

void nano::json_handler::response_errors ()
{
    if (!ec && response_l.empty ())
    {
        // Return an error code if no response data was given
        ec = nano::error_rpc::empty_response;
    }
    if (ec)
    {
        boost::property_tree::ptree response_error;
        response_error.put ("error", ec.message ());
        std::stringstream ostream;
        boost::property_tree::write_json (ostream, response_error);
        response (ostream.str ());
    }
    else
    {
        std::stringstream ostream;
        boost::property_tree::write_json (ostream, response_l);
        response (ostream.str ());
    }
}

[dsiganos]

Looking at the code for RPC accounts_balances, I would expect that it has the same problem:

void nano::json_handler::accounts_balances ()
{
    boost::property_tree::ptree balances;
    for (auto & accounts : request.get_child ("accounts"))
    {
        auto account (account_impl (accounts.second.data ()));
        if (!ec)
        {
            boost::property_tree::ptree entry;
            auto balance (node.balance_pending (account, false));
            entry.put ("balance", balance.first.convert_to<std::string> ());
            entry.put ("pending", balance.second.convert_to<std::string> ());
            entry.put ("receivable", balance.second.convert_to<std::string> ());
            balances.push_back (std::make_pair (account.to_account (), entry));
        }
    }
    response_l.add_child ("balances", balances);
    response_errors ();
}

[dsiganos]

accounts_representatives and accounts_frontiers likely have the same problem too.

[tombertrand]

They do produce different results atm, I could argue that leaving the accounts_frontiers's result for an account to an empty string "" if it doesn't have a frontier instead of omitting it from the response would be a better approach (same as accounts_balances responding with "0"s for an empty account), unless there is a good reason for it that I'm not aware? Else the same approach can be taken to be consistent in responses and I'll diff the request array with the result object to know who doesn't have a representative.

[zhyatt]

@clemahieu Prefers to leave the accounts_balances as is with 0 in the response. But updates for accounts_representatives should be done to return an empty string for unopened accounts and on accounts_frontiers the account should be included in the set with an empty string as well.

@thsfs Can you help get a PR up for the changes noted above?

[zhyatt]

@clemahieu After discussing with @thsfs there are two cases that came up and the first seems like the right way to do it, but we'd like your thoughts on the second one:

1) If all the accounts provided are unopened, should all of the accounts be returned with empty strings or should an error be returned? All accounts should be returned with empty strings

2) How should invalid accounts be handled (failed to decode an account)? Two options come to mind:

a) Add an error to the value for the specific account key with the failure:

{
    "representatives": {
        "nano_3wfddg7a1paogrcwi3yhwnaerboukbr7rs3z3ino5toyq3yyhimo6f6egij6": "nano_3wfddg7a1paogrcwi3yhwnaerboukbr7rs3z3ino5toyq3yyhimo6f6egij6",
        "nano_396sch48s3jmzq1bk31pxxpz64rn7joj38emj4ueypkb9p9mzrym34obze6c": "", // unopened account
        "nano_396sch48s3jmzq1bk31pxxpz64rn7joj38emj4ueypkb9p9mzrym34obze6i": "Bad account number"
    }
}

or b) return an error for the whole call with the first account that was found invalid (this forces account validation prior to calling, which seems reasonable):

{
  "error": "Bad account number: [account]"
}

(note that error text is found in common errors here)

[dsiganos]

1. I do not think we should treat the special case of all accounts being unopened differently because we are unnecessarily increasing complexity of coding and documentation. We would have to explain and code that if all accounts are unopened, we return errors one way, but if some are unopened, we return errors in another way. Let's just return errors one way only.

2. On another PR, we establish an error returning mechanism which seems reasonable, let's use the same approach. "nano_3e3j5tkog48pnny9dmfzj1r16pg8t1e76dz5tmac6iq689wyjfpiij4txtd1": "error: Bad account number", https://github.com/nanocurrency/nano-node/pull/3791

[zhyatt]

@dsiganos For the cases where the account is unopened, do you suggest we return that new error format with Account not found, so "nano_3e3j5tkog48pnny9dmfzj1r16pg8t1e76dz5tmac6iq689wyjfpiij4txtd1": "error: Account not found" for consistency?

[dsiganos]

Yes, that makes sense to me. It is simple, consistent and there is no loss of information.