Skip to content
This repository has been archived by the owner on Sep 5, 2020. It is now read-only.

DAO fork support #970

Merged
merged 10 commits into from
Jul 17, 2016
Merged

DAO fork support #970

merged 10 commits into from
Jul 17, 2016

Conversation

alexvandesande
Copy link

@alexvandesande alexvandesande commented Jul 14, 2016

This PR allows the user to choose his side on the Fork debate. Button positions are randomised to allow a more neutral stance.

screen shot 2016-07-14 at 5 33 32 pm

While this is implemented in practice, we are open to community feedback on the wording. Our intent is to be as informative as possible and represent both sides.

@veox
Copy link

veox commented Jul 14, 2016

s/july/July/
s/june/June/
s/configuration on/configuration of/
s/to connect./to connect to.\n/

@jph108
Copy link

jph108 commented Jul 14, 2016

This seems very fair. It might be nice to have more explanation on what the "I will follow what the community wants" button does. Or is that a link?

June and July should be capitalized, and community-created should have a dash.

Also, maybe: "Click 'Yes' to move to a new chain in which the DAO funds are refunded to the DAO tokens holders. Click 'No' to remain on the current chain, where this will not occur."

Overall, it looks very good.

@aakilfernandes
Copy link

aakilfernandes commented Jul 14, 2016

"You will be able to switch back later"

This is not entirely true due to transaction replay attacks. You cannot interact with one side of the chain and be sure your transactions won't be replayed on the other chain.

@aakilfernandes
Copy link

I would change the wording to "replaces the DAO contract with a refund contract". This fork does not refund users, it only gives them a pathway to a refund.

I also think the words under the "Yes" and "No" are needlessly editorial.

What metric is used for "the majority"?

@rfikki
Copy link

rfikki commented Jul 14, 2016

There should be no statements and words underneath the "yes" and "no" buttons.

@rfikki
Copy link

rfikki commented Jul 14, 2016

  1. "Important decision affecting your wallet" should be changed to something like "Important decision regarding blockchain choice"
  2. "regardless of what the majority does" should be changed to "regardless of what anyone else does".

@kungfualden
Copy link

I'm for the new block chain, I think. But maybe if you're not for it, than explain what happens.. I'm a bit lost when it comes to the old chain, if I stay. Do I loose Dao tokens?

@aakilfernandes
Copy link

"You will be taken to a chain of your choice" is also inaccurate. Users won't have their chain changed if they choose "no". It sets up a false equivalency between two unequal choices. One applies a state change, the other does not.

@taoeffect
Copy link

Forgive me, but I do not understand how this is a user-choice.

There is, and can only ever be, one "Ethereum". The same goes for the token $ETH.

What this seems to do is split the users up into two separate worlds where they each have "ETH" in name only, but in reality only one of them does. I believe this could result in loss of funds as time goes on, and therefore seems to be a very dangerous suggestion.

Finally, the options are not clear, and their consequences are not spelled out. If the consequences are as I've described here, then this should not be a choice that users are asked to make (at least like this) in the first place.

@vbuterin
Copy link

I disagree with the descriptions of the choices :)

Many proponents of the fork now are still proponents of eventually getting to a very strong no-forking norm, for example.

@jph108
Copy link

jph108 commented Jul 15, 2016

...about which blockchain to utilize (not 'connect').

@rabbit
Copy link

rabbit commented Jul 15, 2016

my personal suggestions:

  • change all three actions to text, no buttons
  • replace "yes" and "no" with actionable statements, example:
    • Accept the manual update to The DAO
    • Reject the manual update to The DAO
    • Take no action by following the majority decision
  • keep random choice ordering but include all three actions
  • avoid educating the user about The DAO or its implications
    • do not use phrases like "rapid unplanned withdrawal event" -- it sounds like bullshit to avoid words like "theft" or "security flaw"
    • do not use phrases like "social consensus should be final" -- I might think so but disagree with this action
    • suggest that the user conduct their own research before taking action

@taoeffect
Copy link

I like @rabbit's suggestions, and echo @aakilfernandes's question about the use of the word "majority". Majority of what?

However, I reiterate that all of this quibbling over wording appears to be moot. Unless this is a dialog that is for miners only, this PR seems to put users in real danger of losing their funds and being unable to interact with the network.

Unless, that is, one of the forks is given a name. But then obviously that would bias the result in favor of whichever retains "Ethereum".

@neeeeeeext
Copy link

neeeeeeext commented Jul 15, 2016

The question
"Do you want to move to a new chain in which the DAO funds are refunded to the DAO token holders?"
should be more accurate by adding:
"and the attacker does not get to keep the stolen funds."

If mist is to offer a choice for the non-technical users, I think it's very important to accurately convey that staying on the old chain means staying on the chain where the attacker controls a significant amount of stolen ETH.

@danielmcclure
Copy link

danielmcclure commented Jul 15, 2016

Potential rewording for consideration...

Important decision affecting the Ethereum blockchain.

A new community created blockchain will be available for activation at block #1920000, which is expected to arrive on July 20th 2016, in which the Ether balance of contract 0xbb9bc244d798123fde783fcc1c72d3bb8c189413 (commonly referred to as "The DAO") and all of its descendants will be moved to a refund contract due to an exploited bug in the original contract. This should reclaim funds from all contracts linked to the exploit and allow authenticated holders of "The DAO" tokens to claim a refund.

ACTION REQUIRED: To maintain democratic consensus it is important that you select the blockchain you would like to activate for this wallet. This is a settings configuration, not a vote, meaning that your wallet will operate on the blockchain of your choosing regardless of any other users choice, unless you opt for "No Preference" in which case you wallet will follow consensus as defined here (link or explanation??).

_Do you want to activate the chain in which funds linked to the exploit are reclaimed to a refund contract where they can be claimed by "The DAO" token holders?_

Randomized Buttons
Yes / No / No Preference (Follow Consensus)

Please note that regardless of your choice, all of your Ethereum addresses and balances will exist in the same state on both chains until further balance affecting transactions are made, and it is possible to move to a different blockchain at a later date.

(Click Here to Learn More About The Issue)"

@alexvandesande
Copy link
Author

Thanks for all the feedback. I added some changes to the window based on this feedback:

screen shot 2016-07-15 at 12 03 45 pm

@aakilfernandes
Copy link

Better. Thanks Alex!

However I really, really, really think you should drop or clarify the line about "you will be able to switch back at a later date". Lets say a user creates a contract wallet on a fork using ether they obtained post-fork. That wallet won't exist on the other fork. This is something that is not obvious to many users. It also doesn't indicate the reality about cross chain replay attacks which users must consider.

@aakilfernandes
Copy link

"Activate" here is also inaccurate. "Follow" or "reference" is more accurate.

@frozeman
Copy link
Contributor

You don't seem to remember the choices of the user. Do we want to show this window at all times? I would say it should certainly not appear anymore after the fork block. Maybe just a notice in which chain you are and how to switch. And all its issues with reply attacks.

@frozeman
Copy link
Contributor

screen shot 2016-07-16 at 23 40 31

@frozeman
Copy link
Contributor

Please test and run.

Make sure to run npm install first to download the new nodes.

The only thing missing is the link of the “read more here” link to an actual article!

Please merge and prepare the release and test asap.

@jph108
Copy link

jph108 commented Jul 16, 2016

Just my opinion - the very first version was simple and easier to read. The latest version is very technical, many people won't understand it.

Also, a small typo: "Hard Frok".

@frozeman
Copy link
Contributor

I rather want to have it precise as unclear. I found the first to be not clear. Its not a blockchain choice here, as very likely the loosing chain will not exists for very long.

@alexvandesande feel free to improve my text changes

@jph108
Copy link

jph108 commented Jul 16, 2016

Hi everyone,

I'm new here so this is just my $0.02. Take it for what it's worth :-)

Important Decision Affecting Your Wallet

Sometime on July 20th, 2016, the Ethereum blockchain will split into two forks. On one fork, the Ether balance of a contract named "The DAO" and some of its children will be moved into a 'withdrawal-only' contract. This action is being taken due to a bug in The DAO contract and will allow authenticated holders of 'DAO tokens' to claim a refund.

You are free to create transactions on the blockchain of your choice, but you can only use one blockchain at a time. Use the buttons below to set this application to use one fork or the other. Transactions made on one fork will not appear on the other. This is a setting, not a vote, and you can change this setting later if desired (for technical details, see the link below).

Learn more about the issue, including technical details on how to switch forks.

Do you want to move to a new blockchain in which the DAO funds are refunded to the DAO token holders?

Yes / No

Or, click here to follow the majority vote on CarbonVote.com as of July 15, 2016.

I would move all technical details, including the block number, DAO contract address, details on how to switch forks, etc, into the "Learn more about the issue" page.

@taoeffect
Copy link

taoeffect commented Jul 17, 2016

This PR has a high probability of causing financial harm to users, and an extremely high probability of damaging Ethereum's reputation.

@vbuterin
Copy link

I think what Greg is trying to say is that users will be confused because if they select, for example, the NO button and the YES chain becomes the dominant chain, then they will see themselves receiving, transferring, etc "ETH", "MKR", etc, whereas what the community will mean by ETH, MKR, etc will be a different asset on a different chain, so whatever the fork choice mechanism is should in some way (and I have not yet thought of the best way) make clearer that if they press a different button from what the community ends up settling on then their client will not, in the sense in which the community will understand the term, be acting as an "Ethereum client". This confusion could lead to loss of funds. Some examples:

  • If a user tries to send their ETH to an exchange, but the exchange is on a different fork and so never acknowedges their deposit, then they would instead be essentially donating their alt-ETH to the exchange.
  • If I am trading my 60 ETH for your 1 BTC, and you are on the chain that will eventually be the weak chain, and I could send you 60 weak-ETH, and you send me the BTC, but on the "strong chain" I have the eth and the btc and you have nothing.

@taoeffect
Copy link

I think what Greg is trying to say is that users will be confused because if they select, for example, the NO button and the YES chain becomes the dominant chain, then they will see themselves receiving, transferring, etc "ETH", "MKR", etc, whereas what the community will mean by ETH, MKR, etc will be a different asset on a different chain, so whatever the fork choice mechanism is should in some way (and I have not yet thought of the best way) make clearer that if they press a different button from what the community ends up settling on then their client will not, in the sense in which the community will understand the term, be acting as an "Ethereum client". This confusion could lead to loss of funds. Some examples:

Yep. 👍

@vbuterin
Copy link

More generally (eg. this also touches on the replay issue) perhaps part of our messaging should be 'be very careful about undertaking economically meaningful actions until the hard fork is "settled"'.

@pesho
Copy link

pesho commented Jul 17, 2016

An ALL CAPS warning that staying (and making transactions) on the chain which ends being "weaker" may cause financial loss would be appropriate. Another reason besides those mentioned by Vitalik is that the user might be vulnerable to a replay attack on the other chain (unless he takes measures to prevent that).

@jph108
Copy link

jph108 commented Jul 17, 2016

I agree with taoeffect's point and thanks for the clarifications in those following. Also it seems changes are in and a build is underway. Minor suggestions for the latest text (which to me seems good).

  1. ...be moved to a withdraw**-only** contract...
  2. ...to redeem it them for ether.
  3. ...will exist in both chains and if desired you will be able to switch back...

I imagine some of the warnings mentioned above might wind up in the disclaimer that was added, which seems like a good idea to have. ☔

alexvandesande pushed a commit that referenced this pull request Jul 17, 2016
* bump version to 0.7.6

* updated os timesync

* adding setting to change UI language (fixes #813) (#896)

* simple language setting menu

* use i18n label in menu

* simple language setting menu

* use i18n label in menu

* change language of menu bar by triggering 'backendAction_setLanguage'

* adds translations for language names

* remove duplicate 'gulp update-nodes' (#897)

* add i18n entry for OSX's 'Services' (#887)

* add i18n entry for OSX's 'Services'

* update missing osx menu strings

* update-popup-window: prevent maximize (#885)

* update-popup-window prevent maximize

* add resizable:false

* added shrinkwrap (#903)

* fixed webview tagd and updated electrong to 1.2.5 (#905)

* Fixes the node start IPC connection (#841)

* fixed eth start, but crash is not graceful

* better socket connection logic, remove master ps logic for eth

* fix splash screen display of state text for eth

* better gulp download plugin, remove master passwd stuff once and for all

* Adds Toggle password visibility to on boarding screen (#909)

* add password visibilty toggle to onboarding

* replaced 'showPassword' with 'passwordInputType'

* changed id into class

* 18n cleanup (former #889) (#910)

* Renamed Korean and Albanian to ISO standards
* Added menu items to set to Dutch,  Farsi, Albanian and Italian (these three are rather incomplete)

* changed log error to warn

* fixed ipcBackend notifications

* Fixing wallet tab insertion (#911)

* small cleanup

* added admin:true permission

* removed webview duplicate

* Fully automated UI testing (#788)


* got app launching via test

* work towards testing - private net integration

* change how we name options to pass directly to geth, so that test suite works

* refactor preloader scripts

* cleanly quitting the app at the end of tests

* trying to get electron working properly, upgrade to 1.2.2

* upgrade spectron

* work towards getting tests to work

* dont use NODE_ENV var as Meteor production uses it

* fixed eth start, but crash is not graceful

* better socket connection logic, remove master ps logic for eth

* fix splash screen display of state text for eth

* better gulp download plugin, remove master passwd stuff once and for all

* got basic test working

* update travis build

* trigger build

* update readme, travis

* fix gulp-download-stream use

* fix build command

* testing account creation

* added account creation test

* only do a shallow clone

* final deposit test

* should be able to find geth executable now

* adds the wallet as the default tab (#924)

* Text updates

Fix typos and make writing more clear and accurate

* Capitalization consistency

* adds the wallet as the default tab

* always upsert

* Update version to 0.8 (#930)

* update to 0.8

* fixes menu margin clipping

* remove comma

* work towards new minimonogo sync

* fix ipcpath bug

* got basic persistence working

* minor error

* refactor sendTransaction error alerts (#959)

* "Change language" translated in mist.nb.i18n.json (#953)

"Change language" translated in mist.nb.i18n.json

* timer delay on menu refreshing from tab updates

* rename 'passwordError' to 'wrongPassword' for consistency (#951)

* sync not specific to a window

* refactor to enable access to mongo sync from all windows

* update .meteor paths on .gitignore (#977)

* DAO fork support (#970)

* created DAO fork dummy code

* text update

* added node flags choice 

* changed DAO fork texts

* disable eth-node switch until hardfork is supported (#975)

* disable eth-node until hardfork is supported

* remove test function

* reworded the disclaimer link
alexvandesande pushed a commit that referenced this pull request Jul 23, 2016
Merging of Choose account popup #814

* Creating change account modals

* connect popup

* fixing typo

* connect account popup

* Straightens checkbox on connect account modal

* Connect account popup interface

* fixing scroll gradient on choose account popup

* Informing account names on connect popup

* Adding animations on account and dapp info

* Improvements on connect popup

* Tweaking URL breakdown to show arrows when necessary

* Fixing edgy cases on breadcrumb

* Removing browserBar interface

* Removing browser bar fold-down element

* Vertical flexbox - window stretches appropriately

* Popup updates

* change dapp style and icon (#1)

* more working code

* Reload permissions and code cleaning

* Website placeholder icon

* Cleaning code

* Unpin tab

* Not showing remove button on wallet tab

* Merge Hard Fork choice - Mist into master (#979)

* bump version to 0.7.6

* updated os timesync

* adding setting to change UI language (fixes #813) (#896)

* simple language setting menu

* use i18n label in menu

* simple language setting menu

* use i18n label in menu

* change language of menu bar by triggering 'backendAction_setLanguage'

* adds translations for language names

* remove duplicate 'gulp update-nodes' (#897)

* add i18n entry for OSX's 'Services' (#887)

* add i18n entry for OSX's 'Services'

* update missing osx menu strings

* update-popup-window: prevent maximize (#885)

* update-popup-window prevent maximize

* add resizable:false

* added shrinkwrap (#903)

* fixed webview tagd and updated electrong to 1.2.5 (#905)

* Fixes the node start IPC connection (#841)

* fixed eth start, but crash is not graceful

* better socket connection logic, remove master ps logic for eth

* fix splash screen display of state text for eth

* better gulp download plugin, remove master passwd stuff once and for all

* Adds Toggle password visibility to on boarding screen (#909)

* add password visibilty toggle to onboarding

* replaced 'showPassword' with 'passwordInputType'

* changed id into class

* 18n cleanup (former #889) (#910)

* Renamed Korean and Albanian to ISO standards
* Added menu items to set to Dutch,  Farsi, Albanian and Italian (these three are rather incomplete)

* changed log error to warn

* fixed ipcBackend notifications

* Fixing wallet tab insertion (#911)

* small cleanup

* added admin:true permission

* removed webview duplicate

* Fully automated UI testing (#788)


* got app launching via test

* work towards testing - private net integration

* change how we name options to pass directly to geth, so that test suite works

* refactor preloader scripts

* cleanly quitting the app at the end of tests

* trying to get electron working properly, upgrade to 1.2.2

* upgrade spectron

* work towards getting tests to work

* dont use NODE_ENV var as Meteor production uses it

* fixed eth start, but crash is not graceful

* better socket connection logic, remove master ps logic for eth

* fix splash screen display of state text for eth

* better gulp download plugin, remove master passwd stuff once and for all

* got basic test working

* update travis build

* trigger build

* update readme, travis

* fix gulp-download-stream use

* fix build command

* testing account creation

* added account creation test

* only do a shallow clone

* final deposit test

* should be able to find geth executable now

* adds the wallet as the default tab (#924)

* Text updates

Fix typos and make writing more clear and accurate

* Capitalization consistency

* adds the wallet as the default tab

* always upsert

* Update version to 0.8 (#930)

* update to 0.8

* fixes menu margin clipping

* remove comma

* work towards new minimonogo sync

* fix ipcpath bug

* got basic persistence working

* minor error

* refactor sendTransaction error alerts (#959)

* "Change language" translated in mist.nb.i18n.json (#953)

"Change language" translated in mist.nb.i18n.json

* timer delay on menu refreshing from tab updates

* rename 'passwordError' to 'wrongPassword' for consistency (#951)

* sync not specific to a window

* refactor to enable access to mongo sync from all windows

* update .meteor paths on .gitignore (#977)

* DAO fork support (#970)

* created DAO fork dummy code

* text update

* added node flags choice 

* changed DAO fork texts

* disable eth-node switch until hardfork is supported (#975)

* disable eth-node until hardfork is supported

* remove test function

* reworded the disclaimer link

* Syncing Tabs after account connection

* Enable Tabs sync and persistence in all windows (#2)

* sync not specific to a window

* refactor to enable access to mongo sync from all windows

* Pin/unpin Tabs

* Adding show password on Create account popup

* change remove button animation

* add globe image as default non-icon
@evertonfraga evertonfraga deleted the DAO-fork branch October 5, 2016 21:14
@lock
Copy link

lock bot commented Mar 31, 2018

This thread has been automatically locked because it has not had recent activity. Please open a new issue for related bugs and link to relevant comments in this thread.

@lock lock bot locked and limited conversation to collaborators Mar 31, 2018
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.