Some typos fixed and removed electrum-merchant requirement #1631
Conversation
lib/base_wizard.py
Outdated
| @@ -137,7 +137,7 @@ def import_addresses_or_keys(self): | |||
| title = _("Import Bitcoin Addresses") | |||
| message = _("Enter a list of Bitcoin Cash addresses (this will create a watching-only wallet), or a list of private keys.") | |||
| if bitcoin.is_bip38_available(): | |||
| message += " " + _("BIP38 encrpted keys are supported.") | |||
| message += " " + _("BIP38 encyrpted keys are supported.") | |||
There was a problem hiding this comment.
Oops. That was my typo. Thanks for this.
There was a problem hiding this comment.
Although you continued to typo it -- encrypted -- you have encyrpted :) Typos are contagious sometimes. :)
There was a problem hiding this comment.
Haha, that's hilarious. I'm so done with typing.
There was a problem hiding this comment.
Yeah there is a known phenomenon that typos are contagious. Thanks for catching this. I do this more than I like to admit in my user-facing messages.
| @@ -820,10 +820,11 @@ def help(self): | |||
| config_variables = { | |||
|
|
|||
| 'addrequest': { | |||
| 'requests_dir': 'directory where a bip70 file will be written.', | |||
| 'ssl_privkey': 'Path to your SSL private key, needed to sign the request.', | |||
| 'requests_dir': 'Directory where BIP70 payment request files will be written.', | |||
| if not os.path.exists(path): | ||
| print("Requests directory not configured.") | ||
| print("You can configure it using https://github.com/spesmilo/electrum-merchant") | ||
| sys.exit(1) |
There was a problem hiding this comment.
Ok -- I still have to check if other parts of the code will crash if they don't find the requests_dir when they expected it (namely the stuff in websockets.py and commands.py has to at least be made resilient to the absence of the dir and should not throw up a stack trace).
I do agree checking for index.html here is kind of dumb.
So you are 100% sure no check here is needed?
There was a problem hiding this comment.
Well, as far as I know, electrum-merchant just installs its script there with some other jquery files and stuff like that. On the one hand, enforcing this restricts users to serving their payment requests with electrum-merchant. On the other hand, not enforcing it doesn't break electrum merchant (the user will simply get a broken link when they visit the index_url if electrum-merchant is not installed) but allows users to have a requests_dir and also serve the PR with any URL they want.
There was a problem hiding this comment.
Gotcha.
The thing is wallet.py also refers to that index.html file:
Line 1940 in 1f436b5
So -- what do we do?
There was a problem hiding this comment.
Although if you want to continue to support https://electroncash.readthedocs.io/en/latest/old/merchant.html then that doc should be updated to indicate the need to either install electrum-merchant OR serve the PR some other way.
There was a problem hiding this comment.
It should also explain how to specify BCH when installing electrum-merchant. But imho that static html tool is not great because users can simply spoof their own payments by sending a 'paid' message through the websocket.
There was a problem hiding this comment.
Gotcha.
The thing is
wallet.pyalso refers to that index.html file:Line 1940 in 1f436b5
So -- what do we do?
Yeah, because that index_url is included as part of the PR json file. But I don't know that it ever gets read from anywhere after that. Does it just sit there doing nothing? I think so. When the user's webserver gets a request for mydomain.com/requests_dir/index.html?id=FOO, the PR will be accessed by the javascript in electrum-merchant (https://github.com/spesmilo/electrum-merchant/blob/master/electrum-merchant/simple/index.html) without ever needing to access index_url from the JSON file. Pretty sure that field does nothing. You could remove that line as well. (Hope I'm right about that haha)
When an EC merchant receieves a BIP70 Payment from a customer it can be archived alongside the associated payment request for future reference and for extracting transaction data. A subsequent PaymentACK can be constructed from the Payment and a user-defined memo.
This reverts commit a11d4e4.
|
Hey man sorry I'm still on vacation -- likely next week this can get more thoroughly reviewed and merged. I noticed you pushed more stuff. Thanks. |
|
No problem. I am still relatively new to github so I just realized the
importance of creating a separate branch for each PR. I reverted the
changes I added to the last PR and Im gonna create a new branch for them
now. Enjoy your vacation. When you get you can tell me all about what's
wrong with this PR haha.
…On Sat, Sep 28, 2019, 12:47 Calin Culianu ***@***.***> wrote:
Hey man sorry I'm still on vacation -- likely next week this can get more
thoroughly reviewed and merged. I noticed you pushed more stuff. Thanks.
—
You are receiving this because you authored the thread.
Reply to this email directly, view it on GitHub
<#1631?email_source=notifications&email_token=AACUPVQ7DHM4LAXASPQJMT3QL6DDDA5CNFSM4I2QAT2KYY3PNVWWK3TUL52HS4DFVREXG43VMVBW63LNMVXHJKTDN5WW2ZLOORPWSZGOD725VVQ#issuecomment-536206038>,
or mute the thread
<https://github.com/notifications/unsubscribe-auth/AACUPVRIGIXXLBNX6DUNJADQL6DDDANCNFSM4I2QAT2A>
.
|
Electrum Merchant is the only use-as-is solution for BIP70 payment requests in Electron Cash but there are other solutions that allow for greater flexibility by setting index_url to something other than the index.html file provided by that package. Even still, this check is not necessary to allow electrum-merchant to function or even avoid a crash in the case that it is not installed properly (the link will simply be dead).