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

feat(Clarity2): Parse and assemble principals #2756

Merged
merged 127 commits into from
May 5, 2022
Merged

Conversation

gregorycoppola
Copy link
Contributor

@gregorycoppola gregorycoppola commented Jul 1, 2021

Fixes #2620

Description

We add two new Clarity2 functions "parse-principal" and "assemble-principal". assemble-principal allows a principal representation to be created from its version and pub_key_hash constituents. parse-principal works in the other direction, parsing a principal representation into its constituent parts.

EDIT(@jcnelson): The functions are now called principal-parse and principal-construct. They allow the caller to both construct standard and contract principals from (buff 1) (buff 20) and (buff 1) (buff 20) (string-ascii 40), respectively, and to parse principal values into values with type { version: (buff 1), hash-bytes: (buff 20), name: (optional (string-ascii 40)) }. They both return response types, and permit the user to parse and construct principals with version bytes that are not currently supported by the consensus rules but are otherwise well-formed by representing them in the (err ..) variant.

Testing

See:

M	src/vm/analysis/type_checker/tests/mod.rs
M	src/vm/tests/principals.rs

@project-bot project-bot bot added this to Review in progress in Stacks Blockchain Board Jul 1, 2021
@pavitthrap
Copy link
Contributor

I think maybe you need to switch the branch to merge into from master to something else - there's a lot of unrelated commits right now

@gregorycoppola gregorycoppola changed the base branch from master to next July 6, 2021 16:14
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.

Just a few minor comments!

src/vm/analysis/errors.rs Outdated Show resolved Hide resolved
src/vm/docs/mod.rs Outdated Show resolved Hide resolved
src/vm/functions/mod.rs Outdated Show resolved Hide resolved
src/vm/functions/principals.rs Outdated Show resolved Hide resolved
src/vm/functions/principals.rs Outdated Show resolved Hide resolved
src/vm/functions/principals.rs Outdated Show resolved Hide resolved
src/vm/tests/costs.rs Outdated Show resolved Hide resolved
src/vm/tests/principals.rs Outdated Show resolved Hide resolved
Copy link
Contributor Author

@gregorycoppola gregorycoppola left a comment

Choose a reason for hiding this comment

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

Thanks for the comments!

src/vm/tests/costs.rs Outdated Show resolved Hide resolved
src/vm/analysis/errors.rs Outdated Show resolved Hide resolved
src/vm/functions/principals.rs Outdated Show resolved Hide resolved
src/vm/functions/principals.rs Outdated Show resolved Hide resolved
src/vm/functions/mod.rs Outdated Show resolved Hide resolved
src/vm/functions/principals.rs Outdated Show resolved Hide resolved
src/vm/tests/principals.rs Outdated Show resolved Hide resolved
src/vm/docs/mod.rs Outdated Show resolved Hide resolved
…cy src/vm, and expand them to cover parsing and constructing contract principals
…to build the tuples returned by principal-parse
@jcnelson
Copy link
Member

I'm taking over this PR.

@codecov
Copy link

codecov bot commented Apr 28, 2022

Codecov Report

Merging #2756 (19e7553) into next (2747901) will increase coverage by 0.06%.
The diff coverage is 82.13%.

@@            Coverage Diff             @@
##             next    #2756      +/-   ##
==========================================
+ Coverage   83.75%   83.82%   +0.06%     
==========================================
  Files         265      267       +2     
  Lines      207954   209960    +2006     
==========================================
+ Hits       174166   175993    +1827     
- Misses      33788    33967     +179     
Impacted Files Coverage Δ
clarity/src/vm/analysis/read_only_checker/mod.rs 86.44% <ø> (ø)
clarity/src/vm/analysis/type_checker/mod.rs 93.69% <ø> (ø)
clarity/src/vm/database/key_value_wrapper.rs 96.51% <ø> (ø)
clarity/src/vm/errors.rs 82.22% <ø> (ø)
clarity/src/vm/test_util/mod.rs 61.71% <0.00%> (-5.81%) ⬇️
clarity/src/vm/tests/contracts.rs 98.04% <ø> (ø)
clarity/src/vm/tests/mod.rs 100.00% <ø> (ø)
src/chainstate/burn/db/sortdb.rs 95.21% <ø> (-0.02%) ⬇️
src/chainstate/burn/mod.rs 97.54% <ø> (ø)
src/chainstate/burn/sortition.rs 89.86% <0.00%> (-5.97%) ⬇️
... and 68 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 cc07c63...19e7553. Read the comment docs.

@jcnelson
Copy link
Member

This PR is now ready for review @kantai @obycode @lgalabru @gregorycoppola @pavitthrap

clarity/src/vm/docs/mod.rs Outdated Show resolved Hide resolved
clarity/src/vm/types/mod.rs Outdated Show resolved Hide resolved
Copy link
Member

@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 looks great to me, just had some feedback on the principal-parse function name, and some pretty superficial comments otherwise.

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.

Just a few nits, agree with Aaron's code suggestions too

clarity/src/vm/docs/mod.rs Outdated Show resolved Hide resolved
clarity/src/vm/types/signatures.rs Outdated Show resolved Hide resolved
clarity/src/vm/types/signatures.rs Outdated Show resolved Hide resolved
clarity/src/vm/functions/principals.rs Outdated Show resolved Hide resolved
clarity/src/vm/functions/principals.rs Outdated Show resolved Hide resolved
clarity/src/vm/tests/principals.rs Outdated Show resolved Hide resolved
…ors, adding an enum for principal-* error codes, renaming principal-parse to principal-destruct, better usage of Value:: helper functions)
@jcnelson jcnelson requested a review from kantai May 1, 2022 00:29
@@ -382,7 +391,8 @@ impl DiagnosableError for CheckErrors {
CheckErrors::MaxContextDepthReached => format!("reached depth limit"),
CheckErrors::UndefinedVariable(var_name) => format!("use of unresolved variable '{}'", var_name),
CheckErrors::UndefinedFunction(var_name) => format!("use of unresolved function '{}'", var_name),
CheckErrors::RequiresAtLeastArguments(expected, found) => format!("expecting >= {} argument, got {}", expected, found),
CheckErrors::RequiresAtLeastArguments(expected, found) => format!("expecting >= {} arguments, got {}", expected, found),
CheckErrors::RequiresAtMostArguments(expected, found) => format!("expecting < {} arguments, got {}", expected, found),
Copy link
Contributor

Choose a reason for hiding this comment

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

This should say "expecting <=".

Stacks Blockchain Board automation moved this from Review in progress to Reviewer approved May 5, 2022
@jcnelson jcnelson merged commit cef2f5d into next May 5, 2022
Stacks Blockchain Board automation moved this from Reviewer approved to Done May 5, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
Development

Successfully merging this pull request may close these issues.

Converting and querying valid principal representations
8 participants