Skip to content
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

Fix hard coded receiving method into order detail message. #1260

Merged

Conversation

rodrigodh
Copy link
Contributor

What does this PR do?

This PR introduces a fix for the order detail box about the receiving payment message, so users can have it more clear when they are receiving through on-chain or lightning.

I found this problem on my last transaction when even though i selected on-chain it was showing lightning, but was just a visual issue.

Screenshot 2024-05-01 at 12 28 51

Checklist before merging

  • Install pre-commit and initialize it: pip install pre-commit, then pre-commit install. Pre-commit installs git hooks that automatically check the codebase. If pre-commit fails when you commit your changes, please fix the problems it points out.

It's important to notice that I don't have much experience with open-source or crypto, so I'm up to fix any mistake on my part.

Copy link
Collaborator

@Reckless-Satoshi Reckless-Satoshi left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Good catch. We might need to expose a better param from the API for this to work. The alternative is to delete the via {method} so it will simply read "You receive XX Sats (Approx)"

@@ -207,8 +207,10 @@ const OrderDetails = ({
amount: amountString,
method: order.payment_method,
});
receive = t('You receive via Lightning {{amount}} Sats (Approx)', {
const receives_on_chain = order.swap_fee_rate > 0;
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is order.swap_fee_rate the best param we can use for this? This might display "onchain" for everyone while they are in the step of submitting an invoice.

Ideally we have a order.is_swap boolean that is only sent to the buyer.

Copy link
Contributor Author

@rodrigodh rodrigodh May 1, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Since we don't have this param from the API now I just changed the text.

@rodrigodh rodrigodh closed this May 1, 2024
@rodrigodh rodrigodh force-pushed the fix-order-detail-receive-message branch from 78f1815 to 8d2ca28 Compare May 1, 2024 21:58
Inside the order detail component we have a little text telling the user how much he'sreceiving from the transaction, this commit changes the text removing the lightning word from it because sometimes the receiving method is on-chain. Ideally we would have aorder.is_swap boolean that is only sent to the buyer to identify the method but since we don't have it yet, we are just changing the text.
Copy link
Collaborator

@Reckless-Satoshi Reckless-Satoshi left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thank you @rodrigodh ! It is not the best possible solution, but at least it is not confusing anymore (that's what matters).

Congratulations on your first contribution. Please submit an invoice for 15K Sats for a small reward from the Development fund.

@rodrigodh
Copy link
Contributor Author

Thank you @rodrigodh ! It is not the best possible solution, but at least it is not confusing anymore (that's what matters).

Congratulations on your first contribution. Please submit an invoice for 15K Sats for a small reward from the Development fund.

Thank you! I'm not sure if my Lightning Network is set up correctly, but you can try this invoice. (don't worry if it doesn't work.)

I hope I can contribute even more in the future.

lnbc150u1pnr9674pp55jmsugjuzxmhv777rk44stqt9l27mc64w4a8kqjc6a7dmvvlk4lqsp5u5g0vt4jadcqnsmnsg7hzdex32dpap8dhxwxl3cauem9rqa52q5qdq2ga5hg6r4vgcqzynxqyjw5q9qy9scqfppqmqttp9zvyz5avpc42e4xh53m4z4hlj79rzjqvv2e8a2jc570ksgsxdu3lsd62hrq3xknvdj9qaxx3u6elltj6zgxrx03vqqp9gqqgqqqqlgqqqqqqgq2q9lmwdq4kufmu0pun6hleqjpgc7655yptfwyarrgerqlw662stymskd3wgv86nnl5ddnl0s76khhnts2hm6794sucxcuzhwrqr795y8cp8qeaxr

@rodrigodh
Copy link
Contributor Author

@Reckless-Satoshi is it merging?

@Reckless-Satoshi
Copy link
Collaborator

lnbc150u1pnr9674pp55jmsugjuzxmhv777rk44stqt9l27mc64w4a8kqjc6a7dmvvlk4lqsp5u5g0vt4jadcqnsmnsg7hzdex32dpap8dhxwxl3cauem9rqa52q5qdq2ga5hg6r4vgcqzynxqyjw5q9qy9scqfppqmqttp9zvyz5avpc42e4xh53m4z4hlj79rzjqvv2e8a2jc570ksgsxdu3lsd62hrq3xknvdj9qaxx3u6elltj6zgxrx03vqqp9gqqgqqqqlgqqqqqqgq2q9lmwdq4kufmu0pun6hleqjpgc7655yptfwyarrgerqlw662stymskd3wgv86nnl5ddnl0s76khhnts2hm6794sucxcuzhwrqr795y8cp8qeaxr

I get "No Route". Please feel free to post another one with a long expiration time so I can keep retrying.

Merging this PR 🚀

@Reckless-Satoshi Reckless-Satoshi merged commit c538066 into RoboSats:main May 5, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants