-
Notifications
You must be signed in to change notification settings - Fork 159
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
Implement partial protocol 13 support #276
Conversation
@@ -154,28 +175,28 @@ public int getFee() { | |||
/** | |||
* Generates Transaction XDR object. | |||
*/ | |||
public org.stellar.sdk.xdr.Transaction toXdr() { | |||
public TransactionV0 toXdr() { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Isn't xdr.Transaction
a default now?
public TransactionV0 toXdr() { | |
public Transaction toXdr() { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
until protocol 13 is enabled the sdks should still be generating v0 transactions. We will do another release after protocol 13 is enabled so that the sdks start generating v1 transactions. But, this method should be private the SDK users can get the xdr envelope. they don't need a public method to get the transaction from the envelope. I will change this method to private
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Our previous code using stellar api is broken now. I was told to use the latest version. One of place broken is that we were getting transaction from transaction envelope. Obviouly, it is broken now. Could you please let me know what is the alternative way to get the transaction from the envelope with current version? Are we suppose to keep compatability in version upgrading?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@wumingmo can you please create an issue and provide some example code which demonstrates the broken behavior? thanks!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks, Tamirms, for quick reponse, I created a pull request before, #284, but was closed and told to use the latest version. Of course, after I use the 0.16.0, I got the compilation error. If possible, could you please let me know how to extract the transaction from the envelope with 0.16.0? Thanks
if (encoded.length == 0) { | ||
throw new IllegalArgumentException("Encoded char array cannot be empty."); | ||
} | ||
|
||
byte[] bytes = new byte[encoded.length]; | ||
for (int i = 0; i < encoded.length; i++) { | ||
if (encoded[i] > 127) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Not relevant to this PR, but why is this necesary? base32Encoding.decode
should be taking care of this, shouldn't it?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Maybe this is needed due to guava
not handling it properly ...
This PR still does not handle fee bump transactions and updates to SEP 10 validation. I will add support for both in the next PR.