[amm_info] with malformed account error differs with rippled

[mounikakun]

Clio returns actMalformed whereas rippled returns invalidParams error

Request:

{
  "method": "amm_info",
  "params": [
    {
      "account": "rP32Fk3eRYDNSz8B31CxC5Jphc8Tz6Gt8"
    }
  ]
}

Rippled response:

{
    "result": {
        "error": "invalidParams",
        "error_code": 31,
        "error_message": "Invalid parameters.",
        "ledger_current_index": 3899929,
        "request": {
            "account": "rP32Fk3eRYDNSz8B31CxC5Jphc8Tz6Gt8",
            "command": "amm_info"
        },
        "status": "error",
        "validated": false
    }
}

Clio response:

{
    "result": {
        "error": "actMalformed",
        "error_code": 35,
        "error_message": "Account malformed.",
        "status": "error",
        "type": "response",
        "request": {
            "method": "amm_info",
            "params": [
                {
                    "account": "rP32Fk3eRYDNSz8B31CxC5Jphc8Tz6Gt8"
                }
            ]
        }
    },
    "warnings": [
        {
            "id": 2001,
            "message": "This is a clio server. clio only serves validated data. If you want to talk to rippled, include 'ledger_index':'current' in your request"
        },
        {
            "id": 2002,
            "message": "This server may be out of date"
        }
    ]
}

[godexsoft]

This is caused by a difference in how rippled validates input compared to Clio.

Unfortunately, there is no proper fix for us to apply here to get parity. One way to "fix" would be to move validation logic for account into the process function of the handler and don't validate it in the validation step that is described by a spec. Of course that would be a step backwards so we don't want to do that.

While we could easily fix the exact case reported in this issue, we can't solve the general case. For example this request would still produce different error code/message:

{
  "method": "amm_info",
  "params": [
    {
      "account": "rP32Fk3eRYDNSz8B31CxC5Jphc8Tz6Gt8",
      "amm_account" : "rLL58S26SjzyzmUoc65Cd1MvkpQk74ae4H"
    }
  ]
}

Another way to achieve parity would be to fix this on rippled side among other things that seem to be incorrect for amm_info such as ledger_hash, ledger_index and/or ledger_current_index appearing in error responses - something that does not happen for other APIs even in rippled.

@Bronek what do you think about this?

[godexsoft]

Currently rippled returns inconsistent error code for the same invalid account for the input fields account and amm_account:

$ curl -d '{
  "method": "amm_info",
  "params": [
    {
      "account": "rP32Fk3eRYDNSz8B31CxC5Jphc8Tz6Gt8"
    }
  ]
}'

{
  "result": {
    "error": "invalidParams",
    "error_code": 31,
    "error_message": "Invalid parameters.",
    "ledger_current_index": 4000379,
    "request": {
      "account": "rP32Fk3eRYDNSz8B31CxC5Jphc8Tz6Gt8",
      "command": "amm_info"
    },
    "status": "error",
    "validated": false
  }
}

$ curl -d '{
  "method": "amm_info",
  "params": [
    {
      "amm_account": "rP32Fk3eRYDNSz8B31CxC5Jphc8Tz6Gt8"
    }
  ]
}'

{
  "result": {
    "error": "actMalformed",
    "error_code": 35,
    "error_message": "Account malformed.",
    "ledger_current_index": 4000381,
    "request": {
      "amm_account": "rP32Fk3eRYDNSz8B31CxC5Jphc8Tz6Gt8",
      "command": "amm_info"
    },
    "status": "error",
    "validated": false
  }
}

[Bronek]

@gregtatcam do you think that's something we ought to change in rippled ?

[gregtatcam]

@gregtatcam do you think that's something we ought to change in rippled ?

It makes sense to have a consistent error.

[Bronek]

I think this is because in rippled we first execute this check:

        if ((params.isMember(jss::asset) != params.isMember(jss::asset2)) ||
            (params.isMember(jss::asset) == params.isMember(jss::amm_account)))
            return Unexpected(rpcINVALID_PARAMS);

In this case indeed both asset and amm_account are missing, so we return rpcINVALID_PARAMS before checking the account.

I wonder if we could push this check down.

[Bronek]

I will prepare a PR to change the order of checks in rippled for API version 3. As for API version 2 I think we just need to document the discrepancy and live with it

[godexsoft]

Fixed on rippled side.