Skip to content
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

Implement consensus de/serialization in Clarity #3132

Merged
merged 5 commits into from
May 19, 2022
Merged

Conversation

kantai
Copy link
Member

@kantai kantai commented May 10, 2022

Description

This PR adds to-consensus-buff and from-consensus-buff methods to the Clarity2 native methods.

Applicable issues

Additional info (benefits, drawbacks, caveats)

These methods are both fallible -- this may be surprising in the case of to-consensus-buff, but the reason is that some legal Clarity values serialize into buffers that are larger than the maximum Clarity value size; this is because of the type prefix and length encoding.

The typing of to-consensus-buff produces a buffer size which is the maximum possible buffer size for its input. In the case of uints, ints, and bools this is always correct. For other types, however, it will be an overestimate in many cases (e.g., for principal, it assumes the principal is a contract with the longest possible Clarity name).

@jcnelson jcnelson self-requested a review May 10, 2022 18:49
@codecov
Copy link

codecov bot commented May 10, 2022

Codecov Report

Merging #3132 (d8f7628) into next (e7f3c55) will decrease coverage by 0.01%.
The diff coverage is 97.43%.

@@            Coverage Diff             @@
##             next    #3132      +/-   ##
==========================================
- Coverage   83.64%   83.63%   -0.02%     
==========================================
  Files         267      268       +1     
  Lines      209188   209655     +467     
==========================================
+ Hits       174981   175338     +357     
- Misses      34207    34317     +110     
Impacted Files Coverage Δ
clarity/src/vm/analysis/arithmetic_checker/mod.rs 92.70% <ø> (ø)
clarity/src/vm/analysis/errors.rs 70.33% <0.00%> (-0.34%) ⬇️
clarity/src/vm/functions/conversions.rs 81.63% <92.85%> (+2.64%) ⬆️
clarity/src/vm/types/serialization.rs 94.58% <94.59%> (-0.01%) ⬇️
clarity/src/vm/analysis/type_checker/tests/mod.rs 98.92% <98.40%> (-0.03%) ⬇️
clarity/src/vm/tests/simple_apply_eval.rs 99.53% <99.41%> (-0.02%) ⬇️
clarity/src/vm/analysis/read_only_checker/mod.rs 86.57% <100.00%> (+0.12%) ⬆️
...rc/vm/analysis/type_checker/natives/conversions.rs 100.00% <100.00%> (ø)
...larity/src/vm/analysis/type_checker/natives/mod.rs 96.66% <100.00%> (+0.03%) ⬆️
clarity/src/vm/docs/mod.rs 88.29% <100.00%> (+0.04%) ⬆️
... and 33 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update f38a266...d8f7628. Read the comment docs.

@kantai kantai changed the title Feat/to from buff Implement consensus de/serialization in Clarity May 10, 2022
impl TypeSignature {
/// Return the maximum length of the consensus serialization of a
/// Clarity value of this type. The returned length *may* not fit
/// in in a Clarity buffer! For example, the maximum serialized
Copy link
Member

Choose a reason for hiding this comment

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

typo -- "in in a"

Copy link
Member Author

Choose a reason for hiding this comment

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

Removed in d8f7628

Copy link
Member

@jcnelson jcnelson left a comment

Choose a reason for hiding this comment

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

Overall LGTM. Just had a few superficial / edification comments, with a request for a (trivial) additional bit of test coverage. Thanks for whipping this up!

@MarvinJanssen do you want to do a pass on this before merging? Let us know.

Copy link
Member

@MarvinJanssen MarvinJanssen left a comment

Choose a reason for hiding this comment

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

Amazing, I was working on a to-consensus-buff for the REPL but this code is a lot more elegant. Can easily it be ported to the REPL (and thus Clarinet) before 2.1 launches?

@obycode
Copy link
Contributor

obycode commented May 12, 2022

Can easily it be ported to the REPL (and thus Clarinet) before 2.1 launches?

Migrating the REPL to use this canonical clarity lib is high on my todo list. Once that is done, we would enable a 2.1 option for testing before 2.1 is launched.

Copy link
Contributor

@pavitthrap pavitthrap left a comment

Choose a reason for hiding this comment

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

lgtm

clarity/src/vm/functions/conversions.rs Show resolved Hide resolved
clarity/src/vm/tests/simple_apply_eval.rs Outdated Show resolved Hide resolved
@gregorycoppola gregorycoppola removed their request for review May 17, 2022 15:35
Copy link
Member

@jcnelson jcnelson left a comment

Choose a reason for hiding this comment

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

Cool, looks like all my comments were addressed 👍

@blockstack-devops
Copy link
Contributor

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.

@stacks-network stacks-network locked as resolved and limited conversation to collaborators Nov 18, 2024
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants