Clean Up Deposit Contract #460

nisdas · 2019-01-17T13:14:30Z

This PR cleans up the deposit contract, with changes in declarations, which was suggested by @jacqueswww.

Also another issue was that the vyper contract was not able to be compiled, however @jacqueswww has been kind enough to put up a quick fix here: vyperlang/vyper#1202. After that is merged in the deposit contract should have no issues being compiled.

Changes in Contract

Update version number to reflect latest vyper release.
Perform all calculations in terms of gwei
Change global constant from Gwei_Per_Eth to Wei_Per_Gwei

hwwhww · 2019-01-17T15:54:57Z

There's a MAX_DEPOSIT_AMOUNT thing on L691.

hwwhww · 2019-01-17T15:59:03Z

@nisdas
I labeled DO NOT MERGE since it's pending on the Vyper release.

jacqueswww · 2019-01-18T13:33:22Z

Just tagged beta-7, please give it a shot :)

hwwhww

Tested here: ethereum/deposit_contract#4

hwwhww · 2019-01-19T09:33:33Z

Second thought, the bytecodes are exactly the same. So I don't feel strongly of having this refactoring or not because it brings the "ether" unit back to the spec again. /cc @terenc3t @ralexstokes @JustinDrake

jacqueswww · 2019-01-19T09:35:07Z

Hmmm I see , valid point.

nisdas · 2019-01-19T09:48:47Z

@hwwhww Is there any reason we shouldn't be using the ether unit ?

hwwhww · 2019-01-19T09:59:35Z

@nisdas due to the point of view from #418. :)

nisdas · 2019-01-19T10:07:18Z

@hwwhww ah , k , fair enough then. Yeah then it would make sense to keep it in gwei

terencechain · 2019-01-19T14:36:18Z

Ahh yes. I thought the direction we were heading is to normalize everything to Gwei : )

nisdas · 2019-01-19T15:31:39Z

@hwwhww alright changed it to gwei :)

JustinDrake · 2019-01-19T18:54:09Z

specs/core/0_beacon-chain.md

-    assert msg.value >= as_wei_value(MIN_DEPOSIT_AMOUNT, "gwei")
-    assert msg.value <= as_wei_value(MAX_DEPOSIT_AMOUNT, "gwei")
+    assert msg.value >= MIN_DEPOSIT
+    assert msg.value <= MAX_DEPOSIT

    index: uint256 = self.deposit_count + TWO_TO_POWER_OF_TREE_DEPTH
    deposit_amount: bytes[8] = slice(concat("", convert(msg.value / GWEI_PER_ETH, bytes32)), start=24, len=8)


msg.value / GWEI_PER_ETH is only "correct" by chance. It should be msg.value / WEI_PER_GWEI.

JustinDrake · 2019-01-19T18:56:29Z

specs/core/0_beacon-chain.md

@@ -670,8 +670,8 @@ full_deposit_count: uint256
 @payable
 @public
 def deposit(deposit_input: bytes[2048]):
-    assert msg.value >= as_wei_value(MIN_DEPOSIT_AMOUNT, "gwei")
-    assert msg.value <= as_wei_value(MAX_DEPOSIT_AMOUNT, "gwei")
+    assert msg.value >= MIN_DEPOSIT


In line with using Gwei everywhere, my suggestion is to define deposit_amount (e.g. deposit_amount = msg.value / WEI_PER_GWEI) and then only deal with Gwei amounts (never msg.value directly).

nisdas · 2019-01-20T08:05:34Z

@JustinDrake Alright I changed all references to gwei. Also changed min/max deposits to be represented in terms of gwei

* Use `self.get_deposit_root` instead of `self.deposit_tree[1]` * Use `self.to_bytes` instead of `slice(concat("", convert( , bytes32)), start=24, len=8)` * By consistent with naming (`deposit_amount` and `MAX_DEPOSIT_AMOUNT`) * Remove unnecessary intermediate variables * By consistent with comments (two spaces for inline comments, capitalise first letter) * Symbolic constant now supported for `get_branch` * Simplify code

nisdas · 2019-01-21T11:13:29Z

@JustinDrake Hey, so I was testing this contract out and a bug came out, which is referenced here:
vyperlang/vyper#1206

We shouldn't call functions when emitting logs, as it leads to undecodable logs

JustinDrake · 2019-01-21T13:11:26Z

@nisdas Can you try making to_bytes @public as suggested in vyperlang/vyper#1206? (BTW, how can I reproduce your tests?)

nisdas · 2019-01-21T16:39:58Z

@JustinDrake The bug applies to both public and private function calls. This is our current test:
https://github.com/prysmaticlabs/prysm/blob/ec9465483c739d86ffdc4e04d1d1cd357f45562f/contracts/deposit-contract/depositContract_test.go#L101

And it breaks here:
https://github.com/prysmaticlabs/prysm/blob/ec9465483c739d86ffdc4e04d1d1cd357f45562f/contracts/deposit-contract/depositContract_test.go#L139
https://github.com/prysmaticlabs/prysm/blob/ec9465483c739d86ffdc4e04d1d1cd357f45562f/contracts/deposit-contract/POWlogs.go#L11

The abi decoder will fail to unpack the log data into the separate log arguments

JustinDrake · 2019-01-22T10:34:16Z

I suggest we wait until Vyper gets fixed vyperlang/vyper#1213

The approval was for denomination clean up. Need farther tests for of the latest branch and also pending on #490 discussion.

nisdas · 2019-01-28T10:15:23Z

@JustinDrake Hey vyperlang/vyper#1213 is merged in now are we waiting on #490, or is this good to be merged in ?

JustinDrake · 2019-01-28T12:45:31Z

@nisdas Shall we wait for the 0.1.0-beta.8 release? Also need to merge with #490

nisdas · 2019-01-28T13:57:39Z

@JustinDrake Sounds good to me 👍

vbuterin · 2019-01-28T22:30:37Z

specs/core/0_beacon-chain.md

@@ -667,45 +666,48 @@ deposit_tree: map(uint256, bytes32)
 deposit_count: uint256
 full_deposit_count: uint256

+@private
+@constant
+def to_bytes(value: uint256) -> bytes[8]:


to_bytes -> to_bytes8

jacqueswww · 2019-01-29T21:18:30Z

With regards to when beta8 will be released, I hope sometime next week, this will also include the bytes32 slice() fix, which one can then inline the to_bytes function at a much lower gas cost (I expect, still have to make the change).

djrtwo · 2019-01-31T02:01:31Z

@nisdas Can you sync this with the latest changes in master?

…o depositContractFix

nisdas · 2019-01-31T05:18:22Z

@djrtwo Alright, done

nisdas · 2019-02-05T08:01:40Z

With #562 being merged in, this PR doesnt make sense now. So I am closing it

cleaning up contract

764f115

hwwhww added the DO NOT MERGE label Jan 17, 2019

adding max deposit

20b5425

hwwhww added a commit to hwwhww/deposit_contract that referenced this pull request Jan 18, 2019

Sync with ethereum/consensus-specs#460

d7ab85c

hwwhww mentioned this pull request Jan 18, 2019

Sync with ethereum/eth2.0-specs#460 ethereum/deposit_contract#4

Closed

hwwhww previously approved these changes Jan 18, 2019

View reviewed changes

hwwhww removed the DO NOT MERGE label Jan 18, 2019

hwwhww requested a review from JustinDrake January 19, 2019 07:17

change to gwei

a8f6032

change version

b8e6eb7

JustinDrake reviewed Jan 19, 2019

View reviewed changes

nisdas mentioned this pull request Jan 20, 2019

Replace Solidity Contract With Vyper prysmaticlabs/prysm#1343

Merged

2 tasks

changing deposit to gwei

fcb8e58

nisdas changed the title ~~Make Min/Max Deposits Cleaner in Deposit Contract~~ Clean Up Deposit Contract Jan 20, 2019

JustinDrake added 3 commits January 20, 2019 20:46

Update 0_beacon-chain.md

28b29a3

Update 0_beacon-chain.md

c0a8551

Update 0_beacon-chain.md

e0c2250

vbuterin reviewed Jan 28, 2019

View reviewed changes

nisdas added 2 commits January 29, 2019 10:11

updating func naming

3906c78

make func public

a5f248e

nisdas added 2 commits January 31, 2019 13:09

Merge branch 'master' of https://github.com/ethereum/eth2.0-specs int…

28c1f31

…o depositContractFix

fix merge issues

4effea5

djrtwo changed the base branch from master to dev January 31, 2019 15:49

nisdas closed this Feb 5, 2019

nisdas deleted the depositContractFix branch February 5, 2019 08:02

nisdas mentioned this pull request Feb 11, 2019

Clean Up Deposit Contract ethereum/deposit_contract#9

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Clean Up Deposit Contract #460

Clean Up Deposit Contract #460

nisdas commented Jan 17, 2019 •

edited

Loading

hwwhww commented Jan 17, 2019

hwwhww commented Jan 17, 2019

jacqueswww commented Jan 18, 2019

hwwhww left a comment

hwwhww commented Jan 19, 2019 •

edited

Loading

jacqueswww commented Jan 19, 2019 •

edited

Loading

nisdas commented Jan 19, 2019

hwwhww commented Jan 19, 2019

nisdas commented Jan 19, 2019

terencechain commented Jan 19, 2019 •

edited

Loading

nisdas commented Jan 19, 2019

JustinDrake Jan 19, 2019

JustinDrake Jan 19, 2019

nisdas commented Jan 20, 2019

nisdas commented Jan 21, 2019

JustinDrake commented Jan 21, 2019

nisdas commented Jan 21, 2019

JustinDrake commented Jan 22, 2019

nisdas commented Jan 28, 2019 •

edited

Loading

JustinDrake commented Jan 28, 2019

nisdas commented Jan 28, 2019

vbuterin Jan 28, 2019

jacqueswww commented Jan 29, 2019

djrtwo commented Jan 31, 2019

nisdas commented Jan 31, 2019

nisdas commented Feb 5, 2019

Clean Up Deposit Contract #460

Clean Up Deposit Contract #460

Conversation

nisdas commented Jan 17, 2019 • edited Loading

hwwhww commented Jan 17, 2019

hwwhww commented Jan 17, 2019

jacqueswww commented Jan 18, 2019

hwwhww left a comment

Choose a reason for hiding this comment

hwwhww commented Jan 19, 2019 • edited Loading

jacqueswww commented Jan 19, 2019 • edited Loading

nisdas commented Jan 19, 2019

hwwhww commented Jan 19, 2019

nisdas commented Jan 19, 2019

terencechain commented Jan 19, 2019 • edited Loading

nisdas commented Jan 19, 2019

JustinDrake Jan 19, 2019

Choose a reason for hiding this comment

JustinDrake Jan 19, 2019

Choose a reason for hiding this comment

nisdas commented Jan 20, 2019

nisdas commented Jan 21, 2019

JustinDrake commented Jan 21, 2019

nisdas commented Jan 21, 2019

JustinDrake commented Jan 22, 2019

nisdas commented Jan 28, 2019 • edited Loading

JustinDrake commented Jan 28, 2019

nisdas commented Jan 28, 2019

vbuterin Jan 28, 2019

Choose a reason for hiding this comment

jacqueswww commented Jan 29, 2019

djrtwo commented Jan 31, 2019

nisdas commented Jan 31, 2019

nisdas commented Feb 5, 2019

nisdas commented Jan 17, 2019 •

edited

Loading

hwwhww commented Jan 19, 2019 •

edited

Loading

jacqueswww commented Jan 19, 2019 •

edited

Loading

terencechain commented Jan 19, 2019 •

edited

Loading

nisdas commented Jan 28, 2019 •

edited

Loading