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

LoadAccountMergeAmount does not check for horizon error #529

Closed
mlsteele opened this issue Jul 6, 2018 · 2 comments
Closed

LoadAccountMergeAmount does not check for horizon error #529

mlsteele opened this issue Jul 6, 2018 · 2 comments
Assignees

Comments

@mlsteele
Copy link
Contributor

mlsteele commented Jul 6, 2018

Client.LoadAccountMergeAmount makes a request to horizon but does not check for an error. As a result if there is a horizon error it will likely always return this error, instead of the horizon error.

Could not find `account_credited` effect in `account_merge` operation effects`.

It should probably use something like decodeResponse to unmarshal the horizon response. Although I don't understand why that method accepts http status [200, 300) instead of just 200.

Here is a hacked up standalone example of the gist of this bug.

@robertDurst robertDurst self-assigned this Jul 26, 2018
@robertDurst
Copy link
Contributor

Good catch! Checking this out

@robertDurst
Copy link
Contributor

Opening a pr -- yes, this should use decodeResponse since decodeResponse does more than just decode, it checks status and then if it is unsuccessful, returns the error.

bartekn added a commit that referenced this issue Jul 31, 2018
Fix LoadAccountMerge so that it returns horizon errors #529
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

No branches or pull requests

2 participants