- 
                Notifications
    You must be signed in to change notification settings 
- Fork 0
Suggestions & updates #3
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: main
Are you sure you want to change the base?
Conversation
This was removed from CLI in 8.0(?)
The assumptions are logical, but not needed. Rules pass without them: https://prover.certora.com/output/7749274/46268bb1c02c41f4b86527108f9d2e1d?anonymousKey=c1e61c81e55a93cc3f7ea965a2e7996c264dbdd1 https://prover.certora.com/output/7749274/a02f870d1c4b429ca144f761e9e39020?anonymousKey=6adb620b99e67df55335533f344b4dd13a8b04e1 ... so, better to generalize and simplify, I think.
Jobs run ok without them, so seems better to hide these details.
        
          
                docs/source/token/mutations.rst
              
                Outdated
          
        
      | Note that there are other ways to assess the quality of your rule. | ||
| You can mutate the rule to see if it is vacuous, you can check if the rule is a | ||
| tautology, and you can use UNSAT cores to understand what parts of the code were | ||
| covered by the rule. | 
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.
Do we have resources we can link to for these approaches?
If not, I think we can leave it out since it doesn't stand alone very well.
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.
Vacuity: https://docs.certora.com/en/latest/docs/prover/checking/sanity.html#vacuity-checks
Tautologies:
https://docs.certora.com/en/latest/docs/prover/checking/sanity.html#assert-tautology-checks
UNSAT cores: we don't have a link for this, should probably be dropped.
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.
Updated - thanks!
| ], | ||
| "precise_bitwise_ops": true, | ||
| "prover_version": "master", | ||
| "server": "production" | 
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.
Any reason to keep prover_version and server in the confs?
My tests with 8.1.1 worked ok without them, so seems better to simplify.
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.
yes no good reason IMO
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.
agreed! sorry this was my bad. Thanks for fixing.
| #[rule] | ||
| fn transfer_is_correct(e: Env, to: Address, from: Address, amount: i64) { | ||
| cvlr_assume!( | ||
| e.storage().persistent().has(&from) && e.storage().persistent().has(&to) && to != from); | 
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.
It makes logical sense to require these addresses to exist, but the rule passes without this:
https://prover.certora.com/output/7749274/46268bb1c02c41f4b86527108f9d2e1d?anonymousKey=c1e61c81e55a93cc3f7ea965a2e7996c264dbdd1
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.
you can remove them! I think i was just trying to make something to show the usage of assume but might have been too contrived because at the time I didn't have as many examples.
| e.storage().persistent().has(&from) | ||
| && e.storage().persistent().has(&to) | ||
| && to != from | ||
| && Token::balance(&e, from.clone()) < amount); | 
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 rule passes without these:
https://prover.certora.com/output/7749274/a02f870d1c4b429ca144f761e9e39020?anonymousKey=6adb620b99e67df55335533f344b4dd13a8b04e1
So, better to simplify and generalize the rule(?)
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.
agreed, remove it!
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.
Minor nits. Please fix and wait for Chandra's review before merging.
        
          
                docs/source/token/mutations.rst
              
                Outdated
          
        
      | Note that there are other ways to assess the quality of your rule. | ||
| You can mutate the rule to see if it is vacuous, you can check if the rule is a | ||
| tautology, and you can use UNSAT cores to understand what parts of the code were | ||
| covered by the rule. | 
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.
Vacuity: https://docs.certora.com/en/latest/docs/prover/checking/sanity.html#vacuity-checks
Tautologies:
https://docs.certora.com/en/latest/docs/prover/checking/sanity.html#assert-tautology-checks
UNSAT cores: we don't have a link for this, should probably be dropped.
| .. important:: | ||
|  | ||
| If you are not able to run certoraRun, see the `Sunbeam Troubleshooting`_ section in | ||
| If you are not able to run ``certoraSorobanProver``, see the `Sunbeam Troubleshooting`_ section in | 
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.
good catch
| ], | ||
| "precise_bitwise_ops": true, | ||
| "prover_version": "master", | ||
| "server": "production" | 
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.
yes no good reason IMO
Fixes from Uri Co-authored-by: urikirsh <[email protected]>
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.
Sorry for the late review. Looks great to me. I think you can remove the assumes.
| ], | ||
| "precise_bitwise_ops": true, | ||
| "prover_version": "master", | ||
| "server": "production" | 
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.
agreed! sorry this was my bad. Thanks for fixing.
| #[rule] | ||
| fn transfer_is_correct(e: Env, to: Address, from: Address, amount: i64) { | ||
| cvlr_assume!( | ||
| e.storage().persistent().has(&from) && e.storage().persistent().has(&to) && to != from); | 
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.
you can remove them! I think i was just trying to make something to show the usage of assume but might have been too contrived because at the time I didn't have as many examples.
| e.storage().persistent().has(&from) | ||
| && e.storage().persistent().has(&to) | ||
| && to != from | ||
| && Token::balance(&e, from.clone()) < amount); | 
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.
agreed, remove it!
No description provided.