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

[WIP] Implement BIP32 HD account class #27

Closed
wants to merge 3 commits into from

Conversation

dilatebrave
Copy link

What was wrong?

Issue #24

How was it fixed?

  • Implement a new account class HDAccount which exposes the same API (potentially via subclassing) as LocalAccount,
  • Additional APIs for generating child accounts.
  • Test cases should include any public test vectors that are available as part of the spec.

Cute Animal Picture

b1178afe6c24d9a36cb5dfcfed630e14

@carver
Copy link
Contributor

carver commented Jun 7, 2018

#28 has some thoughts about what the abstract class looks like. I wouldn't recommend subclassing LocalAccount. Just meet the interface for now. If there's any code duplication, we can pull it into utility methods.

@carver
Copy link
Contributor

carver commented Jun 7, 2018

#29 has a preview of the class that should be subclassed: BaseAccount

Edit: you can rebase and subclass BaseAccount now

@vs77bb
Copy link

vs77bb commented Jun 14, 2018

@dilatebrave Are you still working here? Hope you're doing well 🙂

@dilatebrave
Copy link
Author

@vs77bb --- i'm sill working on this but got stuck at signing trans with the extended key. I'm lacking knowledge at this area so it took me sometime to read more

BIP32_HARDEN = 0x80000000 # choose from hardened set of child keys
CURVE_GEN = ecdsa.ecdsa.generator_secp256k1
CURVE_ORDER = CURVE_GEN.order()
EX_MAIN_PRIVATE = [ codecs.decode('0488ade4', 'hex') ] # Version strings for mainnet extended private keys
Copy link
Member

Choose a reason for hiding this comment

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

Can you link to a reference for where this magic number comes from?

Copy link
Author

Choose a reason for hiding this comment

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

These numbers come from the bip https://github.com/bitcoin/bips/blob/master/bip-0032.mediawiki
I don't know how to specify the line in that file so please go there and search for the numbers >.<

Copy link
Contributor

Choose a reason for hiding this comment

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

https://github.com/bitcoin/bips/blob/master/bip-0032.mediawiki#serialization-format <- that link is sufficient, and should go in a source comment

@@ -0,0 +1,3 @@
{
"python.pythonPath": "${workspaceFolder}/venv/bin/python"
Copy link
Member

Choose a reason for hiding this comment

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

This looks like something that needs to be removed and added to the .gitignore

Copy link
Author

Choose a reason for hiding this comment

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

yes, that needs to be removed

import ecdsa

from hashlib import sha256
from ecdsa.curves import SECP256k1
Copy link
Member

Choose a reason for hiding this comment

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

We prefer to be flexible about what implementation people choose for this sort of thing. We have a library called eth-keys which implements the secp256k1 operations for public/private keys and signatures that I believe is going to be our preferred way to do this.

However, I'm not entirely sure it will do everything that you need for HD keys so I'm interested in your feedback on what is missing/needed if that is the case.

Copy link
Author

Choose a reason for hiding this comment

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

i didn't know about that library. i will try it out ^^

@vs77bb
Copy link

vs77bb commented Jun 18, 2018

Hi @dilatebrave mind responding to comments back here?

@dilatebrave
Copy link
Author

@vs77bb --- thanks for notifying me
@pipermerriam --- my apologies for the slow reply

@pipermerriam
Copy link
Member

@dilatebrave no apologies necessary. Thank you for taking this on.

carver added a commit to carver/eth-account that referenced this pull request Apr 21, 2020
Skip venv in template filler & print progress
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.

4 participants