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

Structured debug events #713

Merged
merged 18 commits into from
Mar 14, 2023
Merged

Structured debug events #713

merged 18 commits into from
Mar 14, 2023

Conversation

sisuresh
Copy link
Contributor

@sisuresh sisuresh commented Feb 25, 2023

Refactor of #640.

Add diagnostic ContractEvents that are not metered. I used ContractEvents because they are structured, allowing downstream consumers to parse a format they should already be familiar with if they consume regular events. These are not the same as the existing DebugEvents, which are non structured messages returned by the host. These diagnostic events are also returned by the host, but should be included in a non hashed portion of tx meta.

The associated core and xdr changes would look something like sisuresh/stellar-core@78d67fd and sisuresh/stellar-xdr-next@878cc77.

Note that -

  1. InternalEventsBuffer::record needs metering added for non structured debug events.
  2. Removes the mechanism that converts ContractEvents to DebugEvents when they're rolled back.
  3. Stops rolling back objects.

Copy link
Contributor

@graydon graydon left a comment

Choose a reason for hiding this comment

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

Structurally seems ok, a handful of implementation nits (especially the back-and-forth between XDR and host domain objects/values).

soroban-env-host/src/events/internal.rs Outdated Show resolved Hide resolved
soroban-env-host/src/host.rs Show resolved Hide resolved
soroban-env-host/src/host.rs Outdated Show resolved Hide resolved
soroban-env-host/src/host/diagnostics_helper.rs Outdated Show resolved Hide resolved
soroban-env-host/src/host/diagnostics_helper.rs Outdated Show resolved Hide resolved
soroban-env-host/src/host/metered_clone.rs Outdated Show resolved Hide resolved
soroban-env-host/src/host.rs Outdated Show resolved Hide resolved
soroban-env-host/src/events/internal.rs Outdated Show resolved Hide resolved
soroban-env-host/src/events/internal.rs Outdated Show resolved Hide resolved
soroban-env-host/src/host/diagnostics_helper.rs Outdated Show resolved Hide resolved
@graydon
Copy link
Contributor

graydon commented Mar 11, 2023

@sisuresh resolved most conversations, thanks! a few tiny nits remain but none are blocking, I'll set review-approved and you can merge whenever based on your preference re: fixing them or just moving on :)

@graydon
Copy link
Contributor

graydon commented Mar 11, 2023

(I believe you can push the "resolve" button on those remaining conversations, if I understand GH policy correctly)

@sisuresh sisuresh requested a review from a team as a code owner March 14, 2023 01:29
@sisuresh sisuresh merged commit 188ffa6 into stellar:main Mar 14, 2023
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.

4 participants