Skip to content

Conversation

fdefelici
Copy link
Contributor

@fdefelici fdefelici commented Oct 10, 2025

Description

This PR does 2 things:

  • add StacksTransactionReceipt::vm_error to the ExpectedTransactionOutput
  • try to override ExpectedTransactionOutput::vm_error for the snapshot appending a NON-CONSENSUS BREAKING info message. Basically we would like something like this:
       ExpectedTransactionOutput(
          ...,
          vm_error: Some("whatever"),   //NON-CONSENSUS BREAKING
       ),   
    

With the current solution, based on serde, we can only achieve this:

     ExpectedTransactionOutput(
        ...,
        vm_error: "whatever  //NON-CONSENSUS BREAKING",
     ),   

Basically because they are json fields we are not allowed to add text outside the field.

Here I used a "static" approach so applying the trasformation directly to the ExpectedTransactionOutput definition, but we will have same result using the "dynamic" reduction approach done inside the test execution (because rely on serde/json).

Alternative Approach
I also find a way to do what we want but requires to implement the Debug trait doing all the snaphoshot formatting "manually". Here a short example:

#[derive(Serialize)]
struct MyStruct {
    id: u32,
    name: Option<String>,
}

impl fmt::Debug for MyStruct {
    fn fmt(&self, f: &mut fmt::Formatter<'_>) -> fmt::Result {
        writeln!(f, "MyStruct(")?;
        writeln!(f, "    id: {},", self.id)?;
        match &self.name {
            Some(n) => writeln!(f, "    name: Some({:?}), // REVIEWED", n)?,
            None => writeln!(f, "    name: None, // REVIEWED")?,
        }
        write!(f, ")")
    }
}

#[test]
fn test_debug() {
    let data_some = MyStruct {
        id: 20,
        name: Some("Alice".to_string()),
    };

    insta::assert_debug_snapshot!(data_some);
}

This will produce the following snapshot:

1 │+MyStruct(
2 │+    id: 20,
3 │+    name: Some("Alice"), // REVIEWED
4 │+)

So, for sure this is nice and give us the best flexibility (also for future formatting requirements), but we have to take into account for the whole struct formatting by ourselves.

What do you think?

Applicable issues

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

@fdefelici fdefelici self-assigned this Oct 10, 2025
@fdefelici fdefelici added the aac Avoiding Accidental Consensus label Oct 10, 2025
@fdefelici fdefelici moved this to Status: 💻 In Progress in Stacks Core Eng Oct 10, 2025
@fdefelici fdefelici requested review from Jiloc and jferrant October 10, 2025 10:06
Copy link
Contributor

@Jiloc Jiloc left a comment

Choose a reason for hiding this comment

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

I am ok with the serde approach, the message is clear, i am not concerned with the fact that it's included in the string itself. (note that there is a rust fmt error)

@jferrant
Copy link
Contributor

Hm I have mixed feelings. its annoying to custom write the entire debug struct, but I think the fields are pretty stable so once its written, it won't be so bad and I think its better to be explicit and then we don't actually modify the original struct. So I think I actually prefer the alternative approach if you don't mind XD but its rare that I have strong opinions and even when I do...they are loosely held so if you choose to ignore me, I will approve regardless.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

aac Avoiding Accidental Consensus

Projects

Status: Status: 💻 In Progress

Development

Successfully merging this pull request may close these issues.

AAC: Add tx_result and success or failure to the expected output in the test harness

3 participants