Add solidity-uint compatibility example, ERC20 #366

pdaian · 2017-09-28T19:22:02Z

- What I did

Add well tested Solidity-compatible ERC20 example usable as a base for future tokens, in Viper.

I'm not sure whether the best place for this code is in the Viper repo or in a separate vipererc20 style repository. Arguments for having it in the repo are that I think it's a useful example that people will want to practically use, and I believe test suite 1 is one of the most thorough ERC20 tests I've seen in any language. Arguments against center around the num256 datatype, which was chosen for Solidity interoperability but makes the code slightly less clean than is ideal (with inbuilt num256 functions used, and overflows checked manually).

Arguments against can perhaps be mitigated marginally by #355

- How I did it

A Viper contract was created using the num256 datatype to instantiate a 1:1 Ether-pegged ERC20 token (for compatibility with the ERC20 specification, which specifies uint256 as the datatype for ERC20s). Optional functions are not included. The contract is differentially tested against three ERC20s, two implemented in Solidity and one in Viper, by two programmers other than the original developer of the Viper contract. Two independently developed test suites are used in this differential testing.

It is worth noting that this contract is part of a larger project we are doing, and is structured to be self contained. If Viper contributors feel that this repository is a good place for the code, I will refactor the test scripts into the existing top-level test directory. If consensus is that this would live better in a separate repo, I won't bother putting the effort in.

- How to verify it

See the README for instructions on running tests.

- Description for the changelog

Add Solidity-compatible ERC20 example to examples database.

- Cute Animal Picture


               (
                )
               (
         /\  .-"""-.  /\
        //\\/  ,,,  \//\\
        |/\| ,;;;;;, |/\|
        //\\\;-"""-;///\\
       //  \/   .   \/  \\
      (| ,-_| \ | / |_-, |)
        //`__\.-.-./__`\\
       // /.-(() ())-.\ \\
      (\ |)   '---'   (| /)
       ` (|           |) `
 jgs     \)           (/

pdaian · 2017-09-28T19:25:19Z

@AlexXiong97 @DavidKnott worth noting since you guys participated in the discussion in #275 that several critical issues were fixed between the last code you guys saw and this code. This includes:

Incorrect transferFrom behavior
Non-compliance with final ERC20

Also, the tests have been substantially refined and expanded. I recommend any projects or contracts that used the original Viper ERC20 as their base code make sure to upgrade to include these changes.

pdaian · 2017-09-28T19:37:31Z

(@glambeth you too since you were part of the previous PR discussion)

DavidKnott · 2017-09-28T19:46:27Z

@pdaian First off thank you so much for doing this! I vote for keeping these contracts here as Viper is young enough that developers need all the resources they can get and it'll be easiest for them if all examples are in one place.

On another note, logging/event functionality has recently been added to Viper and I think it'd be cleaner if it was used here. Would you mind changing raw_log to the newer syntax (see test_logging.py)?

pdaian · 2017-09-28T20:08:09Z

@DavidKnott excellent! I was not aware of these changes and that scratches off the main Viper wishlist item for this contract :). Will release an update later tonight that uses the native logging functionality and removes the parts of the README that complain about missing ABI events, etc.

pdaian · 2017-10-02T00:40:16Z

@DavidKnott refactored for log-based events and rebased onto master; now blocked on #373

We are hosting a major event at Cornell Thursday/Friday so I will be able to look into that issue after then if nobody has gotten to it.

pdaian · 2017-10-02T00:47:41Z

After that is fixed, TODO blockers before merge include:

Refactor tests into Viper examples test folder, add to set of automated tests
Some stylistic issues in tests (commented code, unclosed files)

fubuloubu · 2017-10-04T14:52:34Z

@pdaian Please rebase to master

pdaian · 2017-10-05T03:29:53Z

@fubuloubu will do, waiting on #373 which I will also take a look at myself early next week if still open

DavidKnott · 2017-10-05T06:34:13Z

@pdaian Sorry for taking so long to fix the logging issue, #377 should fix it.

pdaian · 2017-10-10T17:42:57Z

@DavidKnott great, thanks! Going to work on getting everything ready for a merge tonight.

fubuloubu · 2017-10-10T23:33:27Z

What's up with this versus #391?

jacqueswww · 2017-10-11T09:41:23Z

@fubuloubu this is a one-for-one num256 contract (I am guessing the assembly will be much closer to the solidity equivalent). Basically in the #391 we use the more natural looking nums internally (which is what we want in the case of anyone implementing actual tokens in viper in the future). This example uses num256 throughout.

fubuloubu · 2017-10-11T15:02:59Z

Oh, I get the difference now. I guess make sure it's obvious in the file structure that one is a solidity replica and the other is only compatible (with different types internally)

pdaian · 2017-10-17T19:43:46Z

@DavidKnott refactored tests into the appropriate directory; I think everything is ready for review!

DavidKnott

Thanks for implementing the logs, it'd be great if you could add in the built in num256 overflow protection (making the viper example cleaner).

DavidKnott · 2017-10-18T00:13:49Z

examples/ERC20_solidity_compatible/ERC20.v.py

+# Events are not yet supported in Viper, so events are NOT
+# included in this token.  This makes this token incompatible
+# with some log-only clients.
+


Events are now supported in Viper :)

DavidKnott · 2017-10-18T00:14:32Z

examples/ERC20_solidity_compatible/ERC20.v.py

+@constant
+def is_overflow_sub(a : num256, b : num256) -> bool:
+    return num256_lt(a, b)
+


I think is_overflow_add and is_overflow_sub are unnecessary due to pr #385

Waiting on #411

DavidKnott · 2017-10-18T00:16:20Z

examples/ERC20_solidity_compatible/README.md

+- Examples of other common token usage patterns (owner-mintable tokens being one 
+    possible example). 
+- [Wishlist] Addition of logs to Viper to remove ugly raw_log calls and 
+    include events in the automatically generated ABI.


This has been done :)

DavidKnott · 2017-10-18T00:17:25Z

tests/examples/ERC20_solidity_compatible/nonviper/ERC20_serpent.se

@@ -0,0 +1,100 @@
+# Author: Florian Tramer


Why are we including a Serpent token when the Serpent compiler isn't safe for production?

Only there to show compatibility in theory, but I will remove since you are right it's a bit pointless to have people install the dependency.

pdaian · 2017-10-18T23:10:24Z

Whoops, mangled the commit history a bit; will fix when I get home tonight (that's what I get for pushing from the train)

pdaian · 2017-10-19T19:01:35Z

@DavidKnott addressed all comments

DavidKnott

Looks great, please remove the last two references of Serpent and I'll merge it in.

DavidKnott · 2017-10-20T21:31:04Z

examples/tokens/ERC20_solidity_compatible/README.md

+- 100% branch and statement coverage (manually verified).
+- Independent tests from two test authors.
+- Differential testing against two example Solidity and one Serpent tokens, written independently
+    (available in the ``nonviper`` subfolder and annotated with comments).


Please remove the serpent reference.

DavidKnott · 2017-10-20T21:31:32Z

tests/examples/tokens/ERC20_solidity_compatible/utils/pyethereum_test_utils.py

+def extract_language(sourcefile):
+    languages = {
+        '.sol': 'solidity',
+        '.se': 'serpent',


Please remove the serpent reference here as well.

… differential testing

pdaian · 2017-10-22T18:49:18Z

Comments addressed, Serpent references removed and minor README wording changes, also rebased onto latest master.

DavidKnott · 2017-10-22T23:46:34Z

LGTM!

pdaian force-pushed the sol_compat_ERC20 branch from d3299dc to 2e22576 Compare September 28, 2017 19:28

pdaian mentioned this pull request Sep 28, 2017

Add Solidity-compatible ERC20 Example #275

Closed

pdaian force-pushed the sol_compat_ERC20 branch from 2e22576 to 22301cd Compare October 2, 2017 00:30

pdaian mentioned this pull request Oct 2, 2017

Parser can't do multiple calls to same event with indexed params #373

Closed

pdaian force-pushed the sol_compat_ERC20 branch from 22301cd to 32a0370 Compare October 17, 2017 19:33

DavidKnott reviewed Oct 18, 2017

View reviewed changes

pdaian force-pushed the sol_compat_ERC20 branch 2 times, most recently from 40c8524 to 0ba4ac4 Compare October 19, 2017 16:46

DavidKnott reviewed Oct 20, 2017

View reviewed changes

pdaian added 5 commits October 22, 2017 14:47

add solidity-uint compatibility example, ERC20

4a49ec5

move to log based events

c0723c8

refactor tests into own directory

5a177be

remove outdated comments implying Viper has no log support

709571f

move solidity compatible example into new tokens example dir

e9ba65a

pdaian added 4 commits October 22, 2017 14:47

remove overflow protection, rely on built-in protection instead

e2685d6

remove Serpent example

9c44fe9

add attribution to lorenz and florian in SolidityERC20 test suite

c0de035

remove serpent support in Solidiy ERC20 test suite, clarify README on…

c193d34

… differential testing

pdaian force-pushed the sol_compat_ERC20 branch from 5065c46 to c193d34 Compare October 22, 2017 18:47

DavidKnott merged commit 753f043 into vyperlang:master Oct 22, 2017

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add solidity-uint compatibility example, ERC20 #366

Add solidity-uint compatibility example, ERC20 #366

pdaian commented Sep 28, 2017

pdaian commented Sep 28, 2017

pdaian commented Sep 28, 2017

DavidKnott commented Sep 28, 2017 •

edited

Loading

pdaian commented Sep 28, 2017

pdaian commented Oct 2, 2017

pdaian commented Oct 2, 2017

fubuloubu commented Oct 4, 2017

pdaian commented Oct 5, 2017

DavidKnott commented Oct 5, 2017

pdaian commented Oct 10, 2017

fubuloubu commented Oct 10, 2017

jacqueswww commented Oct 11, 2017 •

edited

Loading

fubuloubu commented Oct 11, 2017

pdaian commented Oct 17, 2017

DavidKnott left a comment

DavidKnott Oct 18, 2017

DavidKnott Oct 18, 2017

pdaian Oct 18, 2017

DavidKnott Oct 18, 2017

DavidKnott Oct 18, 2017

pdaian Oct 18, 2017

pdaian commented Oct 18, 2017

pdaian commented Oct 19, 2017

DavidKnott left a comment

DavidKnott Oct 20, 2017

DavidKnott Oct 20, 2017

pdaian commented Oct 22, 2017

DavidKnott commented Oct 22, 2017

Add solidity-uint compatibility example, ERC20 #366

Add solidity-uint compatibility example, ERC20 #366

Conversation

pdaian commented Sep 28, 2017

- What I did

- How I did it

- How to verify it

- Description for the changelog

- Cute Animal Picture

pdaian commented Sep 28, 2017

pdaian commented Sep 28, 2017

DavidKnott commented Sep 28, 2017 • edited Loading

pdaian commented Sep 28, 2017

pdaian commented Oct 2, 2017

pdaian commented Oct 2, 2017

fubuloubu commented Oct 4, 2017

pdaian commented Oct 5, 2017

DavidKnott commented Oct 5, 2017

pdaian commented Oct 10, 2017

fubuloubu commented Oct 10, 2017

jacqueswww commented Oct 11, 2017 • edited Loading

fubuloubu commented Oct 11, 2017

pdaian commented Oct 17, 2017

DavidKnott left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

pdaian commented Oct 18, 2017

pdaian commented Oct 19, 2017

DavidKnott left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

pdaian commented Oct 22, 2017

DavidKnott commented Oct 22, 2017

DavidKnott commented Sep 28, 2017 •

edited

Loading

jacqueswww commented Oct 11, 2017 •

edited

Loading