Use TransactionInfo within GetTransactionResponse
#251
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Shaptic
requested review from
aditya1702,
psheth9,
2opremio and
a team
and removed request for
aditya1702 and
psheth9
Shaptic
changed the title
TransactionInfo within GetTransactionResponse
Shaptic
changed the title
TransactionInfo within GetTransactionResponseTransactionInfo within GetTransactionResponse
psheth9
approved these changes
Jul 17, 2024
| @@ -29,8 +29,6 @@ const ( | |||
|
|
|||
| // GetTransactionResponse is the response for the Soroban-RPC getTransaction() endpoint | |||
| type GetTransactionResponse struct { | |||
| // Status is one of: TransactionSuccess, TransactionNotFound, or TransactionFailed. | |||
| Status string `json:"status"` | |||
There was a problem hiding this comment.
We could still keep the Status field to show what actually happened with Txn. Any strong motive to remove this field? or is it rarely used by users?
There was a problem hiding this comment.
Well it's still part of TransactionInfo. It can take on one additional value (TransactionNotFound) for this endpoint, but since it's a string regardless this doesn't impact the code.
This was referenced Oct 10, 2024
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
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.
WARNING: This is a breaking API change; see Known limitations, below.
What
Use
TransactionInfo(from thegetTransactions()handler) as part of thegetTransaction()handler.Why
One is a subset of the other, and this makes it easier for both the code and documentation to reflect that.
It also guarantees that the endpoints don't diverge in the future.
This is partially related to #124, where I realized I was duplicating work by parsing out the transaction fields separately for each endpoint: this would make the work more streamlined.
Known limitations
According to the CI integration test runs, this breaks
getTransaction's integration tests becausecreatedAtis encoded as anint64rather than astringin the schema forgetTransactions. We need to change this for JavaScript which is a breaking API change.