edgecase: nothing inside in `Memo` Field

[jungsooyun]

According to the data validation logic for the Memo Class at, https://github.com/XRPLF/xrpl-py/blob/6900bcfe041bba7adae35f231c622b796bfb2b58/xrpl/models/transactions/transaction.py#L104-L117

An error should occur if any of the three fields in the Memo are missing. However, the transaction E64A81D9FDE13F2FE28A98F337809D894E3279C75ED8E784114C650A04DFC65C has the following value with tesSUCCESS:

    "Memos": [
      {
        "TransactionMetaData": {}
      }
    ],

This causes an issue when loading the data using the Memo class. I'd like to inquire if this is an acceptable case when only using this class for transaction submissions.

---

full tx

{
  "result": {
    "Account": "rEX2Q18vQziVAJoiLwoVHN5u87CXm29LCQ",
    "Fee": "10",
    "Flags": 2147483648,
    "Memos": [
      {
        "TransactionMetaData": {}
      }
    ],
    "Sequence": 45,
    "SigningPubKey": "033D098A1F0664D894D0D542006541CDDC997E1B5A5F1A6E63BE5B83E12AAF00D2",
    "TransactionType": "AccountSet",
    "TxnSignature": "30440220267138C6A207F981BE736424242F30AC1E23434778B222DD1CEB6AA4D975DC16022039560C18E5E12615D8542BF270CDF04501E80CD6D9F27595ABE8F601E91DE7D9",
    "hash": "E64A81D9FDE13F2FE28A98F337809D894E3279C75ED8E784114C650A04DFC65C",
    "meta": {
      "AffectedNodes": [
        {
          "ModifiedNode": {
            "FinalFields": {
              "Account": "rEX2Q18vQziVAJoiLwoVHN5u87CXm29LCQ",
              "Balance": "32005485",
              "Flags": 0,
              "OwnerCount": 2,
              "Sequence": 46
            },
            "LedgerEntryType": "AccountRoot",
            "LedgerIndex": "BCCEA6B346A29656AFD37B8AB7F573DA705AB7A86D9FE197AC6CB1E9D5772EDB",
            "PreviousFields": {
              "Balance": "32005495",
              "Sequence": 45
            },
            "PreviousTxnID": "FECA13A8D357E141DA6D9118C82BDC33188ACE62164FB0609FB5905AB0F03E6F",
            "PreviousTxnLgrSeq": 7266352
          }
        }
      ],
      "TransactionIndex": 3,
      "TransactionResult": "tesSUCCESS"
    },
    "validated": true,
    "date": 456420830,
    "ledger_index": 7266368,
    "inLedger": 7266368
  },
  "id": 1,
  "status": "success",
  "type": "response",
  "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"
    }
  ]
}

[mvadari]

This transaction is from 2014. The typechecking for memos was likely not very robust back then.

[intelliot]

This causes an issue when loading the data using the Memo class.

What issue, specifically, does this cause?

[jungsooyun]

While parsing all transactions from the rippled using xrpl-py, an error occurred for the above transaction_hash data because it lacks any of the three fields. If xrpl-py is not intended to provide a model for robust data parsing or is not concerned with full backward compatibility, then it's not a critical issue.

I just wanted to clarify..

[ckeshava]

@mvadari you are right. rippled introduces a rigorous check only on 21 January, 2015: https://github.com/XRPLF/rippled/blob/69143d71f8973e33b701d7becc19da2ad6a68b68/src/ripple/protocol/impl/STTx.cpp#L479

This might be a hindrance if somebody wants to work with historical data, using xrpl-py. In the spirit of backwards compatibility, I'd prefer to remove this check.

[ckeshava]

Even if xrpl-py removes this check, rippled still performs the same check and throws the appropriate errors. We wouldn't be compromising the functionality anyway.

[mvadari]

The Memo model will break regardless, because the TransactionMetadata field doesn't exist on the class. Removing that check wouldn't fix anything.

I would rather have the library be easier to use for folks processing modern-day data/creating modern-day transactions than support a few minor edge cases from 10 years ago.

If you're just looking to deserialize data or something, you can do that by using the binary codec directly and skipping the models. The models aren't strictly necessary.

[ckeshava]

hmm, I see. I wasn't aware that you could use the binary-codec directly. I misunderstood that there's no way to decode the historical data.

[mvadari]

Closing as you can still decode old transactions via using the binary codec directly, and it doesn't make sense to add support to the models for this.

[jungsooyun]

Will use binary codec. Thanks a lot!