XRPLF / clio

An XRP Ledger API Server
https://xrpl.org
ISC License
61 stars 52 forks source link

[amm_info] error case responses of clio is missing ledger_index and ledger_hash #1155

Open mounikakun opened 7 months ago

mounikakun commented 7 months ago

Request:

{
  "method": "amm_info",
  "params": [
    {
      "asset": "anystring",
      "asset2": {
        "currency": "USD",
        "issuer": "rE5irAofYv7MePpJFVoMUsTS7P3kGjf2rU",
        "value": "10"
      },
      "ledger_index": "validated",
      "Fee": "20"
    }
  ]
}

Rippled response:

{
  "result": {
    "error": "issueMalformed",
    "error_code": 93,
    "error_message": "Issue is malformed.",
    "ledger_hash": "2469951DE611361E7D69D2A05DF5A73A6B48873AB5524EF288C3C42B93E9C01A",
    "ledger_index": 3898307,
    "request": {
      "Fee": "20",
      "asset": "anystring",
      "asset2": {
        "currency": "USD",
        "issuer": "rE5irAofYv7MePpJFVoMUsTS7P3kGjf2rU",
        "value": "10"
      },
      "command": "amm_info",
      "ledger_index": "validated"
    },
    "status": "error",
    "validated": True
  }
}

Clio response:

{
  "result": {
    "error": "issueMalformed",
    "error_code": 93,
    "error_message": "Issue is malformed.",
    "status": "error",
    "type": "response",
    "request": {
      "method": "amm_info",
      "params": [
        {
          "asset": "anystring",
          "asset2": {
            "currency": "USD",
            "issuer": "rE5irAofYv7MePpJFVoMUsTS7P3kGjf2rU",
            "value": "10"
          },
          "ledger_index": "validated",
          "Fee": "20"
        }
      ]
    }
  },
  "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"
    }
  ]
}
godexsoft commented 7 months ago

This is a rippled inconsistency. Clio does not include ledger_hash and ledger_index in error responses. Neither does rippled for most APIs. This appears to be an oversight on rippled's amm_info implementation.

I suggest we update the docs instead specifying that ledger_hash, ledger_index and ledger_index_current may be omitted in error responses. We can leave rippled's implementation as-is since those fields could be considered "extra" fields in the output which is not an API breakage.

godexsoft commented 7 months ago

@Bronek, please take a look. This may be something that could be improved on rippled side.

godexsoft commented 7 months ago

@mDuo13 can you take a look and advise how you would like to document this if at all. Currently we don't see specific error message documentation for this (or other) method on the xrpl.org website.

edit: I now noticed the "The response follows the standard format, with a successful result containing the following fields:" that seems to be on each API doc. Does this suggest that only successful responses should have these fields?

Bronek commented 7 months ago

Note, showing extra fields in responses is fair play; especially if these fields are not documented (so they do not officially fall into API interface). I do not think you need to reproduce rippled behaviour here, this seems to be more of a documentation issue.