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

Ethereum hierarchy for deterministic wallets #601

Merged
merged 8 commits into from
Mar 18, 2019

Conversation

Arachnid
Copy link
Contributor

@Arachnid Arachnid commented Apr 14, 2017

This EIP defines a logical hierarchy for deterministic wallets based on BIP32, the purpose scheme defined in BIP43 and eip-draft-ethereum-purpose.

This EIP is a particular application of eip-draft-ethereum-purpose.


Hardened derivation is used at this level.

Software should prevent a creation of an account if a previous account does not have a transaction history (meaning its address has not been used before).
Copy link
Contributor

Choose a reason for hiding this comment

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

What is the purpose of this? What if I create a wallet, post the private key online (perhaps as an accident, as part of troubleshooting, testing, etc.) and then want to create a new (secure) wallet? This rule forces me to submit a transaction (paying gas) just to make that wallet valid so I can move on to getting a new one.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Perhaps I should reword this to include the possibility that the same software is aware of a previous use for the wallet?

The issue here is that if users can create wallets arbitrarily 'far away' from the previous one, then anything importing it again later won't be able to find those wallets by enumeration.

Copy link

Choose a reason for hiding this comment

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

Or you might want to add a gap concept similar to BIP 44 here https://github.com/bitcoin/bips/blob/master/bip-0044.mediawiki#Address_gap_limit

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes, I'll amend it to add a gap.

Hardened derivation is used at this level.

### Subpurpose
Subpurpose is set to the EIP number specifying the remainder of the BIP32 derivation path. For paths following this EIP specification, the number assigned to this EIP is used.
Copy link
Contributor

Choose a reason for hiding this comment

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

Question: So the Purpose is going to be a constant (TBD) that generally defines this as a derivation path following a new hierarchy for Ethereum-based chains, while the Subpurpose will serve as the type/id of the derivation path. Or as the version?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

That's right. The subpurpose has the same role as the 'purpose' in BIP43/BIP44 paths - it lets us define new sorts of derivation paths with EIPs and without having to consult anyone else.

Copy link
Contributor

Choose a reason for hiding this comment

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

I see. Thanks. That is clear now. Perhaps add one more sentence there to make it clear to other readers too?

What you just told me or a variaton of thereof would be enough I think.

it lets us define new sorts of derivation paths with EIPs and without having to consult anyone else.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I'll do that, thanks.

@jprichardson
Copy link

NACK. I don't see the purpose of this. So many wallets already implement other derivation paths in production. Any wallet can also just use any purpose code that doesn't touch the proposed allocated "reserved for SLIPs". Why go against the grain with what wallets are already doing?

@MicahZoltu
Copy link
Contributor

So many wallets already implement other derivation paths in production

@jprichardson The first part of the answer is the pluralization of path in that quote. There is not consistency among wallets and this causes a notable amount of user confusion.

The second part of the answer is that BIP44 doesn't really make sense for Ethereum, and by far the single most common use case for HD wallets is "one mnemonic, many disassociated addresses". This results in most of the BIP44 segments going unused. So standardizing on BIP44 doesn't really make sense here.

The third part of the issue is that it would be nice if we followed BIP43, since that isn't really Bitcoin specific. However, trying to get BIPs pushed through for Ethereum specific things (or even SLIPs) is a non trivial task. The idea here is to stick to BIP43 for general cross-chain compatibility reasons (and it makes sense in Ethereum) but just have one "Ethereum" branch point that allows for all future BIP32 derivation paths in the Ethereum ecosystem to only have to go through the EIP, rather than having to go through the BIP.

@jprichardson
Copy link

The second part of the answer is that BIP44 doesn't really make sense for Ethereum, and by far the single most common use case for HD wallets is "one mnemonic, many disassociated addresses". This results in most of the BIP44 segments going unused. So standardizing on BIP44 doesn't really make sense here.

BIP44 can make perfect sense for Ethereum addresses and keys. It already is as we see many wallets already using (I'm well aware of the diversification of paths) it for all sorts blockchain assets.

I can see why someone would want to use BIP43 for other uses cases other than addresses, but BIP44 being an implementation of BIP43, it works well for keys and addresses - my product is proof of that.

@MicahZoltu
Copy link
Contributor

BIP 44: m / purpose' / coin_type' / account' / change / address_index

  • purpose: still makes sense (that is BIP43)
  • coin_type: makes a little sense... maybe, except adding a new coin type requires going through the SLIP process which is foreign to Ethereum developers. Also, tokens are a dime a dozen on Ethereum and requiring every token get a new entry into SLIP44 is unrealistic. Finally, in Ethereum using different derivation paths for different tokens doesn't make a lot of sense for the average end-user. It is much simpler to put your REP, MKR and ETH all in one wallet. If a user wants, they can create multiple wallets (one for each token type) but there is really no compelling reason to have that be baked in to the simple derivation path (see EIP Ethereum purpose allocation and path scheme for deterministic wallets #600 for how to add more complex derivation paths).
  • account: This differs from address_index only in the fact that it is hardened while address_index is not. Functionally, they serve basically the same purpose. It has never been clear to me why you wouldn't just want address_index to be hardened and call it a day. People have made arguments about accounting and such but in reality almost no one ever does this. Unfortunately, the result is that some wallets increment the account and others increment the address_index, leading to failed interop and poor end user experience.
  • change: This doesn't make sense in Ethereum, it is a UTXO thing.
  • address_index: as discussed above, the usage of this is unclear. Presumably it adds value being un-hardened but for most people it is unnecessary. This is why this EIP combines them into a single hardened incrementing wallet. One can easily create a new derivation path EIP based on EIP Ethereum purpose allocation and path scheme for deterministic wallets #600 that has both hardened and non-hardened. The goal here is to have something that is simple, rather than defaulting to all-the-complexity.

This EIP is meant to define a very simple BIP32 and BIP43 derivation path. The idea is that this is the default for the novice user that doesn't need anything complex and wants their stuff to "just work" in all of the various wallets of the future. This EIP is not attempting to solve all possible HD wallet problems and people are encouraged to create new EIP derivation path schemes based on EIP #600. This particular derivation path will be the default derivation path for EIP #602 mnemonics (which are mnemonics that have derivation path style baked in). The idea is that a user only has to remember their mnemonic, they don't also have to remember their derivation path.

@jprichardson
Copy link

Before I dig into the other statements, why would a token have to register a coin_type? Since ERC20 tokens are on Ethereum and require gas and cannot join inputs like the UTXO model, they must reside on an address with Ether. So why would they reside on their own respective address?

@MicahZoltu
Copy link
Contributor

One could argue the same thing for why you should have a separate path segment for coin_type at all. It really doesn't provide any value that I can tell. Sure, it may make less sense for tokens than it does for coins that are used for gas but that doesn't really argue for keeping coin_type for Ethereum derivation paths.

Before you spend too much effort arguing each of the BIP43 segments, keep in mind that this proposal is about simplicity. The first segment is so we are BIP43 compatible. The next segment is so we can have future expansion (EIP #600), and the last (and only segment that is actually part of this EIP) is to allow users multiple addresses per mnemonic. Anything more than that is beyond the scope of this EIP as it increases derivation path complexity.

Note that at least in the Ethereum space, despite everyone using BIP44 and BIP44-like derivation paths none of the segments are used in an interesting way outside of whatever one is indexed. There also isn't consistency with regards to the coin types that are in SLIP44 (Testnet, ETH, ETC) despite them being well defined. This illustrates the need for simplicity in my mind.

@Arachnid
Copy link
Contributor Author

This is a courtesy notice to let you know that the format for EIPs has been modified slightly. If you want your draft merged, you will need to make some small changes to how your EIP is formatted:

  • Frontmatter is now contained between lines with only a triple dash ('---')
  • Headers in the frontmatter are now lowercase.

If your PR is editing an existing EIP rather than creating a new one, this has already been done for you, and you need only rebase your PR.

In addition, a continuous build has been setup, which will check your PR against the rules for EIP formatting automatically once you update your PR. This build ensures all required headers are present, as well as performing a number of other checks.

Please rebase your PR against the latest master, and edit your PR to use the above format for frontmatter. For convenience, here's a sample header you can copy and adapt:

---
eip: <num>
title: <title>
author: <author>
type: [Standards Track|Informational|Meta]
category: [Core|Networking|Interface|ERC] (for type: Standards Track only)
status: Draft
created: <date>
---

@MicahZoltu
Copy link
Contributor

@Arachnid I propose we move this to Last Call (after you fixup the necessary changes above).

@Arachnid
Copy link
Contributor Author

@MicahZoltu Thanks for the ping.

I think this probably needs more discussion, primarily around whether it's likely to get adoption given the prevalence of bitcoin-derived HD paths at the moment.

@MicahZoltu
Copy link
Contributor

In that case, I recommend a discussion link and merge as draft.

EIPS/eip-601.md Outdated
category : ERC
status: Draft
created: 2017-04-13
replaces: 84
Copy link
Contributor

Choose a reason for hiding this comment

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

Needs discussion link.

EIPS/eip-601.md Outdated
eip: 612
title: Ethereum hierarchy for deterministic wallets
author: Nick Johnson (@arachnid), Micah Zoltu (@micahzoltu)
type: Standard Track
Copy link
Contributor

Choose a reason for hiding this comment

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

Standard_s_ Track

EIPS/eip-601.md Outdated
@@ -0,0 +1,80 @@
---
eip: 612
Copy link
Contributor

Choose a reason for hiding this comment

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

I'd be fine with numbering this EIP 601

@nicksavers nicksavers merged commit 36cc935 into ethereum:master Mar 18, 2019
ilanolkies pushed a commit to ilanolkies/EIPs that referenced this pull request Nov 12, 2019
Woodpile37 added a commit to Woodpile37/EIPs that referenced this pull request Nov 2, 2023
<p>This PR was automatically created by Snyk using the credentials of a
real user.</p><br /><h3>Snyk has created this PR to fix one or more
vulnerable packages in the `npm` dependencies of this project.</h3>



#### Changes included in this PR

- Changes to the following files to upgrade the vulnerable dependencies
to a fixed version:
    - assets/eip-4675/package.json



#### Vulnerabilities that will be fixed
##### With an upgrade:
Severity | Issue | Breaking Change | Exploit Maturity

:-------------------------:|:-------------------------|:-------------------------|:-------------------------
![medium
severity](https://res.cloudinary.com/snyk/image/upload/w_20,h_20/v1561977819/icon/m.png
"medium severity") | Open Redirect
<br/>[SNYK-JS-GOT-2932019](https://snyk.io/vuln/SNYK-JS-GOT-2932019) |
No | No Known Exploit




<details>
  <summary><b>Commit messages</b></summary>
  </br>
  <details>
    <summary>Package name: <b>solidity-coverage</b></summary>
    The new version differs by 41 commits.</br>
    <ul>
<li><a
href="https://snyk.io/redirect/github/sc-forks/solidity-coverage/commit/0a33e13b166651bf8ce94d425d0abf590fed784c">0a33e13</a>
0.8.0</li>
<li><a
href="https://snyk.io/redirect/github/sc-forks/solidity-coverage/commit/4c63612f83f6b230810f5728f4c1315cfe28cef2">4c63612</a>
Add hardhat to peerDependencies (ethereum#722)</li>
<li><a
href="https://snyk.io/redirect/github/sc-forks/solidity-coverage/commit/9ce20ff4bbc3c7e015ac0a5574cc1ab61cfc216b">9ce20ff</a>
Typo / Grammar fix. (ethereum#738)</li>
<li><a
href="https://snyk.io/redirect/github/sc-forks/solidity-coverage/commit/204a5ebaa4be69fd420f9d3851dd518d9072ece9">204a5eb</a>
Added a section for the report location. (ethereum#739)</li>
<li><a
href="https://snyk.io/redirect/github/sc-forks/solidity-coverage/commit/ed3d5041764e1b6a2270d0b910a3bfaf1a9d3c01">ed3d504</a>
Fix README for v0.8 release</li>
<li><a
href="https://snyk.io/redirect/github/sc-forks/solidity-coverage/commit/05ab320fbccb7bc607d9e5ada4a3261c46a273e8">05ab320</a>
Fixes for Hardhat 2.11.0 (ethereum#740)</li>
<li><a
href="https://snyk.io/redirect/github/sc-forks/solidity-coverage/commit/bc7d07623ec4bd71a2ce4c8ecb4a872af613c8ef">bc7d076</a>
0.8.0 Additional Coverage Measurements &amp; Restructure (Merge)</li>
<li><a
href="https://snyk.io/redirect/github/sc-forks/solidity-coverage/commit/a7db2fedc447f318da0424a162e58a2475072dc1">a7db2fe</a>
More README changes</li>
<li><a
href="https://snyk.io/redirect/github/sc-forks/solidity-coverage/commit/16367d15389ade4f8cc5d9c17ccac2cfd0962e14">16367d1</a>
Remove truffle files from project</li>
<li><a
href="https://snyk.io/redirect/github/sc-forks/solidity-coverage/commit/26898c1ad9f81ae089953fa0a824ab01b7caef08">26898c1</a>
Remove Builder-E2E test</li>
<li><a
href="https://snyk.io/redirect/github/sc-forks/solidity-coverage/commit/8ea8ec93d923a9722a3549e5788d5aaa81b3a41a">8ea8ec9</a>
Fix true/false scoped method definition function visibilities</li>
<li><a
href="https://snyk.io/redirect/github/sc-forks/solidity-coverage/commit/21ca46e2de3e1fe27d120b38fe7383a0a90792b1">21ca46e</a>
Temporarily skip truffle integration tests</li>
<li><a
href="https://snyk.io/redirect/github/sc-forks/solidity-coverage/commit/22992e1c19825d27de665762ffb8798a6b3195fe">22992e1</a>
Fix constructor test</li>
<li><a
href="https://snyk.io/redirect/github/sc-forks/solidity-coverage/commit/cf126ea7311145214fde7f7538bf4428e168b834">cf126ea</a>
Fix assert tests</li>
<li><a
href="https://snyk.io/redirect/github/sc-forks/solidity-coverage/commit/0ba3f11701097693674dab8cf831fac9af113fb0">0ba3f11</a>
Remove more buildler things</li>
<li><a
href="https://snyk.io/redirect/github/sc-forks/solidity-coverage/commit/d57a1315ec73ee0f0a26a2e64212a6055d51c3ab">d57a131</a>
Remove buidler</li>
<li><a
href="https://snyk.io/redirect/github/sc-forks/solidity-coverage/commit/3bcec941ecbeac2fd385c3e17886e7e02c66c2ad">3bcec94</a>
Fix rebase errors &amp; regenerate yarn.lock</li>
<li><a
href="https://snyk.io/redirect/github/sc-forks/solidity-coverage/commit/88c1d0039033cfbb38311c6430ebdc41d5480c03">88c1d00</a>
Fix loops, modifiers, options and statements tests</li>
<li><a
href="https://snyk.io/redirect/github/sc-forks/solidity-coverage/commit/0deb0013cbf9e552a133c18d46e4b6536700982f">0deb001</a>
Fix if/else tests</li>
<li><a
href="https://snyk.io/redirect/github/sc-forks/solidity-coverage/commit/29c0fdda7fcc438861bcbb4ad1c0cb26ffbd08fb">29c0fdd</a>
Fix constructor keyword test</li>
<li><a
href="https://snyk.io/redirect/github/sc-forks/solidity-coverage/commit/d4e8536aa3087f40853f2b4c168dfa2b4e9c6e7a">d4e8536</a>
Update tests for adjusted statement coverage</li>
<li><a
href="https://snyk.io/redirect/github/sc-forks/solidity-coverage/commit/3edfd2537180e0f78ba769b077664a3159ba0b2a">3edfd25</a>
Stop injecting statement coverage into conditionals</li>
<li><a
href="https://snyk.io/redirect/github/sc-forks/solidity-coverage/commit/7eb94a93f3b2a2c68535051758c688470de9a1e2">7eb94a9</a>
Update @ solidity-parser/parser to 0.14.1</li>
<li><a
href="https://snyk.io/redirect/github/sc-forks/solidity-coverage/commit/e9133d719ca3969b63ffa777a13a3424f886fbc8">e9133d7</a>
Generate mocha JSON output with --matrix (ethereum#601)</li>
    </ul>

<a
href="https://snyk.io/redirect/github/sc-forks/solidity-coverage/compare/0e17f18e80e46c598097ab89e32379cb6ffd7e33...0a33e13b166651bf8ce94d425d0abf590fed784c">See
the full diff</a>
  </details>
</details>






Check the changes in this PR to ensure they won't cause issues with your
project.



------------



**Note:** *You are seeing this because you or someone else with access
to this repository has authorized Snyk to open fix PRs.*

For more information: <img
src="https://api.segment.io/v1/pixel/track?data=eyJ3cml0ZUtleSI6InJyWmxZcEdHY2RyTHZsb0lYd0dUcVg4WkFRTnNCOUEwIiwiYW5vbnltb3VzSWQiOiI5MjU1NWU1OC1iZGQ2LTQ5MjctOWVlMy1hZDlkMTE1NDFmNWIiLCJldmVudCI6IlBSIHZpZXdlZCIsInByb3BlcnRpZXMiOnsicHJJZCI6IjkyNTU1ZTU4LWJkZDYtNDkyNy05ZWUzLWFkOWQxMTU0MWY1YiJ9fQ=="
width="0" height="0"/>
🧐 [View latest project
report](https://app.snyk.io/org/woodpile37/project/f0dcf1c9-ecf1-445b-bc07-e8f73c595f54?utm_source&#x3D;github&amp;utm_medium&#x3D;referral&amp;page&#x3D;fix-pr)

🛠 [Adjust project
settings](https://app.snyk.io/org/woodpile37/project/f0dcf1c9-ecf1-445b-bc07-e8f73c595f54?utm_source&#x3D;github&amp;utm_medium&#x3D;referral&amp;page&#x3D;fix-pr/settings)

📚 [Read more about Snyk's upgrade and patch
logic](https://support.snyk.io/hc/en-us/articles/360003891078-Snyk-patches-to-fix-vulnerabilities)

[//]: #
(snyk:metadata:{"prId":"92555e58-bdd6-4927-9ee3-ad9d11541f5b","prPublicId":"92555e58-bdd6-4927-9ee3-ad9d11541f5b","dependencies":[{"name":"solidity-coverage","from":"0.7.22","to":"0.8.0"}],"packageManager":"npm","projectPublicId":"f0dcf1c9-ecf1-445b-bc07-e8f73c595f54","projectUrl":"https://app.snyk.io/org/woodpile37/project/f0dcf1c9-ecf1-445b-bc07-e8f73c595f54?utm_source=github&utm_medium=referral&page=fix-pr","type":"auto","patch":[],"vulns":["SNYK-JS-GOT-2932019"],"upgrade":["SNYK-JS-GOT-2932019"],"isBreakingChange":false,"env":"prod","prType":"fix","templateVariants":["updated-fix-title"],"priorityScoreList":[null],"remediationStrategy":"vuln"})

---

**Learn how to fix vulnerabilities with free interactive lessons:**

🦉 [Open
Redirect](https://learn.snyk.io/lesson/open-redirect/?loc&#x3D;fix-pr)
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.

6 participants