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

[amm_info] with malformed account error differs with rippled #1157

Open
mounikakun opened this issue Feb 6, 2024 · 6 comments · May be fixed by XRPLF/rippled#4924
Open

[amm_info] with malformed account error differs with rippled #1157

mounikakun opened this issue Feb 6, 2024 · 6 comments · May be fixed by XRPLF/rippled#4924

Comments

@mounikakun
Copy link
Collaborator

mounikakun commented Feb 6, 2024

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

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

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

Bronek commented Feb 7, 2024

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

@gregtatcam
Copy link

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

It makes sense to have a consistent error.

@Bronek
Copy link
Collaborator

Bronek commented Feb 7, 2024

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

Bronek commented Feb 20, 2024

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

@Bronek Bronek linked a pull request Feb 21, 2024 that will close this issue
13 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
Status: 📋 Backlog
Development

Successfully merging a pull request may close this issue.

4 participants