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

Add ordinal check when transfering BTC #365

Merged
merged 11 commits into from
May 4, 2023
Merged

Conversation

Imamah-Zafar
Copy link
Contributor

@Imamah-Zafar Imamah-Zafar commented Apr 18, 2023

PR Type

What kind of change does this PR introduce?

  • Enhancement

What is the current behavior?

(Optional) Resolved: #355

What is the new behavior?

Screenshot / Video

Figma link: https://www.figma.com/file/eUJAc1beEvNcLPGszkY1xo/Xverse-%E2%80%94-Extension?node-id=4249%3A133488&t=mJBbDnYRbQHFYzU7-1

@Imamah-Zafar Imamah-Zafar self-assigned this Apr 18, 2023
@Imamah-Zafar Imamah-Zafar added this to the Sprint 31 - April 24 milestone Apr 18, 2023
@Imamah-Zafar Imamah-Zafar linked an issue Apr 18, 2023 that may be closed by this pull request
@Imamah-Zafar Imamah-Zafar requested a review from yknl April 19, 2023 11:46
yknl
yknl previously approved these changes Apr 19, 2023
@DuskaT021 DuskaT021 added the bug Something isn't working label Apr 19, 2023
@DuskaT021
Copy link
Contributor

needs conflict resolved

@DuskaT021
Copy link
Contributor

DuskaT021 commented Apr 27, 2023

The test scenarios in this pr are:

  • Will the message appear if there is an ordinal
  • And the message should not appear if there is no ordinal
  • Check does the UI matches the design (figma link)
BTC address has an ordinal BTC address doesn't have an ordinal

@DuskaT021
Copy link
Contributor

DuskaT021 commented Apr 27, 2023

  • Missing divider in the alert

  • Danger title and the sentences font-size should be 16px (actually is 14px)

  • [ ]

  • left image is figma, right image is branch

image

@DuskaT021
Copy link
Contributor

When I went through the send btc flow, and I had an ordinal in the btc address(but choose not to move it to the ordinal address)
I have sent btc from one btc address to another btc address, without moving an ordinal and the ordinal was sent out as well
https://mempool.space/tx/447d350d4ae27cc2600e036f0eb6bae8a0a74c39cac417cf25a44e877b0b8f69

@yknl @Imamah-Zafar Is this the expected result in this scenario?

@Imamah-Zafar
Copy link
Contributor Author

When I went through the send btc flow, and I had an ordinal in the btc address(but choose not to move it to the ordinal address) I have sent btc from one btc address to another btc address, without moving an ordinal and the ordinal was sent out as well https://mempool.space/tx/447d350d4ae27cc2600e036f0eb6bae8a0a74c39cac417cf25a44e877b0b8f69

@yknl @Imamah-Zafar Is this the expected result in this scenario?

from my understanding, yes that is why the alert is shown so the user can move the ordinal

@DuskaT021
Copy link
Contributor

Note for me: waiting for this tx (https://mempool.space/tx/8b5acea32b4481a2385d0e253523ca5c1349c148764c0142006cfac28ecd4446) so I can check the scenario when I have the ordinal in the btc payment address and restore it before sending btc

@DuskaT021
Copy link
Contributor

@Imamah-Zafar Needs typo fix in the h1 it should Restore Ordinals instead of Restore Oridnals
image

@DuskaT021
Copy link
Contributor

DuskaT021 commented Apr 27, 2023

When I chose to recover the ordinal in the send btc flow, it was displaying in the Restore BTC screen until it was pending. Eventually, the ordinal was displayed in the ordinal gallery, which is good 👍

https://mempool.space/tx/800fd2a599a0329e9f2094f55baac65d5087e4a62608f5599f57a9114230cd3f

image

@DuskaT021
Copy link
Contributor

needs conflict to be resolved

@Imamah-Zafar
Copy link
Contributor Author

Imamah-Zafar commented Apr 27, 2023

When I chose to recover the ordinal in the send btc flow, it went to the btc payment address It should have went to the ordinals address that was displayed in the transfer ordinal screen.

https://mempool.space/tx/800fd2a599a0329e9f2094f55baac65d5087e4a62608f5599f57a9114230cd3f

image

the transaction does show that the funds were transferred to ordinal address. input is the btc address and output is ordinal address

@DuskaT021
Copy link
Contributor

Latest update after the daily:
After we identified that the pending utxo will be displayed in the Restore btc screen until it gets confirmed

we need to handle the pending utxo's, so we need to handle it as well in the xverse-core as well.

@DuskaT021
Copy link
Contributor

needs conflicts resolved

@DuskaT021
Copy link
Contributor

Just to confirm that the issue with the utxo pending displays at the recover btc screen - is not present on mobile.

@DuskaT021
Copy link
Contributor

I need to recheck the flow one last time, waiting for this tx to go through https://mempool.space/tx/7b28c2357a40276fd3c33acd62393f67042b2bd2ace4f30cb975cbf58f55e60e

@DuskaT021
Copy link
Contributor

this can be merged. 👍

@DuskaT021 DuskaT021 requested review from yknl and m-aboelenein May 3, 2023 21:51
@yknl yknl merged commit cf60028 into develop May 4, 2023
dhriaznov pushed a commit that referenced this pull request Jul 9, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Check for ordinal UTXO before sending BTC
3 participants