Display warning for RBF or low-fee transactions

[infertux]

Added link to transaction details:

[dykstranet]

Hi @infertux. This is a good start. I have deployed to UAT for testing at https://consensusreality.io/uat/RSBP/pay.html

[dykstranet]

@infertux I see that you changed

• "inline-style-disabled": true,
• "inline-script-disabled": true,
• "inline-style-disabled": false,
• "inline-script-disabled": false,

and added some style code directly into pay.html.

Can you put that style code into the style.css file instead so we can maintain our style conventions

[dykstranet]

@infertux, Here is some feedback on usability. Data entry & modal window behaviour. https://www.youtube.com/watch?v=futafrgFSgQ&feature=youtu.be

[infertux]

@dykstranet Great feedback, I fixed the issues you mentioned. Please deploy the new version and let me know how it goes.

[dykstranet]

I have deployed the new version to UAT. https://consensusreality.io/uat/RSBP/pay.html

[dykstranet]

I recorded my initial feedback and put it up on YouTube.

[dykstranet]

This is working great! Very exciting. RBF and low fee detection working! Just a few little tweaks are still necessary to make sure users don’t get confused.

1. Initial screen should begin blank. Only display error if there is a problem. Beginning with a default value of “1” will force them to backspace. Default should be a blank data entry box. If it’s easy, perhaps we can only display the error if they hover over the grayed out pay button. I like this idea a lot. Only display messages to the user if they hover over a grayed out PAY button. This will limit confusion on the part of the user.

2. I tested a TRX with Low Fee AND RBF. Only the RBF message is displayed and suggests waiting 10 minutes. I think we should remove suggested wait time in the error message.

3. “Another payment has been received. Waiting for payment…” In my opinion this message should not ever be displayed. The POS must be basically stateless. If the modal window shows an error, the user must check and monitor the transaction or the transaction list.

4. Remove “Close”link. Only dismiss using the X in the upper right corner

5. Address Row on Modal screen creates a display problem on small screens (phone) - I think the address might work better if displayed directly under the QR code. There is more room there for a long address. Displayed address could also be a link to the bitcoin URI. (An earlier version of RSBP does this and there are situations when this is handy)

6. Error messages need some changes.

Would be great if errors were hidden until hover or grayed out PAY button.

Suggested Error message changes:

Please enter a number.

Enter Positive Number

Please select a value that is no less than 0.01.

Enter Positive Number

Another payment has been received. Waiting for payment…

This error should not be displayed. From my testing, it shows up when trying to pay another transaction. State information can’t be preserved in the modal window. If there is a problem, the user needs to monitor the transaction or transaction list from the blockchain. 

Replaceable transaction received. You should wait for this message to disappear before releasing the goods, in about 10 minutes.

WARNING! - Replace By Fee, Low Fee Detected - Please wait for 1 confirmation - [small bitcoin logo, link to TRX]
WARNING! - Low Fee Detected - Please wait for 1 confirmation - [small bitcoin logo, link to TRX]
WARNING! - Replace By Fee - Please wait for 1 confirmation - [small bitcoin logo, link to TRX]

Payment received!

Payment Success!

[infertux]

@dykstranet I tweaked the things you mentioned above. Also made the amount input autofocus on page load (not sure why we didn't do that since the beginning).

I'll let you test it and merge the PR if it all looks good.

[infertux]

[dykstranet]

I have deployed to UAT. I will be testing until Monday (January 30, 2017) @infertux @bluemana

[dykstranet]

Small issues. UI should have as few words and warnings as possible.

UI could be more quiet.

1. NO initial red message "Enter Amount"
2. NO "See transaction details" on the "Payment Success" page. (This is because, if they click that link, they will see a big red box that says "Unconfirmed Transaction!" even though we have said in our UI "Payment Success!" Its a small detail, but it will cause confusion for restaurant workers.

[infertux]

@dykstranet Ready for another round of testing :)

[dykstranet]

@bluemana & @infertux rbf-low-fee-warnings is deployed in UAT. https://consensusreality.io/uat/RSBP/pay.html

During testing I got a low fee warning when a normal fee was paid. (According to Mycelium) The transactions paid 105 Satoshi's per byte. (Checked a couple sites and recommended is 90 Satoshis per byte. ) It appears that Insight.bitpay.com may be down. Did we get the low fee warning because insight.bitpay.com was down?

Also very concerned about providing false warnings.

[infertux]

@dykstranet It gets the recommended fee from https://insight.bitpay.com/api/utils/estimatefee?nbBlocks=2 and it's currently 111 so it makes sense you got the low-fee warning paying 105. btc.com's recommended fee seems to be confirming within 3 blocks. BitPay is at 101 and Btc.com is at 100 right now.

Now that I think about it, we should probably lower the threshold so that we don't give false warnings indeed. I'm tempted to lower it to confirm within 6 blocks (currently at 91 sats).

Also if BitPay is down for some reason, it sets the recommended fee to zero, effectively disabling the warning so that shouldn't be an issue.

[dykstranet]

I updated UAT.

Not able to test right now due to rate limit at bitcoinaverage.com

http://imgur.com/a/sqxdu

[dykstranet]

All appears to be functionally working. Higher threshold for triggering Low Fee warning is working. Fewer false positives possible.

[dykstranet]

Question : Do you think we will get a faster "Payment Success!" verification if we are running a Bitcore or bcoin node on the server with the POS?

Seems to be a bit slower than current implementation at https://consensusreality.io/Kismet/pay.html

[infertux]

All appears to be functionally working. Higher threshold for triggering Low Fee warning is working. Fewer false positives possible.

Great! I'll let you merge if you're happy with it then.

Question : Do you think we will get a faster "Payment Success!" verification if we are running a Bitcore or bcoin node on the server with the POS?

Is it that slow? I haven't timed it during my testing but I would say most transactions were received within 4 seconds. It seems the 90th percentile is within 3 to 4 seconds atm.

I guess insight.bitpay.com is hosted in the US but so is blockchain.info so it shouldn't be slower than before. Running our own Bitcore/Bcoin node would not make it faster unless it's physically closer to the users, e.g. in Singapore or Indo. I assume most of the time is spent propagating the transaction via P2P to the US:

user's wallet --> Indo node --> P2P network -----slow-----> BitPay's node in US --> cR server

[dykstranet]

@infertux, All appears to be working. Unless @bluemana has some last minute feedback, I will merge these changes on Wednesday.

[bluemana]

Hi @infertux, nice work on this pull request! Thank you for contributing.

I'd like to discuss this change before merging: the application status controller (app/js/controller/app-status.js) is the controller for the app-status UI element, not the controller for the global application status. Perhaps the name is ambiguous, but I think the controller should be concerned with keeping the status of the app-status label only. This would mean removing all references to other UI components such as the pay button. I think the pay button code should still go into the pay-button controller at controller/pay-button.js.

Not a show stopper, just a note: I think the use of the javascript pseudo protocol is discouraged in favour of event handlers. Up to you.

[infertux]

@bluemana Fair enough but I didn't want to duplicate the logic to get the firstOnline flag and having a separate file just for one button felt like an unnecessary level of abstraction. But I agree that app-status.js is a misnomer now. What about renaming it to home.js, init.js, or maybe form.js?

I think the use of the javascript pseudo protocol is discouraged in favour of event handlers

You mean the javascript:void(0)? I didn't use action="#" as it's scrolling the viewport back to the top when you submit the form. I don't think it's deprecated for <form> elements. Do you have a link about this?

[bluemana]

@infertux app-status.js is the controller of the status label. That really is all there is to it. I prefer to keep the cohesion of the controller as high as possible by not adding logic that is not necessary to the status label. I don't think the pay button needs the firstOnline flag anyway. It just needs to know when the app is online in general. A simple listener to the connectivity event would do.

This is the link about the javascript pseudo protocol, JavaScript URIs section: https://developer.mozilla.org/en-US/docs/Web/JavaScript/Reference/Operators/void

[infertux]

@bluemana This feels like bike-shedding to me. The pay button does need the firstOnline flag (or something similar) so we don't display "Enter positive amount" on the initial page load (which would be technically correct but is a bad UX.) Bare in mind we want to keep RSBP simple and this means it won't get many new features (especially if we switch to CoinOS) so I don't think it's worth over-engineering this. I just gave you push access to my repo so you can add improvements to this PR. Like I told you the other day, I'm not a fan of JavaScript and would rather help with sysadmin and backend work ;)