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

Correctly omit transactionData field when transaction simulation fails #271

Merged
merged 9 commits into from
Aug 28, 2024

Conversation

Shaptic
Copy link
Contributor

@Shaptic Shaptic commented Aug 20, 2024

What

Correctly handle the case where transaction data is empty. Also, add a bit of verbosity (the type name) to error messages coming from the conversion layer.

Why

Previously, the handler would unconditionally try to encode an empty byte array (since result.TransactionData was empty) and fail with an internal panic:

"xdr_to_json() failed: called `Result::unwrap()` on an `Err` value: Io(Error { kind: UnexpectedEof, message: \"failed to fill whole buffer\" })",

Obviously this is not ideal. It was occurring because the simulation failed, but the code would still try to JSONify results. In the base64 case, EncodeToString will just return an empty string on an empty buffer.

This code modifies xdr2json.ConvertBytes to check its inputs like so:

 func ConvertBytes(xdr interface{}, field []byte) ([]byte, error) {
+       if len(field) == 0 {
+               return []byte(""), nil
+       }
+

to behave akin to EncodeToString and handle this case "in general."

@Shaptic Shaptic added the bug Something isn't working label Aug 20, 2024
@Shaptic Shaptic added this to the platform sprint 50 milestone Aug 20, 2024
@Shaptic Shaptic requested review from 2opremio, sreuland, aditya1702, psheth9 and a team August 20, 2024 18:00
@Shaptic Shaptic self-assigned this Aug 20, 2024
@sreuland
Copy link
Contributor

Alternatively, we could modify xdr2json.ConvertBytes to check its inputs

that approach of sanitizing the json lower in conversion to empty string seems to be a bit cleaner, results in less conditionals in upper code, I would 👍 for that.

@Shaptic
Copy link
Contributor Author

Shaptic commented Aug 21, 2024

@sreuland agreed! Done ✔️

Copy link
Contributor

@psheth9 psheth9 left a comment

Choose a reason for hiding this comment

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

can you add few tests? overall looks good to me just one question on rust code

Copy link
Contributor

@sreuland sreuland left a comment

Choose a reason for hiding this comment

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

lgtm, for consideration, echo the same as other reviewer commented, about adding unit test on conversion.go if feasible.

@Shaptic
Copy link
Contributor Author

Shaptic commented Aug 23, 2024

Good calls @sreuland and @psheth9! Done :)

@Shaptic Shaptic enabled auto-merge (squash) August 28, 2024 20:00
@Shaptic Shaptic merged commit 487db7e into main Aug 28, 2024
18 of 19 checks passed
@Shaptic Shaptic deleted the fix-bad-simulate branch August 28, 2024 20:11
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working
Projects
Status: Done
Development

Successfully merging this pull request may close these issues.

3 participants