Chore/add custom error to clarity types #6751
Conversation
…tioError Signed-off-by: Jacinta Ferrant <[email protected]>
… into chore/add-custom-error-to-clarity-types
… into chore/add-custom-error-to-clarity-types
…om/Jiloc/stacks-core into chore/add-custom-error-to-clarity-types
Codecov Report❌ Patch coverage is ❌ Your project check has failed because the head coverage (77.42%) 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 #6751 +/- ##
===========================================
- Coverage 77.90% 77.42% -0.48%
===========================================
Files 580 580
Lines 361187 361490 +303
===========================================
- Hits 281374 279877 -1497
- Misses 79813 81613 +1800
... and 91 files with indirect coverage changes Continue to review full report in Codecov by Sentry.
🚀 New features to boost your workflow:
|
jacinta-stacks
force-pushed
the
chore/add-custom-error-to-clarity-types
branch
5 times, most recently
from
826c4d7 to
9d27ff0
Compare
Signed-off-by: Jacinta Ferrant <[email protected]>
jacinta-stacks
force-pushed
the
chore/add-custom-error-to-clarity-types
branch
2 times, most recently
from
59cb8ff to
b8ca7dd
Compare
… into chore/add-custom-error-to-clarity-types
Signed-off-by: Jacinta Ferrant <[email protected]>
jacinta-stacks
requested review from
aaronb-stacks,
brice-stacks,
federico-stacks and
francesco-stacks
federico-stacks
left a comment
federico-stacks
left a comment
There was a problem hiding this comment.
Did a first pass. In the meantime, I’m sharing some thoughts
| /// Expected type does not match the actual type during analysis. | ||
| /// The first `Box<TypeSignature>` wraps the expected type, and the second wraps the actual type. | ||
| TypeMismatch(Box<TypeSignature>, Box<TypeSignature>), | ||
| /// Expected an different response type |
There was a problem hiding this comment.
nit
| /// Expected an different response type | |
| /// Expected a different response type |
| /// Empty tuple is not allowed in Clarity. | ||
| EmptyTuplesNotAllowed, | ||
| /// Supertype (e.g., trait or union) exceeds the maximum allowed size or complexity. | ||
| /// This error indicates a transaction would invalidate a block if included. |
There was a problem hiding this comment.
nit: block validation logic/comment should not be part of ClarityTypeError
| /// TODO: remove this comment. For code reviwers: this is only ever called in tests and immediately unwrapped | ||
| pub fn expect_bool(self) -> Result<bool, ClarityTypeError> { |
There was a problem hiding this comment.
It appears this is also used in production code by StacksChainState::is_pox_active(), which relies on expect(), so current change should be still safe.
|
|
||
| pub fn expect_callable(self) -> Result<CallableData, VmExecutionError> { | ||
| /// TODO: remove this comment. For reviwers: this is only called in tests and immediately unwrapped | ||
| pub fn expect_callable(self) -> Result<CallableData, ClarityTypeError> { |
There was a problem hiding this comment.
Should this be tagged with #[cfg(any(test, feature = "testing"))], or do we want to keep it as part of the reusable public API?
This comment apply to other TODO comment like this one.
| } | ||
|
|
||
| fn append( | ||
| pub fn append( |
There was a problem hiding this comment.
Not strictly necessary, but I’m fine with it if the goal is to improve the reusable public API.
| /// TODO: Do we want to make these return errors? I know in theory its infallible, but there is a lot of | ||
| /// in theory infallible that return errors instead of straight expects. |
There was a problem hiding this comment.
Good point! I don’t have a definitive answer. On one hand, I’d prefer to eliminate the expect calls; on the other hand, they do help break the "error chain".
| } | ||
|
|
||
| fn admits_type_v2_1(&self, other: &TypeSignature) -> Result<bool, CommonCheckErrorKind> { | ||
| fn admits_type_v2_1(&self, other: &TypeSignature) -> Result<bool, ClarityTypeError> { |
There was a problem hiding this comment.
Just sharing some reasoning on possibly changing admits_type_v2_0 and admits_type_v2_1 to return a plain bool instead of Result<bool, ClarityTypeError>:
- For both admits_type_v2_0 and admits_type_v2_1, it looks feasible to return only bool since they don’t meaningfully use the error channel.
- The only blocker comes from the top-level admits_type, which currently returns
StacksEpochId::Epoch10 => Err(ClarityTypeError::UnsupportedEpoch(*epoch))
If we could treat that case as StacksEpochId::Epoch10 => false instead, then all admits_type* methods could avoid returning ClarityTypeError entirely.
2 participants
I have added a bunch of comments that I will need to rip out before merging this. Mostly around where I am changing what was previously an "Expect" into a Nonexpect error. Mostly cause they prob shouldn't ever happen/I don't know why they would actually invalidate a block. I also added an ExpectAcceptable and ExpectRejectable type.
Feedback would be good.