Display tx fee on lightning pay confirm view #448
Conversation
src/action/payment.js
Outdated
| await this._transaction.update(); | ||
| } | ||
|
|
||
| async getFeeEstimate(destination, satAmt) { |
There was a problem hiding this comment.
We should probably rename this to estimateLightningFee as we will also have an o-chain estimateFee:
https://github.com/lightninglabs/lightning-app/pull/318/files
Also, all actions use paramter destructuring e.g. ({ param }) for all function. This makes it easier to read the code consuming the api and also has some other benefits. See:
http://2ality.com/2015/01/es6-destructuring.html#simulating-named-parameters-in-javascript
| expect(isValid, 'to be', true); | ||
| expect(store.payment.amount, 'to match', /^0[,.]0{4}1{1}7{1}$/); | ||
| expect(store.payment.note, 'to be', 'foo'); | ||
| expect(store.payment.fee, 'to match', /^0[,.]0{5}1{1}$/); |
There was a problem hiding this comment.
In addition to the unit test, we should also add integration test.
valentinewallace
force-pushed
the
show-tx-fee
branch
4 times, most recently
from
1813542 to
4bc7125
Compare
| payment.amount = toAmount(parseSat(request.num_satoshis), settings.unit); | ||
| payment.note = request.description; | ||
| payment.fee = toAmount(parseSat(fee), settings.unit); | ||
| return true; |
There was a problem hiding this comment.
This would freeze the UI if queryRoutes takes a few seconds. Have you tested this with Yalls and Starblocks?
There was a problem hiding this comment.
Ya that's a good point, I did test with Yalls and Starblocks and it was fine but I'm adding a wait screen if queryRoutes takes more than 500ms.
| const isValid = await payments1.decodeInvoice({ | ||
| invoice: store2.invoice.uri, | ||
| }); | ||
| expect(parseInt(store1.payment.fee), 'to be greater than or equal to', 0); |
There was a problem hiding this comment.
Heads up payment.fee is a string formatted float that is in the selected unit (BTC in this case). You'll wanna call parseFloat(store1.payment.fee) in this case.
valentinewallace
force-pushed
the
show-tx-fee
branch
2 times, most recently
from
92d6c34 to
72d8462
Compare
src/action/payment.js
Outdated
|
|
||
| async decodeInvoice({ invoice }) { | ||
| try { | ||
| const timer = setTimeout(() => this._nav.goWait(), WAIT_DELAY); |
There was a problem hiding this comment.
The user shouldn't need to wait to decode an invoice...
src/action/payment.js
Outdated
| }); | ||
| payment.amount = toAmount(parseSat(request.num_satoshis), settings.unit); | ||
| payment.note = request.description; | ||
| payment.fee = toAmount(parseSat(fee), settings.unit); |
There was a problem hiding this comment.
This function just decodes the invoice and returns a boolean.
I'd set store.payment.fee in estimateLightningFee itself and call it after this._nav.goPayLightningConfirm() here https://github.com/lightninglabs/lightning-app/pull/448/files#diff-c103796afd01e866f6af483b8930cf32R57.
src/action/payment.js
Outdated
| }); | ||
| return routes[0].total_fees; | ||
| } catch (err) { | ||
| log.info(`Unable to retrieve fee estimate: ${JSON.stringify(err)}`); |
There was a problem hiding this comment.
err.message should be enough here.
tanx
left a comment
tanx
left a comment
There was a problem hiding this comment.
I refactored the code a bit. Turns out the queryRoute request is just as fast as decoding the invoice so we don't need to display the wait screen for now. Also added some missing unit tests ;)
Please take a look. Ready to merge from my side.
|
@valentinewallace also a heads up... I rebased this onto the current master. |
|
@tanx Lol omg I was almost ready to submit my (similar) updates on this one xD might have missed something, otherwise is there a way to avoid in the future? Looks good to me though 👌 Thanks for adding the tests I missed :) I ran it in the packaged app and it worked great. Merging |
Closes #431