Skip to content

Conversation

Jiloc
Copy link
Contributor

@Jiloc Jiloc commented Sep 17, 2025

Description

Rename:

  • InterpreterError to VmInternalError
  • InterpreterError::InterpreterError to VmInternalError::InvariantViolation

WARNING: This is a breaking change for consumers, like the sbtc crate.

Applicable issues

@Jiloc Jiloc self-assigned this Sep 17, 2025
@Jiloc Jiloc requested review from a team as code owners September 17, 2025 13:39
@Jiloc Jiloc marked this pull request as draft September 17, 2025 13:40
@Jiloc Jiloc modified the milestones: 3.2.0.0.2, 3.3.0.0.0 Sep 17, 2025
@Jiloc Jiloc added chore Necessary but less impactful tasks such as cleanup or reorg aac-api-breaking Avoiding accidental consensus api breaking change labels Sep 17, 2025
@Jiloc Jiloc moved this to Status: 💻 In Progress in Stacks Core Eng Sep 17, 2025
/// did not match the expected type signature.
FailureConstructingListWithType,
/// An STX transfer failed due to insufficient balance.
InsufficientBalance,
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Is this truly an unexpected error? Does it imply that the calling code should sanitize inputs first, and that this error should never actually be raised?

Copy link
Contributor

Choose a reason for hiding this comment

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

This appears to be used as “the last stand.”.

Looking at STXBalanceSnapshot::transfer_to(...) (invoked in Environment::stx_transfer_consolidated(..)), it attempts to check the amount immediately before performing the transfer.

Copy link
Member

Choose a reason for hiding this comment

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

Exactly -- it's the final check to prevent balance underflows. Balance underflows are checked at several places up the stack, so if this error is needed then Bad Things have already happened in the VM.

@Jiloc Jiloc moved this from Status: 💻 In Progress to Status: In Review in Stacks Core Eng Sep 18, 2025
@Jiloc Jiloc marked this pull request as ready for review September 18, 2025 07:51
Copy link
Contributor

@fdefelici fdefelici left a comment

Choose a reason for hiding this comment

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

LGMT!

A conflict need to be addressed

jferrant
jferrant previously approved these changes Sep 19, 2025
Copy link
Contributor

@jferrant jferrant left a comment

Choose a reason for hiding this comment

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

LGTM. Sorry about the conflicts. :/

Copy link
Contributor

@fdefelici fdefelici left a comment

Choose a reason for hiding this comment

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

CI Format step is failing

/// A generic, unexpected internal error, indicating a logic failure within
/// the VM.
/// The `String` provides a message describing the specific failure.
InvariantViolation(String), // TODO: merge with VmInternalError::Expect
Copy link
Member

Choose a reason for hiding this comment

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

Is there an issue for this TODO?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

good point! I just opened it #6530

@jcnelson
Copy link
Member

LGTM; please address my one comment and I'll approve 👍

fdefelici
fdefelici previously approved these changes Sep 23, 2025
@jferrant
Copy link
Contributor

Replaced by #6497

@jferrant jferrant closed this Sep 24, 2025
@github-project-automation github-project-automation bot moved this from Status: In Review to Status: ✅ Done in Stacks Core Eng Sep 24, 2025
Copy link

codecov bot commented Sep 24, 2025

Codecov Report

❌ Patch coverage is 51.51515% with 160 lines in your changes missing coverage. Please review.
✅ Project coverage is 78.81%. Comparing base (927747c) to head (b20d367).
⚠️ Report is 8 commits behind head on aac-client-breaking.

Files with missing lines Patch % Lines
clarity/src/vm/database/structures.rs 15.21% 39 Missing ⚠️
clarity/src/vm/database/clarity_db.rs 29.78% 33 Missing ⚠️
stackslib/src/clarity_vm/database/marf.rs 38.23% 21 Missing ⚠️
clarity/src/vm/database/key_value_wrapper.rs 29.41% 12 Missing ⚠️
clarity/src/vm/contexts.rs 40.00% 9 Missing ⚠️
stackslib/src/clarity_vm/database/ephemeral.rs 11.11% 8 Missing ⚠️
clarity/src/vm/database/sqlite.rs 62.50% 6 Missing ⚠️
clarity/src/vm/mod.rs 45.45% 6 Missing ⚠️
clarity/src/vm/functions/principals.rs 37.50% 5 Missing ⚠️
clarity-types/src/errors/mod.rs 66.66% 3 Missing ⚠️
... and 12 more

❌ Your project status has failed because the head coverage (78.81%) 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                   @@
##           aac-client-breaking    #6500      +/-   ##
=======================================================
- Coverage                79.88%   78.81%   -1.07%     
=======================================================
  Files                      565      565              
  Lines                   346594   346578      -16     
=======================================================
- Hits                    276882   273162    -3720     
- Misses                   69712    73416    +3704     
Files with missing lines Coverage Δ
clarity-types/src/tests/types/mod.rs 96.86% <100.00%> (ø)
clarity-types/src/tests/types/serialization.rs 98.31% <100.00%> (ø)
clarity-types/src/types/serialization.rs 92.05% <100.00%> (ø)
clarity/src/vm/callables.rs 96.32% <100.00%> (ø)
clarity/src/vm/functions/crypto.rs 84.84% <100.00%> (ø)
clarity-types/src/types/mod.rs 95.84% <96.87%> (ø)
clarity/src/vm/database/clarity_store.rs 40.00% <75.00%> (ø)
clarity/src/vm/functions/mod.rs 97.34% <50.00%> (ø)
clarity/src/vm/functions/options.rs 91.19% <0.00%> (ø)
clarity/src/vm/functions/tuples.rs 80.39% <0.00%> (ø)
... and 17 more

... and 87 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 927747c...b20d367. 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

github-actions bot commented Oct 2, 2025

This pull request has been automatically locked since there has not been any recent activity after it was closed. Please open a new issue for related bugs.

@github-actions github-actions bot added the locked label Oct 2, 2025
@github-actions github-actions bot locked as resolved and limited conversation to collaborators Oct 2, 2025
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
aac-api-breaking Avoiding accidental consensus api breaking change chore Necessary but less impactful tasks such as cleanup or reorg locked
Projects
Status: Status: ✅ Done
Development

Successfully merging this pull request may close these issues.

Rename and document clarity::vm::errors::InterpreterError to VmInternalError
4 participants