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

accounts_representatives is failing when one of the account is not opened #3752

Open
tombertrand opened this issue Mar 18, 2022 · 10 comments
Open
Assignees
Labels
rpc Changes related to Remote Procedure Calls
Milestone

Comments

@tombertrand
Copy link

tombertrand commented Mar 18, 2022

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
Copy link
Contributor

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
Copy link
Contributor

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 ());
	}
}

@zhyatt zhyatt added the rpc Changes related to Remote Procedure Calls label Mar 18, 2022
@zhyatt zhyatt added this to the V24.0 milestone Mar 18, 2022
@dsiganos dsiganos self-assigned this Mar 18, 2022
@dsiganos
Copy link
Contributor

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
Copy link
Contributor

accounts_representatives and accounts_frontiers likely have the same problem too.

@tombertrand
Copy link
Author

tombertrand commented Mar 19, 2022

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.

image

@zhyatt
Copy link
Collaborator

zhyatt commented Apr 12, 2022

@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?

@thsfs thsfs self-assigned this Apr 12, 2022
@zhyatt
Copy link
Collaborator

zhyatt commented Apr 12, 2022

@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
Copy link
Contributor

dsiganos commented Apr 13, 2022

  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",
    Changes the accounts_frontiers RPC to return per account results #3791

@zhyatt
Copy link
Collaborator

zhyatt commented Apr 13, 2022

@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
Copy link
Contributor

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

@qwahzi qwahzi modified the milestones: V24.0, Prioritised Dec 29, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
rpc Changes related to Remote Procedure Calls
Projects
None yet
Development

No branches or pull requests

5 participants