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

Represent XDR strings as bytes #48

Merged
merged 4 commits into from
Dec 12, 2019
Merged

Conversation

tamirms
Copy link
Contributor

@tamirms tamirms commented Dec 10, 2019

According to stellar/go#2022 XDR strings are equivalent to XDR opaque fields. The Java XDR code generator represents XDR strings as Java Strings. This representation is incorrect because an XDR string has no restrictions on the byte format. It is possible for an XDR string to contain invalid ASCII or invalid unicode bytes. The safest representation of an XDR string is a byte array.

XDR strings are equivalent to XDR opaque fields.
An XDR string is a variable length list of bytes with no restrictions on the contents of the bytes.
Copy link
Member

@leighmcculloch leighmcculloch left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm not very familiar with this code, but the change makes sense. I don't see any other way we can ensure strings do not become mutated in Java by changing encodings.

One thing below that I think we're missing.

lib/xdrgen/generators/java.rb Show resolved Hide resolved
@tomerweller
Copy link

@tamirms can you attach an example of how a generated String class will look like? It's hard to understand from the generator.

I think we should add a toString() method to these classes that makes a best effort to stringify the byte-array. If it's not utf8 it can throw an exception or maybe even return a hex representation.

@tamirms
Copy link
Contributor Author

tamirms commented Dec 11, 2019

@tomerweller

checkout lightsail-network/java-stellar-sdk#259 to see what the generated output looks like. The Java sdk converts the bytes to a string. If the bytes are not valid utf 8 the Java String class will convert the bytes to valid utf8.

@tomerweller
Copy link

@tamirms my point was that every time you set a String or read one you will need to encode/decode utf8, so it might save some headache for consumers to have that functionality in the generated string class rather that in the sdk level.

So for example (and this is a matter of java style), having another ctor that takes in a String and a toString() method that handle byte array utf8 conversions behind the scenes, could save consumers the trouble of thinking in byte arrays and utf8.

Some references from the mentioned pr:
https://github.com/stellar/java-stellar-sdk/pull/259/files#diff-420db4b3433ba4cb9daba85f6dd6a5a8R46
https://github.com/stellar/java-stellar-sdk/pull/259/files#diff-420db4b3433ba4cb9daba85f6dd6a5a8R73
https://github.com/stellar/java-stellar-sdk/pull/259/files#diff-11149bfd1ec2bd50ce92029356b7f057R18
https://github.com/stellar/java-stellar-sdk/pull/259/files#diff-11149bfd1ec2bd50ce92029356b7f057R29
https://github.com/stellar/java-stellar-sdk/pull/259/files#diff-fca6cbe50b2719ae470794aa856f6660R153
https://github.com/stellar/java-stellar-sdk/pull/259/files#diff-fca6cbe50b2719ae470794aa856f6660R211

@tamirms
Copy link
Contributor Author

tamirms commented Dec 11, 2019

@tomerweller please check the most recent commit

Copy link
Member

@leighmcculloch leighmcculloch left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This looks really good. The XdrString is a great idea. 👏

Copy link

@tomerweller tomerweller left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Good call on introducing a new XDR String class that handles encoding/decoding. (Much more elegant that my proposed solution)

I think this is a great contract, and also very readable code. So this is approved.

One small comment inline, but that's a matter of style and up to you.


public class XdrString implements XdrElement {
private byte[] bytes;
private String text;

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm not a fan of keeping the same state in two variables. I'd get rid of the text member and just keep the bytes, as that's the closest representation to what's on the wire.


@Override
public String toString() {
return this.text;

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Following on the previous comment, you can construct a new String here from bytes, instead of keeping text around.

Copy link

@tomerweller tomerweller left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

Copy link
Contributor

@bartekn bartekn left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM! Tested in java-stellar-sdk (after changing the code to be compatible) and works like a charm.

@Override
public void encode(XdrDataOutputStream stream) throws IOException {
stream.writeInt(this.bytes.length);
stream.write(this.bytes, 0, this.bytes.length);
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We should probably add a maxSize check here too.

@tamirms tamirms merged commit 08e9c35 into stellar:master Dec 12, 2019
@tamirms tamirms deleted the xdr-string branch December 12, 2019 15:55
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants