-
Notifications
You must be signed in to change notification settings - Fork 724
Export events in readonly endpoints #6741
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
base: develop
Are you sure you want to change the base?
Export events in readonly endpoints #6741
Conversation
Codecov Report❌ Patch coverage is ❌ Your project check has failed because the head coverage (68.98%) is below the target coverage (80.00%). You can increase the head coverage or adjust the target coverage.
Additional details and impacted files@@ Coverage Diff @@
## develop #6741 +/- ##
===========================================
- Coverage 78.18% 68.98% -9.20%
===========================================
Files 580 582 +2
Lines 361096 361659 +563
===========================================
- Hits 282312 249497 -32815
- Misses 78784 112162 +33378
... and 343 files with indirect coverage changes Continue to review full report in Codecov by Sentry.
🚀 New features to boost your workflow:
|
jcnelson
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Please see my earlier comment. There is no need to modify the Clarity VM to get events; the Clarity API already offers a way to return the events directly. It just needs to be used in the RPC endpoint implementation. Thanks!
brice-stacks
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM just a nit
aaronb-stacks
left a comment
There was a problem hiding this 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, and I definitely prefer this approach to the previous one. The only issue to me is still related to the event rollbacks.
As currently implemented, this endpoint will return events for code which is rolled back. This could be a correctness (and eventually a security) issue for consumers of the endpoint: if they only expect events for "successful" executions, they'd be surprised when events are included for executions which would have been rolled back.
I think this is solvable by expanding the callback trait to have like an on_rollback method. This probably needs a "reason" field so that you can tell if the rollback happened because of a tx abort, or because the read only call is explicitly getting rolled back (which I assume you want to ignore).
brice-stacks
left a comment
There was a problem hiding this 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, pending the solution Aaron mentioned above.
Description
This patch adds the list of events generated by read-only calls to the output of the readonly endpoints (base and fast).
This is the example output for a simple function:
{"events":[{"key":"print","sender":"ST2DS4MSWSGJ3W9FBC6BVT0Y92S345HY8N3T6AV7R.hello-world","value":"0000000000000000000000000000000064"},{"key":"print","sender":"ST2DS4MSWSGJ3W9FBC6BVT0Y92S345HY8N3T6AV7R.hello-world","value":"01000000000000000000000000000003e8"},{"key":"print","sender":"ST2DS4MSWSGJ3W9FBC6BVT0Y92S345HY8N3T6AV7R.hello-world","value":"0d0000000474657374"},{"key":"print","sender":"ST2DS4MSWSGJ3W9FBC6BVT0Y92S345HY8N3T6AV7R.hello-world","value":"03"}],"okay":true,"result":"0x070100000000000000000000000000000001"}Additional info (benefits, drawbacks, caveats)
The patch includes a refactoring of the two endpoints to share the contract call logic (they differs only for the cost tracker and the max_execution_time)
Checklist
docs/rpc/openapi.yamlandrpc-endpoints.mdfor v2 endpoints,event-dispatcher.mdfor new events)clarity-benchmarkingrepo