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

JSON: Memo: An empty "memo" is not being generated for a transaction even if "memo_type" is "text" #149

Closed
michaeljmonte opened this issue Nov 8, 2017 · 9 comments · Fixed by #635

Comments

@michaeljmonte
Copy link

I'm going to start with an example JSON returned from Horizon using the following transaction:

1113f23c225495534b2fd589a037798155ea73ee68a418e74364c1a3be4a20d8

JSON:

{
  "_links": {
    "self": {
      "href": "https://horizon.stellar.org/transactions/1113f23c225495534b2fd589a037798155ea73ee68a418e74364c1a3be4a20d8"
    },
    "account": {
      "href": "https://horizon.stellar.org/accounts/GALPCCZN4YXA3YMJHKL6CVIECKPLJJCTVMSNYWBTKJW4K5HQLYLDMZTB"
    },
    "ledger": {
      "href": "https://horizon.stellar.org/ledgers/7871"
    },
    "operations": {
      "href": "https://horizon.stellar.org/transactions/1113f23c225495534b2fd589a037798155ea73ee68a418e74364c1a3be4a20d8/operations{?cursor,limit,order}",
      "templated": true
    },
    "effects": {
      "href": "https://horizon.stellar.org/transactions/1113f23c225495534b2fd589a037798155ea73ee68a418e74364c1a3be4a20d8/effects{?cursor,limit,order}",
      "templated": true
    },
    "precedes": {
      "href": "https://horizon.stellar.org/transactions?order=asc\u0026cursor=33805687590912"
    },
    "succeeds": {
      "href": "https://horizon.stellar.org/transactions?order=desc\u0026cursor=33805687590912"
    }
  },
  "id": "1113f23c225495534b2fd589a037798155ea73ee68a418e74364c1a3be4a20d8",
  "paging_token": "33805687590912",
  "hash": "1113f23c225495534b2fd589a037798155ea73ee68a418e74364c1a3be4a20d8",
  "ledger": 7871,
  "created_at": "2015-10-01T04:17:31Z",
  "source_account": "GALPCCZN4YXA3YMJHKL6CVIECKPLJJCTVMSNYWBTKJW4K5HQLYLDMZTB",
  "source_account_sequence": "12884901893",
  "fee_paid": 100,
  "operation_count": 1,
  "envelope_xdr": "AAAAABbxCy3mLg3hiTqX4VUEEp60pFOrJNxYM1JtxXTwXhY2AAAAZAAAAAMAAAAFAAAAAAAAAAEAAAAAAAAAAQAAAAAAAAAJAAAAAAAAAAHwXhY2AAAAQGCy99saUm6alXIRyp0NNh2OFSCybp1JGPDN2pb/+Fw07/X7y4lPEp/B6WIV130a+2eY+5T3ujbiKa6TIcUaNwQ=",
  "result_xdr": "AAAAAAAAAGQAAAAAAAAAAQAAAAAAAAAJAAAAAAAAAAEAAAAAH6Ue1GOPj6Hb/ROPyIFCJpQPMujihEIvJSfK0UfMDIgAAK11sXTKRwAAAAA=",
  "result_meta_xdr": "AAAAAAAAAAEAAAACAAAAAwAAHrcAAAAAAAAAAB+lHtRjj4+h2/0Tj8iBQiaUDzLo4oRCLyUnytFHzAyIAACtdb1ePEoAAB6hAAAAAAAAAAAAAAAAAAAAAAAAAAABAAAAAAAAAAAAAAAAAAAAAAAAAQAAHr8AAAAAAAAAAB+lHtRjj4+h2/0Tj8iBQiaUDzLo4oRCLyUnytFHzAyIAAFa627TBpEAAB6hAAAAAAAAAAAAAAAAAAAAAAAAAAABAAAAAAAAAAAAAAAAAAAA",
  "fee_meta_xdr": "AAAAAgAAAAMAAB63AAAAAAAAAAAW8Qst5i4N4Yk6l+FVBBKetKRTqyTcWDNSbcV08F4WNg3gtrN3tPO0AAAAAwAAAAQAAAAAAAAAAQAAAAAfpR7UY4+Podv9E4/IgUImlA8y6OKEQi8lJ8rRR8wMiAAAAAAAAAAAAQAAAAAAAAAAAAAAAAAAAAAAAAEAAB6/AAAAAAAAAAAW8Qst5i4N4Yk6l+FVBBKetKRTqyTcWDNSbcV08F4WNg3gtrN3tPNQAAAAAwAAAAUAAAAAAAAAAQAAAAAfpR7UY4+Podv9E4/IgUImlA8y6OKEQi8lJ8rRR8wMiAAAAAAAAAAAAQAAAAAAAAAAAAAAAAAAAA==",
  "memo_type": "text",
  "signatures": [
    "YLL32xpSbpqVchHKnQ02HY4VILJunUkY8M3alv/4XDTv9fvLiU8Sn8HpYhXXfRr7Z5j7lPe6NuIprpMhxRo3BA=="
  ]
}

Notice there is the "memo_type" element with the value of "text". This suggests there should be a "memo" element somewhere(because client SDKs don't allow null memos of type text). If we decode the envelope_xdr we can see there is indeed a MemoText in there, with a value of empty string.

This will cause issues when client SDKs try to deserialize this transaction from JSON. I have tested this in the official java-stellar-sdk and it does indeed throw a null reference exception.

@bartekn
Copy link
Contributor

bartekn commented Nov 8, 2017

I created this test:

    @Test
    public void testTransactionEnvelopeWithEmptyMemoText() throws IOException {
        String transactionEnvelopeToDecode = "AAAAABbxCy3mLg3hiTqX4VUEEp60pFOrJNxYM1JtxXTwXhY2AAAAZAAAAAMAAAAFAAAAAAAAAAEAAAAAAAAAAQAAAAAAAAAJAAAAAAAAAAHwXhY2AAAAQGCy99saUm6alXIRyp0NNh2OFSCybp1JGPDN2pb/+Fw07/X7y4lPEp/B6WIV130a+2eY+5T3ujbiKa6TIcUaNwQ=";
        Base64 base64Codec = new Base64();
        byte[] bytes = base64Codec.decode(transactionEnvelopeToDecode);

        TransactionEnvelope transactionEnvelope = TransactionEnvelope.decode(new XdrDataInputStream(new ByteArrayInputStream(bytes)));
        assertEquals(MemoType.MEMO_TEXT, transactionEnvelope.getTx().getMemo().getDiscriminant());
        assertEquals("", transactionEnvelope.getTx().getMemo().getText());
    }

and it's working fine. Could you provide a code that throws null reference exception?

@bartekn
Copy link
Contributor

bartekn commented Nov 8, 2017

OK, I understand what's the issue. This is happening because of the way encoding/json works on structs in Go, if transaction has an empty memo_text value, the memo field won't be present in a JSON representation of a transaction. I don't think we want to change this behaviour in Horizon. @nullstyle @tomerweller @nikhilsaraf any opinion here?

This is fixed in lightsail-network/java-stellar-sdk#54.

@nullstyle
Copy link
Contributor

IMO, the memo field should only be missing when the memo type is "none". This seems like a bug in horizon to me.

@elucidsoft
Copy link
Contributor

I agree, if the memo_type is "none", then and only then should the memo field be missing. Seems like a bug in Horizon.

@nikhilsaraf
Copy link
Contributor

+1 on the memo_type="none" -> missing memo field

to @bartekn's comment, should we disallow an empty value for the memo_text field?

@bartekn
Copy link
Contributor

bartekn commented Nov 9, 2017

to @bartekn's comment, should we disallow an empty value for the memo_text field?

Core accepts such transactions so we probably shouldn't do it. I was thinking more like: there's no easy way to fix this in encoding/json with structs and omitempty fields in Go but then I realized we can probably create an intermediate struct in such cases or a map so it shouldn't be that hard to fix.

@nullstyle
Copy link
Contributor

nullstyle commented Nov 9, 2017

@nikhilsaraf we can't disallow empty text memos. It's valid in the core protocol and I imagine it will remain that way. No memo is a distinct value from a text memo that's an empty string.

Basically, we need to extract https://github.com/stellar/go/blob/master/services/horizon/internal/resource/main.go#L238-L239 into another struct that can more tightly control the encoding behavior by defining an implementation of https://golang.org/pkg/encoding/json/#Marshaler

edit: yeah, what @bartekn said 😛

@nikhilsaraf
Copy link
Contributor

Cool, thanks for clarifying. Agree with the separated struct with marshaler approach.

@kr
Copy link
Contributor

kr commented Sep 5, 2018

Hey, @lechengfan and I would like to work on this!

bartekn added a commit that referenced this issue Apr 23, 2020
… string (#2504)

This PR fixes a problem similar to #149: memo_bytes was not present in
transaction response for empty string memo.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging a pull request may close this issue.

7 participants