Skip to content

Conversation

@joallard
Copy link
Contributor

@joallard joallard commented Jun 7, 2020

Without strict, produces newline characters which Horizon will reject


(Split from #95:)

And so I was getting some TransactionMalformed errors. Looks like this is because Base64.encode64 by default inserts some newlines, which Horizon doesn't like.

The tests probably didn't catch this because of some combination of using VCR and upgrades on our side.

@joallard joallard changed the title Client#submit_tx.: Use strict_encode64 Client#submit_tx.: Fix Base64 encoding Jun 7, 2020
@nebolsin
Copy link
Member

nebolsin commented Jun 7, 2020

@joallard Thank you for the quick update! I know it's a lot more back and forth than you would expect for a one-liner change, but it seems that Rubocop is unhappy now because the single-quoted string literals violate the standard style guide we're adopting for this SDK (in case you're interested, we have a blog post which outlines the reasons behind this practice).

Both double-quoted string literal and symbol would be in line with a style guide, although using :base64 symbol feels a bit cleaner and more semantically correct for this specific case.

@nebolsin nebolsin self-assigned this Jun 7, 2020
@nebolsin nebolsin added this to the sdk-0.9.0 milestone Jun 7, 2020
@joallard
Copy link
Contributor Author

joallard commented Jun 7, 2020

No problem, I personally prefer up-to-the-notch code. I'll amend that real quick.

Ruby's standard Base64.encode64 inserts newline separators by default, 
which Horizon doesn't accept.
@sonarqubecloud
Copy link

sonarqubecloud bot commented Jun 7, 2020

Kudos, SonarCloud Quality Gate passed!

Bug A 0 Bugs
Vulnerability A 0 Vulnerabilities (and Security Hotspot 0 Security Hotspots to review)
Code Smell A 0 Code Smells

No Coverage information No Coverage information
2.9% 2.9% Duplication

Copy link
Member

@nebolsin nebolsin left a comment

Choose a reason for hiding this comment

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

:lgtm:

@joallard
Copy link
Contributor Author

joallard commented Jun 7, 2020

I also wholeheartedly agree that in this case, 'base64' is not semantically a string, but rather a symbol, as it's code rather than data. The case/when's on the other side, which I tried to match, had confused me ;)

Gotta admire the commitment to real good code! (That said that makes me wonder about my other PR in which I took way more Ruby artistic license haha!)

@nebolsin nebolsin merged commit a17add7 into astroband:master Jun 7, 2020
@nebolsin nebolsin removed their assignment Jun 7, 2020
@joallard joallard deleted the fix-base64 branch June 7, 2020 23:16
@joallard
Copy link
Contributor Author

joallard commented Jun 7, 2020

Ah, good job on the cherry-picking and commit message rewording @nebolsin, this is very well phrased. It's the little things, as they say!

@nebolsin
Copy link
Member

nebolsin commented Jun 7, 2020

Very nice catch on this regression. Thank you! 👍

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants