feat(clarity): Improve emitted events for Clarity post-conditions #4814

ldiego08 · 2024-05-21T20:30:06Z

Description

Adds post-condition validation status to Stacks transaction receipts.

Applicable issues

fixes Improve emitted events for Clarity post-conditions #4744

Additional info (benefits, drawbacks, caveats)

Checklist

Test coverage for new or modified code paths
Changelog is updated
Required documentation changes (e.g., docs/rpc/openapi.yaml and rpc-endpoints.md for v2 endpoints, event-dispatcher.md for new events)
New clarity functions have corresponding PR in clarity-benchmarking repo
New integration test(s) added to bitcoin-tests.yml

codecov · 2024-05-22T21:05:31Z

Codecov Report

Attention: Patch coverage is 86.35097% with 49 lines in your changes missing coverage. Please review.

Project coverage is 83.27%. Comparing base (abe0a0a) to head (fb41b66).
Report is 4 commits behind head on master.

Additional details and impacted files

@@             Coverage Diff             @@
##           master    #4814       +/-   ##
===========================================
+ Coverage   19.14%   83.27%   +64.12%     
===========================================
  Files         465      470        +5     
  Lines      333874   335454     +1580     
  Branches        0      323      +323     
===========================================
+ Hits        63932   279345   +215413     
+ Misses     269942    56101   -213841     
- Partials        0        8        +8

Files	Coverage Δ
stackslib/src/chainstate/stacks/db/blocks.rs	`90.09% <100.00%> (+66.53%)`	⬆️
stackslib/src/chainstate/stacks/events.rs	`83.33% <ø> (ø)`
testnet/stacks-node/src/event_dispatcher.rs	`85.44% <83.33%> (+27.00%)`	⬆️
stackslib/src/chainstate/stacks/db/transactions.rs	`97.58% <92.99%> (+93.35%)`	⬆️
stackslib/src/chainstate/stacks/mod.rs	`74.57% <25.71%> (+66.02%)`	⬆️

... and 434 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 b7f7295...fb41b66. Read the comment docs.

stackslib/src/chainstate/stacks/db/transactions.rs

stackslib/src/chainstate/stacks/events.rs

jcnelson

You're on the right track with this PR. Thanks for sending it over! 🙏 Just need to add some test coverage and address a couple other nits.

ldiego08 · 2024-05-26T16:34:53Z

@jcnelson, thanks for reviewing! This PR is ready for a second look.

Here's a summary of the changes:

Return a TransactionPostConditionStatus struct instead of just the failed post-condition to account for cases where moved assets aren't covered by a post-condition.
Update tests related to post-conditions to cover for introduced changes.
Add a post-condition status matcher to help assert in tests.

#4744 mentions that upstream event dispatchers should be updated as well, but it seems like they leverage the same StacksTransactionReceipt struct, so no changes are needed. If this is not the case, please let me know what else needs updating, and I'll do so!

ldiego08 · 2024-05-26T16:48:25Z

@jcnelson seems like #4744 refers to this code.. If so, what would the recommended JSON output be for the receipt's post_condition_status?

ldiego08 · 2024-05-30T21:19:44Z

testnet/stacks-node/src/event_dispatcher.rs

Update on event dispatcher JSON output: I've added the post_condition_status field to the payload object. Not entirely sure of its JSON shape. @jcnelson if a different shape is desired, please let me know.

ldiego08 · 2024-06-24T15:18:10Z

@jcnelson (or @wileyj) - this PR is ready for another look in case anyone has bandwidth.

wileyj · 2024-06-24T15:23:31Z

thanks @ldiego08 - can you rebase this against the develop branch?

…ions

… payload

CLAassistant · 2024-06-24T15:53:14Z

All committers have signed the CLA.

ldiego08 · 2024-06-24T15:58:03Z

@wileyj - done! Thanks for pointing me in the right direction. With the right base branch, reviewer groups have been tagged and other workflows triggered. :)

obycode

Thanks for this contribution @ldiego08! I have some suggestions that I think would simplify the code a bit.

obycode · 2024-06-25T12:48:39Z

stackslib/src/chainstate/stacks/db/transactions.rs

@@ -578,6 +591,7 @@ impl StacksChainState {
 post_condition_mode: &TransactionPostConditionMode,
 origin_account: &StacksAccount,
 asset_map: &AssetMap,
+ post_condition_statuses: &mut Vec<TransactionPostConditionStatus>,


How about instead of adding this vec, we change the return value to Result<Option<TransactionPostConditionStatus> and it returns Ok(None) on success or else Ok(Some(failing_condition))? I think that could simplify things.

@obycode I originally tried that approach, but things got tangled up quickly. This function is passed in a callback to the Clarity VM's run_contract_call and bubbling up the failed post-condition from here to here where the AbortedByCallback error is created would require a more extensive and invasive refactor. That's why I resourced to a "post-condition stream" vec as a simpler solution. Also, seems like the callback is expecting other error cases? Eager to hear your thoughts as I might be missing something.

Got it. I appreciate your patience; I have had some higher priority items recently. Someone will try to take a look soon and help push this forward.

stackslib/src/chainstate/stacks/mod.rs

obycode · 2024-06-25T16:03:53Z

stackslib/src/chainstate/stacks/mod.rs

+
+#[derive(Debug, Clone, PartialEq, Serialize, Deserialize)]
+pub enum TransactionPostConditionStatus {
+ Success,


I think we can remove this case.

stackslib/src/chainstate/stacks/db/transactions.rs

obycode · 2024-06-25T16:10:19Z

stackslib/src/chainstate/stacks/db/transactions.rs

@@ -1614,6 +1689,42 @@ pub mod test {
 &TestBurnStateDB_2_05 as &dyn BurnStateDB,
 ];

+ #[derive(Debug, Clone, PartialEq, Serialize, Deserialize)]
+ pub enum TransactionPostConditionStatusAssert {


I think this enum is unnecessary and could be removed with some refactoring of the test code.

ldiego08 mentioned this pull request May 21, 2024

Improve emitted events for Clarity post-conditions #4744

Open

jcnelson reviewed May 22, 2024

View reviewed changes

stackslib/src/chainstate/stacks/db/transactions.rs Outdated Show resolved Hide resolved

jcnelson reviewed May 22, 2024

View reviewed changes

stackslib/src/chainstate/stacks/events.rs Outdated Show resolved Hide resolved

jcnelson reviewed May 22, 2024

View reviewed changes

ldiego08 requested a review from jcnelson May 26, 2024 16:46

ldiego08 marked this pull request as ready for review May 30, 2024 21:16

ldiego08 commented May 30, 2024

View reviewed changes

ldiego08 added 3 commits June 24, 2024 09:48

feat(post-conditions): improve emitted events for Clarity post-condit…

1a35e84

…ions

chore(post-conditions): add tests and matchers

f530ec5

chore(post-conditions): add post-condition status to event dispatcher…

8323f0d

… payload

ldiego08 changed the base branch from master to develop June 24, 2024 15:49

ldiego08 requested review from a team as code owners June 24, 2024 15:49

ldiego08 changed the base branch from develop to master June 24, 2024 15:50

ldiego08 force-pushed the feat/improve-clarity-emitted-events-post-conditions branch from fb41b66 to 8323f0d Compare June 24, 2024 15:53

ldiego08 changed the base branch from master to develop June 24, 2024 15:53

wileyj requested review from obycode June 24, 2024 15:58

obycode reviewed Jun 25, 2024

View reviewed changes

chore: add more data to failed post-conditions

be118f7

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat(clarity): Improve emitted events for Clarity post-conditions #4814

feat(clarity): Improve emitted events for Clarity post-conditions #4814

ldiego08 commented May 21, 2024 •

edited

Loading

codecov bot commented May 22, 2024 •

edited

Loading

jcnelson left a comment

ldiego08 commented May 26, 2024

ldiego08 commented May 26, 2024

ldiego08 May 30, 2024

ldiego08 commented Jun 24, 2024

wileyj commented Jun 24, 2024

CLAassistant commented Jun 24, 2024 •

edited

Loading

ldiego08 commented Jun 24, 2024

obycode left a comment

obycode Jun 25, 2024

ldiego08 Jun 27, 2024 •

edited

Loading

obycode Jul 17, 2024

obycode Jun 25, 2024

obycode Jun 25, 2024

feat(clarity): Improve emitted events for Clarity post-conditions #4814

Are you sure you want to change the base?

feat(clarity): Improve emitted events for Clarity post-conditions #4814

Conversation

ldiego08 commented May 21, 2024 • edited Loading

Description

Applicable issues

Additional info (benefits, drawbacks, caveats)

Checklist

codecov bot commented May 22, 2024 • edited Loading

Codecov Report

jcnelson left a comment

Choose a reason for hiding this comment

ldiego08 commented May 26, 2024

ldiego08 commented May 26, 2024

ldiego08 May 30, 2024

Choose a reason for hiding this comment

ldiego08 commented Jun 24, 2024

wileyj commented Jun 24, 2024

CLAassistant commented Jun 24, 2024 • edited Loading

ldiego08 commented Jun 24, 2024

obycode left a comment

Choose a reason for hiding this comment

obycode Jun 25, 2024

Choose a reason for hiding this comment

ldiego08 Jun 27, 2024 • edited Loading

Choose a reason for hiding this comment

obycode Jul 17, 2024

Choose a reason for hiding this comment

obycode Jun 25, 2024

Choose a reason for hiding this comment

obycode Jun 25, 2024

Choose a reason for hiding this comment

ldiego08 commented May 21, 2024 •

edited

Loading

codecov bot commented May 22, 2024 •

edited

Loading

CLAassistant commented Jun 24, 2024 •

edited

Loading

ldiego08 Jun 27, 2024 •

edited

Loading