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

Augment getTransaction response to include events emitted during the transaction #16

Closed
tamirms opened this issue Oct 20, 2023 · 10 comments · Fixed by #58
Closed

Augment getTransaction response to include events emitted during the transaction #16

tamirms opened this issue Oct 20, 2023 · 10 comments · Fixed by #58
Assignees

Comments

@tamirms
Copy link
Contributor

tamirms commented Oct 20, 2023

What problem does your feature solve?

There is no easy way to get events for a transaction. If you have a failed transaction in particular, it would be nice to be able to debug why the tx failed by viewing the diagnostic events that were emitted during the transaction.

What would you like to see?

Include events in the getTransaction response so we can see what events where emitted during the transaction.

What alternatives are there?

Manually decoding the tx meta xdr in the getTransaction response and then extract the events from the tx meta.

You could also find events for a tx using two rpc calls:

  1. Call getTransaction to get the ledger and tx index for the transaction in question.
  2. Call getEvents with the cursor set appropriately so that includes the ledger and tx index
@mollykarcher
Copy link
Contributor

We'd like to keep this within getEvents, rather than getTransaction if possible. We should investigate if it's possible to do this in a single call when adding additional indices.

@tyvdh can you give us some context on how people are typically attempting to use this?

@2opremio
Copy link
Contributor

2opremio commented Oct 31, 2023

You could also find events for a tx using two rpc calls:

1. Call `getTransaction` to get the ledger and tx index for the transaction in question.

2. Call `getEvents` with the cursor set appropriately so that includes the ledger and tx index

I would advocate for this.

We can even make it a single call if we keep an in-memory index of txHash -> Ledger+tx (which shouldn't be too hard)

Then we can simply add StartTransactionHash to the getEvents request

@tamirms
Copy link
Contributor Author

tamirms commented Oct 31, 2023

It would be easier to include the events in the getTransaction response because we already have the tx meta for the transaction stored in-memory. We could implement this feature by parsing out the events from the tx meta in the getTransaction handler. We wouldn't need to store any additional data in the transaction or event memory stores.

@kalepail
Copy link
Contributor

kalepail commented Nov 2, 2023

I'm actually not sure how painful of an issue this is. With recent improvements to the JS SDK I've been using:

Manually decoding the tx meta xdr in the getTransaction response and then extract the events from the tx meta.

And that works fine. There are other things the getEvents and getTransaction endpoints need but at least for me and the Discord conversations I'm seeing this isn't a pain point.

@kalepail
Copy link
Contributor

kalepail commented Nov 2, 2023

I will say it would be nice to be able to call getEvents with a transaction hash to get that transactions events but I'm guessing the indexing overhead wouldn't be worth it

@leighmcculloch
Copy link
Member

It would be easier to include the events in the getTransaction response because we already have the tx meta for the transaction stored in-memory.

@tamirms Could the getEvents endpoint when a tx id is passed be backed by the getTransaction?

@tamirms
Copy link
Contributor Author

tamirms commented Nov 16, 2023

@leighmcculloch you are right, we could allow the getEvents endpoint to be called with a tx id. The implementation should be pretty simple as we can lookup the transaction by hash using the in-memory transaction store that backs the getTransaction endpoint. Once we have the transaction, we can decode the events from the tx meta in the transaction

@psheth9
Copy link
Contributor

psheth9 commented Jan 10, 2024

Posting this questions from my limited understanding :)

  1. Based on discussion it seems like we want to update getEvents request/response and not the getTxn response.
    If that is the case, how do we know the required fields of getEvents endpoint e.g startLedger and filter? Is it okay to make it optional or will users have all those details while calling?

  2. It looks like users use getTxn endpoint in order to check the status of specific transaction, so why can we not add the event data for failed txn there? imo seems like better place than getEvents.

@tomerweller
Copy link

tomerweller commented Jan 29, 2024

I think that this issue got side tracked a bit with getEvents. I support @tamirms original ask of including events in the result of getTransaction.

Events are important in the context of the getTransaction result. Clients are already polling that endpoint for success/failure and when they get it inspecting for the events emitted makes sense. We know that developers are already doing this but they are forced to decode metas for it, which is not great in terms of ergonomics. Asking the developer to make another request to get that information is also not great ergonomics.

From an implementation perspective do we really need to use the events backend? Why not decode events from metas on the fly when handling these requests?

@2opremio
Copy link
Contributor

From an implementation perspective do we really need to use the events backend? Why not decode events from metas on the fly when handling these requests?

Yep, we can do this.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
Status: Done
Development

Successfully merging a pull request may close this issue.

7 participants