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

services/horizon + horizonclient: Add new async transaction submission endpoint #5188

Merged
merged 86 commits into from
Apr 26, 2024

Conversation

aditya1702
Copy link
Contributor

@aditya1702 aditya1702 commented Jan 31, 2024

PR Checklist

PR Structure

  • This PR has reasonably narrow scope (if not, break it down into smaller PRs).
  • This PR avoids mixing refactoring changes with feature changes (split into two PRs
    otherwise).
  • This PR's title starts with name of package that is most changed in the PR, ex.
    services/friendbot, or all or doc if the changes are broad or impact many
    packages.

Thoroughness

  • This PR adds tests for the most critical parts of the new functionality or fixes.
  • I've updated any docs (developer docs, .md
    files, etc... affected by this change). Take a look in the docs folder for a given service,
    like this one.

Release planning

  • I've updated the relevant CHANGELOG (here for Horizon) if
    needed with deprecations, added features, breaking changes, and DB schema changes.
  • I've decided if this PR requires a new major/minor version according to
    semver, or if it's mainly a patch change. The PR is targeted at the next
    release branch if it's not a patch change.

What

Closes #4082

Adds the following:

  • New endpoint transactions-async that allows clients to submit transactions and get a response immediately. It does not block unlike the current endpoint.
  • Unittests for the endpoint.
  • Integration tests for the endpoint.
  • Corresponding code in horizonclient SDK.

Why

  • The current endpoint blocks until Horizon ingests the transaction in the DB. This implementation removes the wait and returns an immediate response.
  • It brings the transaction submission process in sync with Soroban-RPC's implementation.

More on the requirements about this endpoint here: https://docs.google.com/document/d/1uiEnZJ-Yv1-8fBe8A8FDeR_HbpsyQLnY7KnlsTKl4vk/edit

Known limitations

Once implementation is finalised from the team, I will update the documentation wherever necessary in the stellar-docs.

@aditya1702 aditya1702 self-assigned this Jan 31, 2024
@aditya1702 aditya1702 requested review from JakeUrban and Shaptic and removed request for Shaptic January 31, 2024 20:41
@aditya1702 aditya1702 changed the title [WIP] services/horizon: Add new async transaction submission endpoint [WIP] services/horizon + horizonclient: Add new async transaction submission endpoint Feb 26, 2024
@aditya1702 aditya1702 marked this pull request as ready for review February 27, 2024 23:02
@aditya1702 aditya1702 requested a review from tamirms April 25, 2024 20:37
Copy link
Contributor

@tamirms tamirms left a comment

Choose a reason for hiding this comment

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

🎉

@overcat
Copy link
Contributor

overcat commented Jul 25, 2024

Hi @aditya1702, according to the design intention, a user submitted a transaction through this interface, and it was successfully executed. If the user submits the same transaction again through this interface (which is completely identical to the previous one), what kind of exception will the user receive?

// While this part of code assumes that any error < 200 or error >= 300 is a Horizon problem, it is not
// true for the response from /transactions_async endpoint which does give these codes for certain responses
// from core.
if !(resp.StatusCode >= 200 && resp.StatusCode < 300) && (resp.Request == nil || resp.Request.URL == nil || resp.Request.URL.Path != "/transactions_async") {
Copy link
Contributor

@overcat overcat Jul 26, 2024

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:

{
  "type": "https://stellar.org/horizon-errors/transaction_malformed",
  "title": "Transaction Malformed",
  "status": 400,
  "detail": "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.",
  "extras": {
    "envelope_xdr": "AAAAAgAAAAAoXi7Kz8/PAl9GZl5AD09H080g961ptb1MMfj6evUsJwAAAGQACuJqAAAAEQAAAAEAAAAAAAAAAAAAAABmoxT9AAAAAQAAAA9IZWxsbywgU3RlbGxhciEAAAAAAQAAAAAAAAABAAAAAPSbkKYju7E2GtKbpAnQ0twJCAGBwIcxea690WueeVSjAAAAAAAAAADQsJmHAAAAAAAAAAF69SwnAAAAQHgHTIiwgcIvdm0//yTH9o9rsGlMCng7YjKrOszNaJ0qwvPeLs8/bNdTRdg51gs3/RQAhT4lDeJR4Bs5wgZjeMgM=",
    "error": {}
  }
}

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.

// ErrorResultXDR is present only if Status is equal to proto.TXStatusError.
// ErrorResultXDR is a TransactionResult xdr string which contains details on why
// the transaction could not be accepted by stellar-core.
ErrorResultXDR string `json:"errorResultXdr,omitempty"`
Copy link
Contributor

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.

Copy link
Contributor Author

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.

Copy link
Contributor

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 be error_result_xdr.

Copy link
Contributor Author

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

Copy link
Contributor

@overcat overcat Aug 20, 2024

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.

package main

import client "github.com/stellar/go/clients/horizonclient"

func main() {
	client := client.Client{
		HorizonURL: "https://horizon-testnet.stellar.org",
	}
	_, err := client.AsyncSubmitTransactionXDR("invalidAAAAAgAAAAoXi7Kz8/PAl9GZl5AD09H080g961tb1MMfj6evUsJwAAAGQACuJqAAAAEgAAAAEAAAAAAAAAAAAAAABmo4DLAAAAAQAAAA9IZWxsbywgU3RlbGxhciEAAAAAAQAAAAAAAAABAAAAAPSbkKYju7E2GtKbpAnQ0twJCAGBwIcxea690WueeVSjAAAAAAAAAADQsJmHAAAAAAAAAAF69SwnAAAAQFAaIAq+4YAPnY7BEukwtmtiXVvHvjPi+6kJUpl/ljoYSmnZcMIUSFHH7LGct0ftWl7PnUKGE4JGDFk38tg8YAs=")
	if err != nil {
		println("error is not empty")
		panic(err)
	}
	println("We shouldn't have arrived here.")
}

Copy link
Contributor Author

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!

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

Successfully merging this pull request may close these issues.

services/horizon: support async transaction submission
7 participants