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

Dynamic merchant wallets #6

Closed
mattpass opened this issue Apr 17, 2018 · 11 comments
Closed

Dynamic merchant wallets #6

mattpass opened this issue Apr 17, 2018 · 11 comments
Labels
help wanted Extra attention is needed question Further information is requested

Comments

@mattpass
Copy link
Contributor

mattpass commented Apr 17, 2018

Lots of notes following discussion with @ch4ot1c

Terminology

  • xPub is public key from which you can derive wallet addresses
  • xPrv is the private key from which an xPub key can be created, it is the HD (Hierarchical Deterministic) wallet

Code - generate-keys.js

'use strict';

const Mnemonic = require('bitcore-mnemonic');

console.log('Generating your Master Private Key - Seed Words (BIP39) - this will only be shown ONCE!');

var seed = new Mnemonic(Mnemonic.Words.ENGLISH);
var xprv = seed.toHDPrivateKey('OPTIONAL PASSWORD');
console.log('xprv: ', seed.toString());
var xpub = xprv.xpubkey.toString();
console.log('xpub (corresponds to addrs; use this as input when running): ', xpub);

produces:

jsl@JSLDESKTOP:/mnt/c/Users/Jon/Documents/GitHubProjects/store-demo$ node generate-keys.js
Generating your Master Private Key - Seed Words (BIP39) - this will only be shown ONCE!
xprv:  surprise guess few seven hope roast main pass transfer rotate neutral bitter
xpub (corresponds to addrs; use this as input when running):  xpub661MyMwAqRbcFVn4h1NdbGKJ59QcCi4D2RkbmAhK7h7svN5PwxdAgpGNzzceEjtaJHfLJHBbuMDbATyvnad5FLPhoWqjYP4u6UUKdRursNX

Notice we are using bitcore-mnemonic here - uses bip39 - apparently the bip32 implementation (and the old one from bitcore-lib, at that) is NON STANDARD / COMPLIANT

@mattpass mattpass added help wanted Extra attention is needed question Further information is requested v1 labels Apr 18, 2018
@mattpass
Copy link
Contributor Author

mattpass commented Apr 25, 2018

Generating addresses
Can use HDPrivateKey.deriveChild

However, this this is related to their erroneous bip32 implementation that is in our current code; so @ch4ot1c was looking to see what we'd need to fix it

References re erroneous code:
bitpay/bitcore-lib#97 - DanFinlay one of the main ETH guys yelling at them
https://github.com/bitpay/copay/issues/5667 - Trezor CTO yelling at them still today
https://github.com/bitpay/bitcore-lib/commits/master/lib/hdprivatekey.js history

Notes: at worst we will need to derive() the child keys with a non-fkd version of HDPrivateKey (would cherrypick those changes, or try current bitcore-lib (yikes - at least its already btcpified at ch4ot1c/bitcore-lib), or just use/copy over the part of the fixed code we need) (and yes rebuild the .js)

@mattpass
Copy link
Contributor Author

mattpass commented Apr 25, 2018

There is no 'one solution' here and it depends on the merchants competence, will and ability to set up more secure solutions. They may wish to start off simple and become more secure over time.

Plans

Static Wallet Address
They can simply use their wallet address within the widget. This isn't ideal as it's best to use dynamically generated wallet addresses from an xPub, which allows you to derive addresses to use, then you can use 1 new wallet address per transaction and transfer overall funds from an xPub and addresses underneath if you wish. Still, static wallet addresses are an option.

Dynamic Wallet Addresses
To operate under this more recommended dynamic setup, you will need an xPrv and xPub created from it. So, need to establish those. In preference order:

  • Provide Node script for them to run and generate if they run full node server with bitcore
  • Generate offline from paper wallet GUI
  • From within Vendor site, allow them to click button to generate, is only ever displayed onscreen
  • Also allow them to generate from Merchant support within widget

...provide info on what these 4 choices mean so they can make informed decisions

With an xPrv and xPub generated, they need to save xPrv to a safe location and xPub can be utilised within site, via one of these different choices to get a new address from it, again in recommendation order:

<?php
$address = file_get_contents(
"https://vendor-api.btcprivate.org/get-address?from=xpub&xpub=123456789"
);
?>
  • Use us, client side: Call upon function in widget or via your own JS perhaps, eg
address = btcpWidget.getAddressFromXPub(123456789);

...again, provide info on what these 3 choices mean so they can make informed decisions

@mattpass
Copy link
Contributor Author

Note that the above xPrv and xPub generation is from the perspective of someone who isn't running their own full node server and is using our setup. If they run their own, we will provide the functionality baked in to handle all of this for them.

@mattpass
Copy link
Contributor Author

This is essentially the code we need to generate and work with xPrv and xPub and derive the addresses from xPub under a number of different contexts:

https://bitcore.io/api/lib/hd-keys

@mattpass
Copy link
Contributor Author

Finally, @ch4ot1c notes:
Shielded addresses don’t support HD keys, to complicate things further. So eventually we will need abstraction based on ‘per address/privkey’ , not solely indices of an HD key’s series of private keys
Should test what we’ve got with z addrs some time

@ch4ot1c
Copy link
Collaborator

ch4ot1c commented Apr 25, 2018

This is a good overview. Some followup:

  • I think we can remove the example Use us, server side: Get from API we provide, eg via PHP:, because of the bower-bundled bitcore-lib-btcp.js that is available to the client/browser.

  • Because of the historical code issues regarding BIP32 and bitcore, we need to sync up with their latest, at least regarding the derive() methods, and test that derived keys and addresses are indeed as expected. bitpay/bitcore-mnemonic is their up-to-date reference code, which uses the latest bitpay/bitcore-lib.

  • Let's say that xprv generation must happen offline, and the xpub must be input / remain on the merchant server, and see how far we get. This is the recommended approach. The presented invoice should only contain the merchant's address being watched for this particular invoice, as expected.

  • As an aside, the static single-address case works well / has been tested with the current widget. The merchant's backend (database) is never exempt of responsibility to handle & route these invoice payments through its accounting + shipping logic.

@mattpass
Copy link
Contributor Author

@ch4ot1c Notes on those 4 items:

  • If we don't offer server side retrival of address, it reduces their options down to running their own Node code (not always possible) or retrieving from JS (some may say less secure). Having an API method helps by providing a middleground that can be hit from any language and server side too, so a good option?
  • Cool, sounds like an update there is due
  • I worry insisting it happens offline, we'll immediately run into most of the merchants not being able to do this and a huge dropoff. Agree that having xPub clientside is not ideal as potentially less secure (see first point)
  • Static address is definitely a viable option and I suspect what most merchants will go for. Need to make dynamic wallet addresses as painless and secure as possible to setup to encourage more to go that way.

@daniel-farina
Copy link
Contributor

What do you guys think of going with the static wallet for the Beta testers?

@mattpass
Copy link
Contributor Author

@daniel-farina I'd agree it's a good idea for v1. @ch4ot1c actually suggested that earlier and cover donation widget only. I think static wallet for buy or donate but soon after v1 cover the above.

@mattpass mattpass added v2 and removed v1 labels Apr 26, 2018
@mattpass
Copy link
Contributor Author

mattpass commented May 4, 2018

New issue added to provide a plan for us to move from static to dynamic wallets as the only option that may be used: https://github.com/mattpass/btcp-widget/issues/56

@mattpass
Copy link
Contributor Author

We pushed ahead on dynamic wallets and so this issue can be closed.

@mattpass mattpass removed the v2 label May 29, 2018
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
help wanted Extra attention is needed question Further information is requested
Projects
None yet
Development

No branches or pull requests

3 participants