-
Notifications
You must be signed in to change notification settings - Fork 139
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
Feature: handle lnurl success action #3362
base: main
Are you sure you want to change the base?
Feature: handle lnurl success action #3362
Conversation
…in-completed-screen
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
also please add a screenshot
app/screens/send-bitcoin-screen/send-bitcoin-completed-screen.tsx
Outdated
Show resolved
Hide resolved
app/screens/send-bitcoin-screen/send-bitcoin-confirmation-screen.tsx
Outdated
Show resolved
Hide resolved
app/screens/send-bitcoin-screen/send-bitcoin-details-screen.tsx
Outdated
Show resolved
Hide resolved
app/screens/send-bitcoin-screen/send-bitcoin-details-screen.tsx
Outdated
Show resolved
Hide resolved
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
dont forget to disable auto redirect to home when it has success action
SuccessLUD09Message, | ||
SuccessLUD09URL, | ||
SuccessLUD09URLWithDesc, | ||
} from "../../app/screens/send-bitcoin-screen/send-bitcoin-completed-screen.stories" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
please use @app/screens/...
@@ -381,6 +381,15 @@ const SendBitcoinDetailsScreen: React.FC<Props> = ({ route }) => { | |||
} | |||
|
|||
const result = await requestInvoice(requestInvoiceParams) | |||
|
|||
setPaymentDetail((prev) => { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
again, why prev? this is more like paymentDetail
|
||
setPaymentDetail((prev) => { | ||
if (prev?.setSuccessAction) { | ||
return prev.setSuccessAction(result.successAction) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
what happen if result.successAction
is undefined? maybe better add && result.successAction
to the if
@@ -396,6 +405,10 @@ const SendBitcoinDetailsScreen: React.FC<Props> = ({ route }) => { | |||
paymentRequest: invoice, | |||
paymentRequestAmount: btcAmount, | |||
}) | |||
|
|||
paymentDetailForConfirmation = |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
what if result.successAction
is undefined?
@@ -154,6 +154,8 @@ const SendBitcoinConfirmationScreen: React.FC<Props> = ({ route }) => { | |||
params: { | |||
arrivalAtMempoolEstimate: extraInfo?.arrivalAtMempoolEstimate, | |||
status, | |||
successAction: paymentDetail?.successAction, | |||
preimage: extraInfo?.preimage, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
did you test this?
</View> | ||
</View> | ||
)} | ||
<TouchableOpacity |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
please add accessibility label on touchable elements
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
you are missing aes test... you can use this data
successAction: {
tag: "aes",
description: "Here is your redeem code",
ciphertext: "564u3BEMRefWUV1098gJ5w==",
iv: "IhkC5ktKB9LG91FvlbN2kg==",
},
preimage: "25004cd52960a3bac983e3f95c432341a7052cef37b9253b0b0b1256d754559b",
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Great, thanks!
successAction: undefined, | ||
preimage: undefined, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
if they are undefined then there is no need to set them here
successAction: undefined, | ||
preimage: undefined, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
same here
…adding accesibility labels
Implemented: LNURL successAction: LUD-09, LUD-10