Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
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
services/horizon + horizonclient: Add new async transaction submission endpoint #5188
services/horizon + horizonclient: Add new async transaction submission endpoint #5188
Changes from 58 commits
475af3f
d4555b3
aa4084a
e883120
00e1a29
28a009e
2ed0054
6ff4fa2
525fb0e
9b7e3f6
31fd091
d425558
4f30e29
326e21e
37d01ab
c7e46af
ed812ea
7450b0a
808a4d8
17f8fee
1725c91
9dfc179
9a4e0d8
2b97e2d
1edb2b0
55fcb5f
fd03686
b95d57c
0e4142a
6ebb93a
b879bd4
d1a4eb9
aa31ab2
085352f
66a99df
2ba210d
d331e41
a096b5f
63ce1b9
0f14793
17611aa
e98fd22
eeddadc
f5ddbf2
5e498fb
ef96628
48932aa
9f50ddd
f34abbc
8d527f2
c44ca0d
9b3deb9
ee31a95
c646e0b
42cd689
21687d2
e931893
2114193
3aca364
5c0089e
d5b3459
9517e76
122a087
c571dd5
7e3e495
03b2c66
c869205
71893ba
be59a2c
667e6b2
d632a67
d31a01e
62d02d1
32c71f8
d3b8c1e
573bdb6
00985db
6e253de
1932250
e4f33e4
28e392a
ce3e6aa
30b0c25
1057ab3
401d2e2
c8d4bad
File filter
Filter by extension
Conversations
Jump to
There are no files selected for viewing
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.
I think there is a problem with the implementation here. For AsyncSubmitTransactionXDR, if the user passes in an invalid XDR, the data returned by Horizon is as follows:
However, due to this change, the err returned by AsyncSubmitTransactionXDR will be nil, which does not meet expectations.
BTW, for non-2xx status codes, Horizon returns responses in a format similar to the one above. The transactions_async endpoint is an exception to this.
Check failure on line 126 in clients/horizonclient/mocks.go
GitHub Actions / golangci
Check failure on line 132 in clients/horizonclient/mocks.go
GitHub Actions / golangci
Check failure on line 138 in clients/horizonclient/mocks.go
GitHub Actions / golangci
Check failure on line 16 in clients/stellarcore/mocks.go
GitHub Actions / golangci
Check failure on line 21 in clients/stellarcore/mocks.go
GitHub Actions / golangci
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.
Perhaps using
error_result_xdr
would be more consistent.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.
@overcat The
error_result_xdr
is returned from core when the transaction submission has issues. In the invalid XDR case, it never reaches core but fails as a validation check from Horizon.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.
@aditya1702 I think he's pointing out the naming convention
errorResultXdr
should probably beerror_result_xdr
.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.
Ah my bad, misunderstood your comment. @overcat Yes that is a good catch and I will include this change for the next release. However, I do want to note that this will be a breaking change to the API
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.
Hi @aditya1702,
Yes, this will be a breaking update. Some measures need to be considered before making the changes in order to ensure a smooth transition downstream.
Did you check my other comment? That's a bug. I think we need to fix it. Below is the code to reproduce 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.
@overcat Ah gotcha. Yes this is a problem here and I will need to change the condition in the
internal.go
file you linked above. Thanks for spotting this!