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

soroban-rpc: simulateTransaction: try to return diagnostic events on failure #878

Merged
merged 6 commits into from
Aug 22, 2023

Conversation

2opremio
Copy link
Contributor

@2opremio 2opremio commented Aug 21, 2023

Even if host.invoke_host_function() fails, leading to an error, we can collect and return diagnostic events.

Note that this implies that the events field of the simulate transaction response can be set when an error is returned.

Also, sneak in some review feedback from #875 which I incorrectly pushed to this branch (sorry @tsachiherman )

(Found out about it in stellar/rs-soroban-env#1008 ).

@willemneal we should probably do the same in the CLI's snapshot mode (in case we don't do it already).

@2opremio 2opremio changed the title soroban-rpc: simluateTransaction: try to return diagnostic events on failure soroban-rpc: simulateTransaction: try to return diagnostic events on failure Aug 21, 2023
@willemneal
Copy link
Member

I have another PR that is targeting the current futurenet branch: AhaLabs#8

But it should log all simulation events.

Copy link
Contributor

@Shaptic Shaptic left a comment

Choose a reason for hiding this comment

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

all for this 👏 the more error feedback to clients, the better

Copy link
Contributor

@tsachiherman tsachiherman left a comment

Choose a reason for hiding this comment

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

one tiny request, looks great otherwise.

cmd/soroban-rpc/internal/test/simulate_transaction_test.go Outdated Show resolved Hide resolved
@2opremio 2opremio force-pushed the also-return-events-on-failure branch from ad63115 to 53021ae Compare August 21, 2023 23:31
@2opremio 2opremio merged commit b53e6d4 into stellar:main Aug 22, 2023
19 of 20 checks passed
@2opremio 2opremio deleted the also-return-events-on-failure branch August 22, 2023 00:31
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 this pull request may close these issues.

4 participants