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

Memo check doesn't take into account ASCII encoding #257

Closed
Gzernov opened this issue Dec 5, 2019 · 7 comments · Fixed by #259
Closed

Memo check doesn't take into account ASCII encoding #257

Gzernov opened this issue Dec 5, 2019 · 7 comments · Fixed by #259
Assignees
Labels

Comments

@Gzernov
Copy link

Gzernov commented Dec 5, 2019

When this transaction was decoded from Json using SDK it failed with following Exception:

Caused by: org.stellar.sdk.MemoTooLongException: text must be <= 28 bytes. length=54
        at org.stellar.sdk.MemoText.<init>(MemoText.java:21)

As text memo could use either UTF8 or ASCII encoding second option should be checked as well, when decoding transactions from json.

@leighmcculloch
Copy link
Contributor

A memo could also use another encoding that isn't ASCII or UTF-8, so I'm unconvinced we should limit to decoding the data with just those two. Java String's are UTF-16, so unfortunately the moment we decode the memo from bytes into the String we're possibly changing the string.

Could we instead change how memos are decoded so that the MemoText class will accept as byte[] and will pass that through? The MemoText can still support String` too as an input, but will convert that into UTF-8 bytes at the time of input.

@Gzernov
Copy link
Author

Gzernov commented Dec 7, 2019

Yes, you are right and memo could use any encoding. Currently, there are 2 places where MemoText is used:

  1. User input (somebody using SDK attaching memo to transaction) -- which we could validate and ask user to provide encoding (or just as you suggested pass a byte array). However, if I understand correctly (I haven't looked in go sources though), it must be encoded in ASCII (?) and would be rejected by Horizon if it's too long
  2. The current problem is parsing transaction Json received from blockchain. In that scenario do we really need to validate the string at all? My implementation still validates the string but under assumption that string byte representation using ASCII/UTF-8 is no longer than 28 bytes.

So, regarding your comment.

Java String's are UTF-16, so unfortunately the moment we decode the memo from bytes into the String we're possibly changing the string.

Yes, that's true, so for user input we can ask for byte string representation. (Q: how do we encoded it? Using UTF-8/ASCII? Using user-provided encoding?)

Could we instead change how memos are decoded so that the MemoText class will accept as byte[] and will pass that through?

But we still need to convert it to string to submit to Horizon, don't we? Should we use ASCII?

The MemoText can still support String too as an input, but will convert that into UTF-8 bytes at the time of input.

Won't there be a problem with string that are too long in UTF-8 encoding but are short enough in ASCII encoding? (As in example transaction)

Thanks!

@Gzernov
Copy link
Author

Gzernov commented Dec 7, 2019

Another question why can't we simply use memo from XDR as ground truth on decoding stage?

@tamirms
Copy link
Contributor

tamirms commented Dec 9, 2019

@inkon I have opened #259 please comment on the PR with any feedback you may have

@Gzernov
Copy link
Author

Gzernov commented Dec 9, 2019

@inkon I have opened #259 please comment on the PR with any feedback you may have

Thanks, the change LGTM!

@tamirms
Copy link
Contributor

tamirms commented Dec 13, 2019

@inkon the fix was included in the 0.12.0 release

@Gzernov
Copy link
Author

Gzernov commented Dec 13, 2019

@tamirms thank you very much! 🙏

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging a pull request may close this issue.

3 participants