-
Notifications
You must be signed in to change notification settings - Fork 699
feat: use clarity-serialization
in clarity
#6310
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: develop
Are you sure you want to change the base?
Changes from 22 commits
83ae404
96dddb8
59d9bf2
cfa3cf6
04a9323
3eabeb4
1f1c190
7848406
738bea8
526cec1
1944b20
47cfbfe
9475370
dd8ad84
228bdee
05931c8
5462de1
28036bc
9660ae2
1c05004
4ba0a36
355d0ef
40a95ee
b0957e3
fa6d7d7
aef2fe9
a1090c4
454d7e0
15ae223
7b42ca5
9eabf67
69a1941
abccbe9
4a35628
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -16,10 +16,12 @@ | |
|
||
use std::{error, fmt}; | ||
|
||
use clarity_serialization::errors::CodecError; | ||
use clarity_serialization::types::{TraitIdentifier, TupleTypeSignature, TypeSignature, Value}; | ||
|
||
use crate::vm::costs::{CostErrors, ExecutionCost}; | ||
use crate::vm::diagnostic::{DiagnosableError, Diagnostic}; | ||
use crate::vm::representations::SymbolicExpression; | ||
use crate::vm::types::{TraitIdentifier, TupleTypeSignature, TypeSignature, Value}; | ||
|
||
pub type CheckResult<T> = Result<T, CheckError>; | ||
|
||
|
@@ -493,3 +495,52 @@ impl DiagnosableError for CheckErrors { | |
} | ||
} | ||
} | ||
|
||
impl From<CodecError> for CheckErrors { | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Instead of doing this, why not just create a single There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Actually, nevermind -- see https://github.com/stacks-network/stacks-core/pull/6310/files#r2252293332 There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. CodecError dropped in 40a95ee |
||
fn from(err: CodecError) -> Self { | ||
match err { | ||
CodecError::ValueTooLarge => CheckErrors::ValueTooLarge, | ||
CodecError::ValueOutOfBounds => CheckErrors::ValueOutOfBounds, | ||
CodecError::TypeSignatureTooDeep => CheckErrors::TypeSignatureTooDeep, | ||
CodecError::SupertypeTooLarge => CheckErrors::SupertypeTooLarge, | ||
CodecError::EmptyTuplesNotAllowed => CheckErrors::EmptyTuplesNotAllowed, | ||
CodecError::ListTypesMustMatch => CheckErrors::ListTypesMustMatch, | ||
CodecError::CouldNotDetermineType => CheckErrors::CouldNotDetermineType, | ||
CodecError::CouldNotDetermineSerializationType => { | ||
CheckErrors::CouldNotDetermineSerializationType | ||
} | ||
CodecError::InvalidStringCharacters => CheckErrors::InvalidCharactersDetected, | ||
CodecError::InvalidUtf8Encoding => CheckErrors::InvalidUTF8Encoding, | ||
CodecError::NoSuchTupleField(name, sig) => CheckErrors::NoSuchTupleField(name, sig), | ||
CodecError::TypeError { expected, found } => CheckErrors::TypeError(*expected, *found), | ||
CodecError::TypeValueError { expected, found } => { | ||
CheckErrors::TypeValueError(*expected, *found) | ||
} | ||
CodecError::Expect(s) => CheckErrors::Expects(s), | ||
// These errors don't have a match in CheckErrors, so we convert | ||
// them to a descriptive string inside the `Expects` variant. | ||
// Based on the current code, this should never happen. | ||
CodecError::Io(_) | ||
| CodecError::Serialization(_) | ||
| CodecError::Deserialization(_) | ||
| CodecError::DeserializeExpected(_) | ||
| CodecError::UnexpectedSerialization | ||
| CodecError::LeftoverBytesInDeserialization | ||
| CodecError::ParseError(_) | ||
| CodecError::BadTypeConstruction | ||
| CodecError::FailureConstructingTupleWithType | ||
| CodecError::FailureConstructingListWithType | ||
| CodecError::NameAlreadyUsedInTuple(_) | ||
| CodecError::InvalidClarityName(_, _) | ||
| CodecError::InvalidContractName(_, _) => { | ||
CheckErrors::Expects(format!("Unexpected error: {err:?}")) | ||
} | ||
} | ||
} | ||
} | ||
|
||
impl From<CodecError> for CheckError { | ||
fn from(err: CodecError) -> Self { | ||
CheckError::new(err.into()) | ||
} | ||
} |
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -58,9 +58,8 @@ fn check_special_list_cons( | |
type_arg.type_size()?, | ||
)?; | ||
} | ||
TypeSignature::parent_list_type(&typed_args) | ||
.map_err(|x| x.into()) | ||
.map(TypeSignature::from) | ||
let list_type = TypeSignature::parent_list_type(&typed_args).map_err(CheckError::from)?; | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Please revert this change. You should assume that all error types are consensus-critical, along with their valid conversations. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. done in 40a95ee |
||
Ok(TypeSignature::from(list_type)) | ||
} | ||
|
||
fn check_special_print( | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -65,14 +65,17 @@ fn check_special_list_cons( | |
for arg in args.iter() { | ||
// don't use map here, since type_check has side-effects. | ||
let checked = checker.type_check(arg, context)?; | ||
let cost = checked.type_size().and_then(|ty_size| { | ||
checker | ||
.compute_cost( | ||
ClarityCostFunction::AnalysisListItemsCheck, | ||
&[ty_size.into()], | ||
) | ||
.map_err(CheckErrors::from) | ||
}); | ||
let cost = checked | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Please revert this and use the original error conversions. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. done in 40a95ee |
||
.type_size() | ||
.map_err(CheckErrors::from) | ||
.and_then(|ty_size| { | ||
checker | ||
.compute_cost( | ||
ClarityCostFunction::AnalysisListItemsCheck, | ||
&[ty_size.into()], | ||
) | ||
.map_err(CheckErrors::from) | ||
}); | ||
costs.push(cost); | ||
|
||
if let Some(cur_size) = entries_size { | ||
|
Uh oh!
There was an error while loading. Please reload this page.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Okay, so, the obnoxious thing about the way we've structured this series of PRs is that the stakes are much, much higher on this specific PR, whereas lots of potential bugs are present in the previous #6297 PR that this PR depends on. #6297 was merged due to expediency more than correctness since (1) it didn't affect any production code paths and (2) it was supposedly, ostensibly, release-blockingly important that
clarity-serialization
be a crate in Stacks Core as part of the last release instead of when it could be given due consideration for how it would impact consensus in the future. This has now made my job of reviewing this PR and your job of acting on feedback a lot more difficult than it needed to be.Here are some correctness-related bugs introduced in #6297 that will need to be fixed in this PR.
in
src/types/mod.rs
, the functionStandardPrincipalData::new()
now returns aCodecError
instead of anInterpreterError
. This is a consensus bug, because this PR treatsCodecError
as aCheckErrors
instead of anInterpreterError
. A transaction can be mined if it encountered aCheckErrors
, but it cannot be mined if it encountered anInterpreterError
.StandardPrincipalData::new()
will need to be modified to return anInterpreterError
as it did before.The above goes for
QualifiedContractIdentifier::local()
andQualifiedContractIdentifier::parse()
. These need to continue to returnInterpreterError
, notCodecError
(which this PR really treats as aCheckErrors
).The above goes for
TraitIdentifier::parse_fully_qualified()
,TraitIdentifier::parse_sugared_syntax()
, andTraitIdentifier::parse()
-- these need to returnInterpreterError
.There are more (many) places where you've replaced an existing error type with
CodecError
in #6297. You will need to revert all of these changes. You should instead consolidate every error type, including their associatedFrom
,TryFrom
,Display
, etc. implementations, in theclarity
create exactly as they are defined intoclarity-serialization
. You should also dropCodecError
entirely -- it was not needed before, so it will not be needed post-refactoring.