Move signer_list in JSON response for api_version 2

[Frassle]

High Level Overview of Change

signer_lists is returned in the root of the account_info response, no longer nested under account_data.

Context of Change

See XRPLF/xrpl-dev-portal#938 for context.

Type of Change

• New feature (non-breaking change which adds functionality)

[nbougalis]

Thank you for your contribution!

[HowardHinnant]

Is it intended that testSignerListsApiVersion2() not be run?

[Frassle]

Thanks for pointing that out, did think it a bit odd "rippled -u" passed so easily. I've fixed up the test so it will pass if ApiMaximumSupportedVersion is set to 2 and I've added the call to the test commented out with an explanation. Will wait to hear more from @scottschurr and @pwang200 on how to actually write tests for the v2 api.

[scottschurr]

@Frassle, thanks for the attempt. Good effort. The attempt at a unit test in your pull request raises a significant question. If we're eventually going to support V2 of the API, how do we test additions to the V2 API before that support is enabled for public consumption?

As it stands, testing of RPC API V2 is blocked by this code: https://github.com/ripple/rippled/blob/develop/src/ripple/rpc/impl/RPCHelpers.cpp#L898-L902

This is the same code that blocks RPC calls using API V2 before it is enabled. In the general case, we probably want that check left in place.

So, apparently, we need some way for Env to disable that check? Or some other hack?

@pwang200, do you have any thoughts on this topic? Thanks.

[pwang200]

@scottschurr How about something like:

constexpr unsigned int ApiExperimentalVersion = 2;

// no longer constexpr
// init to 1, and unit tests can promote it to ApiExperimentalVersion
extern unsigned int ApiMaximumSupportedVersion; 

To be more precise, I create this PR https://github.com/pwang200/rippled/pull/2/files (against my own develop) to show what I meant with a V2 version RPC call. If interested, please take a look at the RPCHelpers.h file, although I am not sure if the simple class ApiExperiment can work with parallel RPC tests.

As a simpler alternative, we may have

void
allowExperimentalApiVersion()
{
    RPC::ApiMaximumSupportedVersion = RPC::ApiExperimentalVersion;
}

called when Env is constructed, if we don't have to worry about demote RPC::ApiMaximumSupportedVersion to its original value.

[scottschurr]

@pwang200, oh yuck. You're right to worry about behavior of the parallel unit tests while changing the maximum supported RPC version. That means the maximum supported RPC version value needs to be a member variable that aggregates (through some layers) to the Application. Some other parts of the system solve this problem by making the hack setting part of the Config. But there are other options...

I know you're neck deep in other stuff right now. Shall I make a run at solving this problem and consult with you? Or do you want to figure it out?

[scottschurr]

@pwang200, the problem is worse than I initially realized. Currently the RPC handlers are separated by version here: https://github.com/ripple/rippled/blob/develop/src/ripple/rpc/impl/Handler.cpp#L222-L223

Do we even have a mechanism for an RPC handler to register itself as both a version 1 and version 2 handler?

Do you know if we have a migration plan for how Version 2 RPCs are introduced to the code base and tested (before being enabled)? @carlhua, you may have an opinion here as well...

[pwang200]

@scottschurr Thanks for offering to help! I will need it for sure, especially if we need a solution quickly. One of the things that are not clear to me is how parallel unit tests work. I am under the impression that parallel tests run in separate processes and the tests in the same process run sequentially.

Regarding registering handlers, please see the change I made in Handler.cpp, only a dummy example to show how registering could work. I added a V3 version RPC handler, and printed out the RPC results (in a unit test):

api_version=0 null

api_version=1 
{
        "status" : "success",
        "version" : 
        {
                "first" : "1.0.0",
                "good" : "1.0.0",
                "last" : "1.0.0"
        }
}

api_version=2 
{
        "status" : "success",
        "version" : 
        {
                "api_version_lower_limit" : 1,
                "api_version_upper_limit" : 2
        }
}

api_version=3 null

Promoted ApiMaximumSupportedVersion
api_version=3 
{
        "status" : "success",
        "version" : 
        {
                "api_version" : 3
        }
}

api_version=4 null

[scottschurr]

@pwang200, I don't know that we necessarily need a solution quickly. But this pull request will be held off until we have a good answer. I suspect @Frassle can handle that, as long as it's for a good reason.

Regarding the parallel unit tests, you know more than I do. Since you pointed it out, yes, it looks like the children all work in their own processes. So my concern about static variables affecting tests running in parallel is ill founded. Each process should have its own copy of the static variables.

I just did a little looking around regarding the parallel unit tests. It looks like the heart of it happens here: https://github.com/ripple/rippled/blob/develop/src/ripple/app/main/Main.cpp#L281-L323 But you've probably already found that.

I haven't looked closely at your suggested approach yet. I'll do that now.

[pwang200]

@scottschurr thanks for pointing the code about parallel unit tests out to me. I just read the code. Yes, children run in their own processes. The way that children coordinate ports is also interesting. https://github.com/ripple/rippled/blob/c0a0b79d2d483b318ce1d82e526bd53df83a4a2c/src/test/unit_test/multi_runner.cpp#L518
I agree that static variables will not affect tests running in parallel. I was not sure about that children have their own processes. Sorry for the noise.

It also seems that a child runs test suites one by one. If so, then adjusting ApiMaximumSupportedVersion could be simple.

[scottschurr]

@pwang200, how would you feel about putting together a pull request for this functionality? Then we can talk about the changes over there, rather than cluttering up this pull request.

It looks to me like your basic idea should work, but I think there are some loose ends that will need to be tied up. For instance I just figured out that this unit test is not working as expected:

https://github.com/ripple/rippled/blob/develop/src/test/rpc/Version_test.cpp#L74

std::string::find returns std::string::npos not zero when the find fails. The string being returned is "couldn't parse reply from server", not "invalid_API_version".

[Frassle]

I suspect @Frassle can handle that, as long as it's for a good reason.

I'm in no rush. Just nudge on here when a framework is in place to test v2 with and I'll fix this PR up.

[pwang200]

@scottschurr right, I will put together a pull request.
@Frassle Thanks!

[mDuo13]

I'm supportive of making this change, though it sounds like there are some things to figure out on the versioning stuff, which will also be useful for future efforts.

[scottschurr]

@Frassle, thanks for your patience on this. I think we have a good solution to the testing problem here: #3781. Since that pull request has passed code review I expect it will be part of the next beta. If you are able to rebase to that upcoming beta once it becomes available I'm hoping you will be able to finish up this work.

My apologies for the delay. I really appreciate how your change pointed out a lack in the code base.

[Frassle]

If you are able to rebase to that upcoming beta once it becomes available I'm hoping you will be able to finish up this work.

Will do. No worries about the delay.

[scottschurr]

@Frassle, sorry this has taken so long. The most recent code in develop now contains the testing support that was needed for this pull request. If you're still interested you can now proceed with this pull request. If you have any questions please ping and we'll do our best to answer.

[Frassle]

No worries. I'll pick this up again this week sometime.

[scottschurr]

👍 Looks good to me. 100% unit test coverage. Nice!

[intelliot]

Typo in the PR description

signer_lists is returned in the root of the account_lines response

Should be account_info, not account_lines

[cjcobb23]

Hi @Frassle , can you please rebase this PR onto develop, and remove the merge commit in the process? Thanks.

[Frassle]

Think this should all be ready to go now. Let me know if anything else needed.

[scottschurr]

@HowardHinnant and @cjcobb23, how is this pull request looking to you folks? Thanks.

[cjcobb23]

Looks great. Thank you for your contribution.