Skip to content

Conversation

@Jiloc
Copy link
Contributor

@Jiloc Jiloc commented Jul 21, 2025

Description

Points for Discussion from #6297:

  • Error handling: In clarity-serialization, I consolidated all error types into a single CodecError enum with a 1:1 match with the original errors in clarity. However, when converting back to the older error types using From<CodecError>, some variants don’t have a clear or meaningful equivalent. I’ve routed them through generic catch-all cases. Even if the current code doesn't reach those conversions, I am not happy with this mapping strategy and am open to suggestions!
  • Retro-compatibility of ClarityName and ContractName: The guarded_string! macro for these types now returns the new CodecError instead of a RuntimeErrorType.
  • Feel free to throw any other topic you think would be worth discussing!

Applicable issues

  • fixes #

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

@Jiloc Jiloc added this to the 3.2.0.0.0 milestone Jul 21, 2025
@Jiloc Jiloc self-assigned this Jul 21, 2025
@Jiloc Jiloc requested review from a team as code owners July 21, 2025 14:50
@Jiloc Jiloc marked this pull request as draft July 21, 2025 14:51
@Jiloc Jiloc moved this to Status: 💻 In Progress in Stacks Core Eng Jul 21, 2025
@Jiloc Jiloc modified the milestones: 3.2.0.0.0, 3.2.0.0.1 Jul 21, 2025
@aldur aldur modified the milestones: 3.2.0.0.1, 3.2.0.0.2 Aug 20, 2025
@moodmosaic
Copy link
Contributor

We've opened a PR as a follow-up to expand input coverage further, in line with #6355.

Any feedback/thoughts?

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 LGTM, just one comment where there's a line that I think has slightly different semantics.

@Jiloc Jiloc requested a review from kantai September 8, 2025 14:33
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.

LGTM!

@Jiloc Jiloc enabled auto-merge September 9, 2025 06:33
@Jiloc Jiloc disabled auto-merge September 9, 2025 06:34
@Jiloc Jiloc enabled auto-merge September 9, 2025 07:44
@Jiloc Jiloc added this pull request to the merge queue Sep 9, 2025
@github-project-automation github-project-automation bot moved this from Status: In Review to Status: 💻 In Progress in Stacks Core Eng Sep 9, 2025
Merged via the queue into stacks-network:develop with commit 1393537 Sep 9, 2025
1 of 2 checks passed
@Jiloc Jiloc deleted the feat/clarity-depend-on-clarity-serialization branch September 9, 2025 08:23
@github-project-automation github-project-automation bot moved this from Status: 💻 In Progress to Status: ✅ Done in Stacks Core Eng Sep 9, 2025
@codecov
Copy link

codecov bot commented Sep 9, 2025

Codecov Report

❌ Patch coverage is 68.55415% with 572 lines in your changes missing coverage. Please review.
✅ Project coverage is 64.86%. Comparing base (30cd5e4) to head (acc851f).
⚠️ Report is 101 commits behind head on develop.

Files with missing lines Patch % Lines
clarity-serialization/src/errors/analysis.rs 70.86% 88 Missing ⚠️
clarity-serialization/src/tests/types/mod.rs 58.03% 81 Missing ⚠️
clarity-serialization/src/errors/ast.rs 52.11% 68 Missing ⚠️
clarity-serialization/src/representations.rs 78.27% 58 Missing ⚠️
clarity-serialization/src/token.rs 20.00% 48 Missing ⚠️
clarity-serialization/src/types/mod.rs 83.17% 35 Missing ⚠️
clarity/src/vm/types/signatures.rs 61.36% 34 Missing ⚠️
clarity-serialization/src/types/signatures.rs 55.38% 29 Missing ⚠️
clarity-serialization/src/errors/lexer.rs 15.15% 28 Missing ⚠️
clarity-serialization/src/types/serialization.rs 79.38% 27 Missing ⚠️
... and 8 more

❌ Your project status has failed because the head coverage (64.86%) is below the target coverage (80.00%). You can increase the head coverage or adjust the target coverage.

❗ There is a different number of reports uploaded between BASE (30cd5e4) and HEAD (acc851f). Click for more details.

HEAD has 14 uploads less than BASE
Flag BASE (30cd5e4) HEAD (acc851f)
139 125
Additional details and impacted files
@@             Coverage Diff              @@
##           develop    #6310       +/-   ##
============================================
- Coverage    75.57%   64.86%   -10.71%     
============================================
  Files          555      555               
  Lines       344533   341181     -3352     
============================================
- Hits        260390   221323    -39067     
- Misses       84143   119858    +35715     
Files with missing lines Coverage Δ
clarity-serialization/src/lib.rs 100.00% <ø> (ø)
clarity-serialization/src/tests/representations.rs 75.00% <ø> (ø)
...larity-serialization/src/tests/types/signatures.rs 1.47% <100.00%> (-98.10%) ⬇️
clarity/src/vm/analysis/analysis_db.rs 92.45% <ø> (ø)
clarity/src/vm/analysis/arithmetic_checker/mod.rs 92.48% <ø> (ø)
clarity/src/vm/analysis/read_only_checker/mod.rs 85.65% <ø> (-1.74%) ⬇️
clarity/src/vm/analysis/type_checker/contexts.rs 95.06% <ø> (ø)
...ity/src/vm/analysis/type_checker/v2_05/contexts.rs 98.25% <ø> (ø)
clarity/src/vm/analysis/type_checker/v2_05/mod.rs 87.83% <ø> (ø)
.../src/vm/analysis/type_checker/v2_05/natives/mod.rs 89.17% <100.00%> (-0.02%) ⬇️
... and 53 more

... and 289 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 30cd5e4...acc851f. 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.

@github-actions
Copy link

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 locked as resolved and limited conversation to collaborators Sep 17, 2025
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Labels

Projects

Status: Status: ✅ Done

Development

Successfully merging this pull request may close these issues.

Refactor clarity to enable lightweight serialization crate

6 participants