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

assure function #1325

Closed
wants to merge 6 commits into from
Closed

Conversation

jacqueswww
Copy link
Contributor

What I did

Fixes #711.

How I did it

How to verify it

Play with the assure function.

Description for the changelog

Added assure function.

Cute Animal Picture

Put a link to a cute animal picture inside the parenthesis-->

@fubuloubu
Copy link
Member

fubuloubu commented Mar 5, 2019

Would like a few more examples showing the use of assure alongside assert (namely, assert should be used for pre-conditions, and assure for post-condition invariants that should never be violated)

It may even be useful to have a few assert post-conditions (non-invariant checks) to show that assure is sort of a "last resort" mechanism used in only specific use cases.

@jacqueswww
Copy link
Contributor Author

jacqueswww commented Mar 5, 2019

@fubuloubu I am not that keen that we have 2 different asserts, and I still don't think having two types is a good idea (even more so because we have reason string support -> e.g. assert 1 != 2, "never"). So I would rather not advertise the use case, people that will want to use it will know how in any case. We don't currently have a examples for assert, so I don't think we should have examples on assure either then - people just pick it up and go a long.
If you would like to add a good documented section on assertion I am keen to read it and merge it, but to be honest I am probably not the right person to write it up.

@jacqueswww
Copy link
Contributor Author

jacqueswww commented Mar 5, 2019

Having thought about it some more. Scratch most of what I said above. I am actually OK with having the function, especially because of the tool support (invalid being the de facto).
I also think having support for what these tools/analyzers offer are great.
@fubuloubou - I am still keen on you writing the examples though, as I think you will write it better than myself.

@fubuloubu
Copy link
Member

Okay, I assigned myself to add documentation on this feature and how to use it effectively. Let's keep #711 open until that is complete.

@jacqueswww
Copy link
Contributor Author

Cool, thanks :) it's been a while since I have thought about the analyzers, do we still have that model checker issue somewhere?

@fubuloubu
Copy link
Member

It would be fantastic to get someone to review this new feature.

cc @maurelian @wuestholz @japesinator @montyly could you aid us?

@wuestholz
Copy link

@fubuloubu @jacqueswww Great to see this being added! In general, any solution that allows to distinguish between parameter validation (require in Solidity or REVERT in EVM) and properties that should (in principle) always hold (assert in Solidity and INVALID in EVM) will be very useful to developers and tool builders.

The ideal solution would probably be to change the semantics of the existing assert keyword to compile to the INVALID opcode and introduce a new require (or assume) keyword that is used for parameter validation (using REVERT). This would be in line with Solidity and less confusing to developers. I was actually involved in a similar discussion there (ethereum/solidity#1702) way back that led to the introduction of require.

On the other hand, I understand that you might not want to change the semantics and then your solution will work as long as it is well-documented.

@jacqueswww
Copy link
Contributor Author

jacqueswww commented Mar 7, 2019

@fubuloubu @wuestholz the main reason we didn't want to change assert is that Vyper is rooted in python. Where assert gets used for input validation, as well as test assertions, this is why the terminology was chosen as such in #711 - assert with revert is also the natural choice someone writing a contract should use by default (because of the gas refund). I think it makes sense to leave the naming as is (I remember discussing this at length regarding the positives and negatives of either - and then settling on the above.).

@jacqueswww
Copy link
Contributor Author

Meeting notes: closing in favour a proposal to be done where assure gets removed and assert <condition>, UNREACHABLE is used instead.

@fubuloubu
Copy link
Member

@maurelian @wuestholz @japesinator @montyly we've made a decision to modify the syntax to what @jacqueswww has proposed here: #711 (comment)

Please give us feedback!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants