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

Documentation for default funcs #896

Merged
merged 7 commits into from
Jul 27, 2018
Merged

Conversation

liamzebedee
Copy link
Contributor

@liamzebedee liamzebedee commented Jun 14, 2018

- What I did

Fixes: #892.

Documented new default function usage.

- How I did it

Read the source changes, researched a bit about the EVM execution sematics to communicate a stronger understanding.

- How to verify it

Generate docs and visit structure-of-a-contract.html

- Description for the changelog

Documented default functions of contracts (fallback funcs).

- Cute Animal Picture

image

@liamzebedee
Copy link
Contributor Author

Fixes #892


A contract can also have a default function, which is executed on a call to the contract if no other functions match the given function identifier (or if none was supplied at all, such as through someone sending it Eth). It is the same construct as fallback functions `in Solidity <https://solidity.readthedocs.io/en/latest/contracts.html?highlight=fallback#fallback-function>`_.

This function is always named `__default__` and must be annotated with `@public` and `@payable`. It cannot have arguments and cannot return anything.
Copy link
Member

Choose a reason for hiding this comment

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

I think @payable is optional, confirm @jacqueswww?

Copy link
Contributor Author

@liamzebedee liamzebedee Jun 14, 2018

Choose a reason for hiding this comment

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

Ah woops! Will fix that.

Copy link
Member

Choose a reason for hiding this comment

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

I'm not 💯, wait for confirmation from @jacqueswww unless you found in the code/tests where this is allowed.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@fubuloubu Haha I found the test, I just misread the failure condition initially.

https://github.com/ethereum/vyper/blob/master/tests/parser/functions/test_default_function.py#L34

Copy link
Contributor

Choose a reason for hiding this comment

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

Yes payable is optional, depending on the use case ;)

Default function
----------------

A contract can also have a default function, which is executed on a call to the contract if no other functions match the given function identifier (or if none was supplied at all, such as through someone sending it Eth). It is the same construct as fallback functions `in Solidity <https://solidity.readthedocs.io/en/latest/contracts.html?highlight=fallback#fallback-function>`_.
Copy link
Member

Choose a reason for hiding this comment

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

"if no other functions match the given function identifier" I am pretty sure this is the behavior we have, but I don't actually like this behavior now that I think about it. The default function should not have any call data associated with it (aka fails if call data is provided) and I think in general if the function signature switch falls through, the contract should revert by default. We can keep this behavior in place for the fallback function, since that will check that call data is zero if no function signatures match.

@jacqueswww Please confirm current behavior matches this documentation, and also comment on my suggestion for modifying the behavior of the default function. This should be compatible with any contracts created by Solidity. In fact, do we have a test case for one contract sending another Eth internally?

Copy link
Contributor

Choose a reason for hiding this comment

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

@fubuloubu this is the current behaviour.

And therefore I think it's fine to leave it as is (in the docs). However we can definitely scope a VIP for splitting default function into 2 cases:
1.) Receive funds
2.) Function Not Found (404)
I saw this on solidity something similar is being discussed. ethereum/solidity#3198 (comment)


::

Sent: event({sender: indexed(address)})
Copy link
Member

Choose a reason for hiding this comment

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

Suggestion: maybe add amount to this event, so if someone tries to run it you can see the amount you sent in the event.

Copy link
Contributor

Choose a reason for hiding this comment

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

Good idea.

- Calling an external function which consumes a large amount of gas
- Sending Ether

Since global variables are still accessible, you can pass data to the default function through the raw byte buffer provided to EVM's CALL (`msg.data`).
Copy link
Member

Choose a reason for hiding this comment

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

See above, I think we should disallow this behavior


Unlike Solidity, Vyper generates a default function is one isn't found, in the form of a REVERT call. Note that this still `generates an exception <https://github.com/ethereum/wiki/wiki/Subtleties>`_ and thus will not succeed in receiving funds.

Send calls to the contract give it a maximum stipend of 2300 gas, leaving not much room to perform other operations except basic logging. The following operations will consume more gas than the 2300 gas stipend:

Choose a reason for hiding this comment

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

You can still choose to include a higher gas amount when you send ETH to an address, which allows more complex fallback functions that will automatically fail unless extra gas is included. At least thats how it is with Solidity fallbacks, hoping its the same here.

Many ICOs allowed purchasing by sending ETH to the fallback and including ~200,000 gas. I have even heard of MEW automatically s the higher gas amount when it detected a user sending to a known ICO with a fallback.

Copy link
Contributor

Choose a reason for hiding this comment

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

@haydenadams yes it is I believe, created an issue to demonstrate this as a tests.
@liamzebedee please make it clear that one can still send more gas to a default function, if you don't use send. for example raw_call / CALL allows you to send as much gas as you like.

@liamzebedee
Copy link
Contributor Author

Ready for another review!

@liamzebedee
Copy link
Contributor Author

Ping @fubuloubu

Copy link
Member

@fubuloubu fubuloubu left a comment

Choose a reason for hiding this comment

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

Some updates, but it's looking good.

- Calling an external function which consumes a large amount of gas
- Sending Ether

Since global variables are still accessible, you can pass data to the default function through the raw byte buffer provided to EVM's CALL (``msg.data``).
Copy link
Member

Choose a reason for hiding this comment

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

I think accessing msg.data is discouraged behavior. Please mention that, or skip this section. You could perhaps mention accessing msg.sender instead.

Copy link
Contributor

Choose a reason for hiding this comment

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

+1 a block mentioning that when using msg.data care should be taking.

Considerations
~~~~~~~~~~~~~~

Unlike Solidity, Vyper generates a default function is one isn't found, in the form of a REVERT call. Note that this still `generates an exception <https://github.com/ethereum/wiki/wiki/Subtleties>`_ and thus will not succeed in receiving funds.
Copy link
Member

Choose a reason for hiding this comment

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

Typo: "... generates a default function if one isn't found"


Unlike Solidity, Vyper generates a default function is one isn't found, in the form of a REVERT call. Note that this still `generates an exception <https://github.com/ethereum/wiki/wiki/Subtleties>`_ and thus will not succeed in receiving funds.

Ethereum specifies that the operations will be rolled back if the contract runs out of gas in execution. ``send`` calls to the contract come with a free stipend of 2300 gas, which does not leave much room to perform other operations except basic logging. **However**, if the sender includes a higher gas amount through a ``call`` instead of ``send``, then more complex functionality can be run.
Copy link
Member

Choose a reason for hiding this comment

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

This is more nuanced than your description. I can call a default/fallback directly from a raw transaction (in a contract or from an EOA) and the gas stipend is more than 2300. This stipend only holds when when an address.transfer(uint) is called, as implemented by Solidity and Vyper compilers as a mitigation against excessive gas usage.

It is therefore considered a best practice to ensure your payable default function is compatible with the 2300 gas stipend. If it is currently an error, then overusing the gas stipend should be a warning instead. There are legitimate use cases for a default that exceeds this amount. Lots of nuance here.

@jacqueswww, do you agree?

@jacqueswww
Copy link
Contributor

jacqueswww commented Jul 10, 2018

@liamzebedee looking good, make above changes and we are good to go!
Also make sure your ethereum address set in your openbounty profile.

@liamzebedee
Copy link
Contributor Author

RE: global variables that are accessible, I scraped this list from expr.py of what we can currently access in contracts:

msg.sender
msg.value
msg.gas
block.difficulty
block.timestamp
block.coinbase
block.number
block.prevhash
tx.origin

Would be worth adding this later on :)

@liamzebedee
Copy link
Contributor Author

👍 take a peek!

@jacqueswww
Copy link
Contributor

@liamzebedee sorry for only getting back to you now, slipped past my radar. I think it's ready to merge. Thanks :)

@jacqueswww jacqueswww merged commit 6c98cab into vyperlang:master Jul 27, 2018
@jacqueswww
Copy link
Contributor

jacqueswww commented Jul 31, 2018

@liamzebedee do you have a payout address set in openbounty.status.im? There is a bounty on this issue, and I want to pay it out 😄

@liamzebedee
Copy link
Contributor Author

@jacqueswww sure am :)

@pablanopete
Copy link

@liamzebedee sorry for the slow response on bounty payout, could you drop your wallet address here so I can send you the bounty amount Out of Band while we wait on a dashboard fix in Status Open Bounty.

If you don't feel comfortable putting it here in the comment could you email a ERC20 compatible wallets address to me? chris.hutchinson@status.im

@liamzebedee
Copy link
Contributor Author

Cheers @pablanopete :)

0xf87de2add59a3c23ab6365f8addfc5db9c86b3b9

@pablanopete
Copy link

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.

5 participants