Frontend lightning transactions #3049
Conversation
- changed to use View component - cleaned up action-buttons - added global banners
|
I tested it on android and macos. It is a good improvement, but more has to be done for it to be identical to the other account pages. |
|
I don't like much the Sent to/Received to without any additional information. Could we change it, e.g. using Sent/Received?
|
| .map(payment => ({ // TODO: giant hack start | ||
| internalID: payment.id, | ||
| addresses: [], | ||
| amountAtTime: { |
There was a problem hiding this comment.
I think it would be better to hide the fiat conversion, since it is not available. wdyt?
There was a problem hiding this comment.
This is what it currently should show if fiat is not available.
We should make fiat conversion available here, but that is a backend thing. So I would keep as is and consider providing the fiat conversions.
There was a problem hiding this comment.
@Beerosagos added a commit to "temporary hide fiat for lightning transactions".
| </span> | ||
| </TxDetail> | ||
| <TxDetail label="fee"> | ||
| {payment.feeMsat} Msat |
There was a problem hiding this comment.
nit: I'd suggest to use msat instead, without the capital m
| unit: 'sat' as accountApi.CoinUnit, | ||
| estimated: false | ||
| }, | ||
| type: payment.paymentType === 'sent' ? 'send' : 'receive' as accountApi.TTransactionType, |
There was a problem hiding this comment.
There is also type closedChannel
There was a problem hiding this comment.
yeah I know, but afaik it is currently not supported by BBApp on staging-ln branch. Something to keep in mind for later.
Or what do you suggest to do now?
There was a problem hiding this comment.
flipped it so that closedChannel will use send for now, with a TODO to properly support it in the future.
| type: payment.paymentType === 'sent' ? 'send' : 'receive' as accountApi.TTransactionType, | ||
| txID: payment.id, | ||
| note: payment.description || '', | ||
| status: 'complete' as accountApi.TTransactionStatus, // always use complete? |
There was a problem hiding this comment.
Why not use payment.status ?
| time={new Date(payment.paymentTime * 1000).toString()} | ||
| /> | ||
| <TxDetail label="Amount"> | ||
| {payment.amountMsat * 0.001} sat |
There was a problem hiding this comment.
There is toSat() fn available for this in utils/conversion.ts, which adds rounding
| @@ -86,6 +88,7 @@ export const Lightning = () => { | |||
|
|
|||
| const offlineErrorTextLines: string[] = []; | |||
|
|
|||
| console.log('detailID', detailID); | |||
There was a problem hiding this comment.
can we remove the log here?
| onClose(); | ||
| }} | ||
| payment={payment} | ||
| sign={getTxSign(payment.paymentType === 'sent' ? 'send' : 'receive')} |
There was a problem hiding this comment.
Same here about closedChannel txs, which I think should be showed as "sent"
makes 100% sense, but I prefer to not change the transaction component in the staging-ln branch, if possible. |
thisconnect
force-pushed
the
frontend-lightning-transactions
branch
from
beca437
to
4c4b0bb
Compare
Transaction component expects TTransaction data, so this is a bit a hack but probably good enough for testing purposes.
thisconnect
force-pushed
the
frontend-lightning-transactions
branch
from
4c4b0bb
to
8ebb344
Compare
Until we have conversion rates for lightning.
thisconnect
force-pushed
the
frontend-lightning-transactions
branch
from
8ebb344
to
5cfe465
Compare
|
@Beerosagos I thanks for your review, think I address everything but I didn't test closedChannel. |
tACKed, but I think that it would make sense to have different objects for lightning TXs. Adapting the base chain TX object to represent LN ones looks a bit hacky to me |
yeah I fully agree, hence the comment about the giant hack. I'll look into how the transaction component could be refactored in the main version. Let me know if I should merge this as is or wait. |
Ty 🙏 I think we can merge, and we'll refactor everything before merging into master |
No description provided.