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

VIP: Disallow side-effecting expressions in assert #1150

Closed
charles-cooper opened this issue Dec 17, 2018 · 7 comments
Closed

VIP: Disallow side-effecting expressions in assert #1150

charles-cooper opened this issue Dec 17, 2018 · 7 comments
Labels
VIP: Approved VIP Approved VIP: Discussion Used to denote VIPs and more complex issues that are waiting discussion in a meeting

Comments

@charles-cooper
Copy link
Member

charles-cooper commented Dec 17, 2018

Simple Summary

Ensure assertions don't have side effects.

Abstract

Assertions should not have any side effects. This also applies to the new assertion type proposed in #711.
(Note: memory safety is guaranteed by Vyper's memory model. If Vyper were to have reference types however, the compiler would have to ensure that function calls do not modify memory outside of the function frame).

Motivation

Check out the following misleading function

user_balances: map(address, wei)
balance: wei
def get_balance() -> wei {
  bal: wei = self.balance;
  self.balance = 0 # Uh oh!!
  return bal
}
def accept_funds() {
  self.balance += msg.value
  self.user_balances[msg.sender] += msg.value
  assert self.user_balances[msg.sender] <= self.get_balance()
}

The main issue for readability is that an assertion could be true before it has been asserted but not after!

Specification

Add constraint to any assertion which ensures that the asserting expression has no side effects. I believe the only case in which this can happen is if the asserting expression calls a non-constant or payable function, so, this boils down to checking that if any function calls are made they must be to constant functions. (Note: in Vyper @constant implies non-payable, but I am not sure if this is ensured by the ABI).

EDIT: I realized that even if a function's ABI tells us that it is constant or non-payable, we can't prove it unless it is a function we are currently compiling (like a private function). So we must also use STATICCALL. This lets us use a very straightforward implementation (although the error messages might be confusing) - set context.is_constant to True while compiling the assertion test.

Backwards Compatibility

This could weed out existing programs which have side-effecting asserts. However, I think this is a feature!

Copyright

Copyright and related rights waived via CC0

@jakerockland
Copy link
Contributor

jakerockland commented Dec 17, 2018

Probably worth adding "VIP: " to the title here as this seems like a noteworthy functional change, I like this idea a lot @charles-cooper 👍

@charles-cooper charles-cooper changed the title Disallow side-effecting expressions in assert VIP: Disallow side-effecting expressions in assert Dec 17, 2018
@charles-cooper
Copy link
Member Author

@jakerockland good point - updated, thanks!

@fubuloubu
Copy link
Member

On board with this, good catch!

@jacqueswww jacqueswww added the VIP: Discussion Used to denote VIPs and more complex issues that are waiting discussion in a meeting label Dec 18, 2018
@jacqueswww
Copy link
Contributor

jacqueswww commented Jan 7, 2019

As discussed, this proposal is approved.

@jacqueswww jacqueswww added the VIP: Approved VIP Approved label Jan 10, 2019
@vbuterin
Copy link
Contributor

vbuterin commented Jan 10, 2019

Support this! In general, side effects inside of things that don't look like they're state-changing expressions are IMO un-Vyperic and should be banned with prejudice 😄

@jacqueswww
Copy link
Contributor

This has been merged, closing.

@fubuloubu
Copy link
Member

As noted in #1468, it seems like a special exception should be made for external calls that return values after modifications are made.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
VIP: Approved VIP Approved VIP: Discussion Used to denote VIPs and more complex issues that are waiting discussion in a meeting
Projects
None yet
Development

No branches or pull requests

5 participants