Raise useful Horizon errors #95

joallard · 2020-06-06T23:46:16Z

I caught this on the side while working on something else, my apologies for the missing tests; I didn't have time to figure out how to set up the accounts correctly just yet.

I ran into Horizon rejecting the XDR envelopes made from a regular send_payment request. The exception raised was as such:

     Faraday::BadRequestError:
       the server responded with status 400
     # ./lib/account.rb:20:in `send_payment'

Which is not exactly helpful. Which is why I'm humbly proposing to wrap those into exceptions ourselves to raise some more useful information. It would now become:

     Stellar::HorizonError::TransactionMalformed:
       Horizon could not decode the transaction envelope in this request. A transaction should be an XDR TransactionEnvelope struct encoded using base64.  The envelope read from this request is echoed in the `extras.envelope_xdr` field of this response for your convenience.

# or

     Stellar::HorizonError::TransactionFailed:
       The transaction failed when submitted to the stellar network. extras.result_codes contained: `{"transaction"=>"tx_failed", "operations"=>["op_no_destination"]}`  Descriptions of each code can be found at: https://www.stellar.org/developers/guides/concepts/list-of-operations.html

It also makes available .extras on the exception. (On further thought, original_hash should probably be named original_response).

[...]

After being done, I saw there were already Stellar::AccountRequiresMemoError and Stellar::Horizon::Problem, we could probably clean this up together if you'd like.

Cheers!

nebolsin · 2020-06-07T00:19:57Z

Hi, thank you for the PR! Unfortunately, we did a mistake during the recent stellar-base/stellar-sdk merge which resulted in commit history for the base gem completely missing, and in order to fix this mistake we've had to force-push the master (and I did it just now). Could you please rebase your PR against the updated master? The new master should be identical to the old one as far as the repository content is concerned, it's just a commit history which changed, so your patches should apply perfectly. Really sorry about this mess.

Also, I would suggest separating the bug fix and the error handling into different PRs. We definitely want to fix the base64 encoding bug for the upcoming release, but since we're in the rc1 stage already, I'm not sure if we should go ahead and include the error decoding part right now (also, as you pointed out, it needs some cleanup in regards to Stellar::Horizon::Problem/Stellar::HorizonError duplication).

joallard · 2020-06-07T00:31:24Z

Of course! Done. The Base64 fix is now in #96.

A simple test could take the form:

When I send a payment to an uncreated account
Then I get a Stellar::HorizonError::TransactionFailed error

charlie-wasp

Hi @joallard, thank you very much for this contribution! Looks good, I've got just a couple of small remarks on it

charlie-wasp · 2020-06-10T11:16:13Z

sdk/lib/stellar/horizon_error.rb

+  class HorizonError < StandardError
+    attr_reader :original_response, :extras
+
+    def self.make(hash)


Since we then assigned the hash parameter to the instance variable original_response, I believe it's better to name it the same here. Also it would make it more accurate. The same goes for the initialize method

charlie-wasp · 2020-06-10T11:21:36Z

sdk/lib/stellar/horizon_error.rb

+        title = nil
+      end
+
+      [title, detail].join(": ")


For non-generic class title will be nil, and this expression will result in ": detail", which is not what we would like to get 🙂

Good catch! I'll add a compact here

charlie-wasp · 2020-06-10T11:27:19Z

sdk/lib/stellar/horizon_error.rb

+
+    class TransactionFailed < self
+      def make_message(detail:)
+        detail.sub!(


Wouldn't such substitution be useful on the generic level? In other words, can we show extras in the message for every error? If they exist, of course

As far as I know, extras.result_codes is only provided for TransactionFailed's. That said, I only checked 2 or 3 types.

A cursory search seems to confirm.

charlie-wasp · 2020-06-10T11:38:09Z

I saw there were already Stellar::AccountRequiresMemoError and Stellar::Horizon::Problem, we could probably clean this up together if you'd like

I think we can get rid of Stellar::Horizon::Problem, it seems not used anywhere.

Stellar::AccountRequiresMemoError doesn't come straight from Horizon, it's more of application-level error, so I believe we can keep it with Stellar::HorizonError separately

Implements Stellar::HorizonError

sonarcloud · 2020-06-14T04:49:13Z

Kudos, SonarCloud Quality Gate passed!

0 Bugs
0 Vulnerabilities (and 0 Security Hotspots to review)
0 Code Smells

No Coverage information
2.9% Duplication

joallard · 2020-06-26T20:52:42Z

sdk/lib/stellar/horizon_error.rb

+    def make_message(detail:, title:)
+      # Include title only on generic class
+      # Condition passes only for child classes
+      if self.class != HorizonError


Just noticed .instance_of? existed

instance_of?(HorizonError)

nebolsin requested a review from a team June 6, 2020 23:49

joallard force-pushed the fixes branch from c9f08b9 to c79bfe8 Compare June 6, 2020 23:51

nebolsin force-pushed the master branch from 40b5c1e to 63b72ac Compare June 7, 2020 00:08

nebolsin self-assigned this Jun 7, 2020

nebolsin changed the base branch from master to master-missing-base-history June 7, 2020 00:25

joallard force-pushed the fixes branch from c79bfe8 to 720fa57 Compare June 7, 2020 00:26

nebolsin changed the base branch from master-missing-base-history to master June 7, 2020 00:26

joallard changed the title ~~Encode Base64 strictly + Raise useful Horizon errors~~ Raise useful Horizon errors Jun 7, 2020

joallard mentioned this pull request Jun 7, 2020

Client#submit_tx.: Fix Base64 encoding #96

Merged

joallard force-pushed the fixes branch from 720fa57 to 3b61810 Compare June 7, 2020 00:29

nebolsin added the stellar-sdk label Jun 7, 2020

joallard force-pushed the fixes branch from 3b61810 to 0ae47ae Compare June 7, 2020 23:42

charlie-wasp requested changes Jun 10, 2020

View reviewed changes

Client#submit_tx: Return HorizonError on bad requests

693a10f

Implements Stellar::HorizonError

joallard force-pushed the fixes branch from 0ae47ae to 693a10f Compare June 14, 2020 04:48

joallard commented Jun 26, 2020

View reviewed changes

nebolsin force-pushed the main branch from 2d0f07b to 2ccbf15 Compare May 7, 2021 20:13

nebolsin force-pushed the main branch from fcb1889 to fa6af73 Compare June 7, 2021 07:35

charlie-wasp force-pushed the main branch 3 times, most recently from a88d15e to e52ecc4 Compare September 6, 2021 17:03

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Raise useful Horizon errors #95

Raise useful Horizon errors #95

joallard commented Jun 6, 2020 •

edited

nebolsin commented Jun 7, 2020

joallard commented Jun 7, 2020 •

edited

charlie-wasp left a comment

charlie-wasp Jun 10, 2020

joallard Jun 10, 2020

charlie-wasp Jun 10, 2020

joallard Jun 10, 2020

charlie-wasp Jun 10, 2020

joallard Jun 10, 2020

joallard Jun 14, 2020

charlie-wasp commented Jun 10, 2020

sonarcloud bot commented Jun 14, 2020

joallard Jun 26, 2020

Raise useful Horizon errors #95

Are you sure you want to change the base?

Raise useful Horizon errors #95

Conversation

joallard commented Jun 6, 2020 • edited

nebolsin commented Jun 7, 2020

joallard commented Jun 7, 2020 • edited

charlie-wasp left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

charlie-wasp commented Jun 10, 2020

sonarcloud bot commented Jun 14, 2020

Choose a reason for hiding this comment

joallard commented Jun 6, 2020 •

edited

joallard commented Jun 7, 2020 •

edited