-
Notifications
You must be signed in to change notification settings - Fork 715
feat: static cost analysis #6704
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?
feat: static cost analysis #6704
Conversation
|
|
17ff3bf to
3560f55
Compare
ae9f7bd to
5eeafae
Compare
Codecov Report❌ Patch coverage is ❌ Your project check has failed because the head coverage (58.07%) 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 #6704 +/- ##
============================================
- Coverage 75.65% 58.07% -17.59%
============================================
Files 577 581 +4
Lines 357690 358952 +1262
============================================
- Hits 270624 208446 -62178
- Misses 87066 150506 +63440
... and 403 files with indirect coverage changes Continue to review full report in Codecov by Sentry.
🚀 New features to boost your workflow:
|
| } | ||
|
|
||
| /// NativeFunctions -> cost via appropriate cost fn | ||
| pub(crate) fn get_cost_function_for_native( |
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.
Could you refactor a bit and use lookup_reserved_functions here? We could probably do some refactoring in the clarity crate to make this easier to use and to ensure that we use the cost functions correctly for those functions that are SpecialFunctions and not NativeFunctions. I think there will be some discrepancies with the way those costs are computed. For example, let passes the length of the bindings list into its cost function, not the length of its total args.
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.
I updated to use this 👌 . It cleaned up a lot. It did indeed change some costs (len, concat, etc)
| let cost = match clarity_version { | ||
| ClarityVersion::Clarity1 => cost_function.eval::<Costs1>(arg_count), | ||
| ClarityVersion::Clarity2 => cost_function.eval::<Costs2>(arg_count), | ||
| ClarityVersion::Clarity3 => cost_function.eval::<Costs3>(arg_count), | ||
| ClarityVersion::Clarity4 => cost_function.eval::<Costs4>(arg_count), | ||
| } |
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.
The cost contract to use will actually change with the epoch, not with the Clarity version. You may want to pass an epoch in here and use that to select the cost function, or it might make sense to just always default to the latest.
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.
👌 Passed epoch as well. Ended up defaulting Epoch1.0 to use costs1 since it doesn't have a direct mapping (I think)
Allow measuring static cost statically for contracts.