Skip to content

Create TransactionEventPayload struct and add vm_error to it #6025

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

Open
wants to merge 14 commits into
base: develop
Choose a base branch
from

Conversation

jferrant
Copy link
Contributor

Closes #6021

@jferrant jferrant force-pushed the chore/add-vm-error-to-transaction-event-payload branch from 72f575f to 1e55e15 Compare April 18, 2025 18:43
Signed-off-by: Jacinta Ferrant <[email protected]>
Signed-off-by: Jacinta Ferrant <[email protected]>
Signed-off-by: Jacinta Ferrant <[email protected]>
@jferrant jferrant marked this pull request as ready for review April 18, 2025 20:45
@jferrant jferrant requested a review from a team as a code owner April 18, 2025 20:45
… into chore/add-vm-error-to-transaction-event-payload
@jferrant jferrant requested review from kantai and obycode April 18, 2025 20:46
Copy link

codecov bot commented Apr 19, 2025

Codecov Report

Attention: Patch coverage is 87.09677% with 16 lines in your changes missing coverage. Please review.

Project coverage is 84.00%. Comparing base (cf9fc79) to head (707c09a).
Report is 32 commits behind head on develop.

Files with missing lines Patch % Lines
stackslib/src/net/api/mod.rs 42.85% 12 Missing ⚠️
testnet/stacks-node/src/event_dispatcher.rs 95.69% 4 Missing ⚠️
Additional details and impacted files
@@             Coverage Diff             @@
##           develop    #6025      +/-   ##
===========================================
- Coverage    84.10%   84.00%   -0.10%     
===========================================
  Files          528      528              
  Lines       385765   386373     +608     
  Branches       323      323              
===========================================
+ Hits        324442   324577     +135     
- Misses       61315    61788     +473     
  Partials         8        8              
Files with missing lines Coverage Δ
stackslib/src/chainstate/burn/operations/mod.rs 63.57% <100.00%> (+1.29%) ⬆️
testnet/stacks-node/src/event_dispatcher.rs 90.42% <95.69%> (+0.29%) ⬆️
stackslib/src/net/api/mod.rs 84.55% <42.85%> (-7.62%) ⬇️

... and 50 files with indirect coverage changes


Continue to review full report in Codecov by Sentry.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update a7646f9...707c09a. Read the comment docs.

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.
  • 📦 JS Bundle Analysis: Save yourself from yourself by tracking and limiting bundle sizes in JS merges.

Copy link
Contributor

@obycode obycode left a comment

Choose a reason for hiding this comment

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

Looks good, just two small requests.

@jferrant jferrant requested a review from obycode April 22, 2025 23:17
Signed-off-by: Jacinta Ferrant <[email protected]>
@jferrant jferrant requested a review from obycode April 26, 2025 03:21
obycode
obycode previously approved these changes Apr 28, 2025
Copy link
Contributor

@obycode obycode left a comment

Choose a reason for hiding this comment

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

👍

Copy link
Contributor

@kantai kantai left a comment

Choose a reason for hiding this comment

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

This looks good to me, but we should have some tests that make tighter assertions about the outputted JSON -- we want this to be compatible with the previous version, so we should add some explicit equality checks on the outputted JSON with fixtures generated from the prior version.

…rds compatibility satisfied for TransactionEventPayload

Signed-off-by: Jacinta Ferrant <[email protected]>
Signed-off-by: Jacinta Ferrant <[email protected]>
… into chore/add-vm-error-to-transaction-event-payload
@jferrant
Copy link
Contributor Author

This looks good to me, but we should have some tests that make tighter assertions about the outputted JSON -- we want this to be compatible with the previous version, so we should add some explicit equality checks on the outputted JSON with fixtures generated from the prior version.

Added a proper deserializer for the custom block stack ops. Not sure that is exactly what you want. The equality check on the outputted JSON will not match. In my update, we now output the vm_error whereas before we did not. So the results will not match. They match on everything else and now that I have added the deserializer, it shows that it can serialize and deserialize into the same result whether using the legacy output or the current output. Note though that the serialization of some of those blockstack ops are lossy (For example PoxAddresses are serialized in a way that is lossy)

@jferrant jferrant requested review from kantai and obycode April 30, 2025 00:59
… into chore/add-vm-error-to-transaction-event-payload
… into chore/add-vm-error-to-transaction-event-payload
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.

3 participants