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

feat: add hook for confirmed btc balance, inscription callouts #711

Merged
merged 12 commits into from
Dec 28, 2023

Conversation

abdulhaseeb4239
Copy link
Contributor

πŸ”˜ PR Type

What kind of change does this PR introduce?

  • Bugfix
  • Enhancement
  • Code style update (formatting, local variables)
  • Refactoring (no functional changes, no API changes)
  • Build related changes
  • CI related changes
  • Documentation content changes
  • Other... Please describe:

πŸ“œ Background

Xverse currently displays unconfirmed UTXOs as part of users' balance. This can lead to failed transaction attempts if users try to inscribe, thinking that they have sufficient funds, while some of their balance consists of unconfirmed UTXOs.

Issue Link:
ENG-3385
ENG-3416

πŸ”„ Changes

  • added hook to get confirmed BTC balance
  • update xverse-core to unreleased version
  • added callouts to show insufficient funds, and ledger account

This PR depends on unreleased version from xverse-core PR

Impact:

  • create inscription, repeat inscription

πŸ–Ό Screenshot / πŸ“Ή Video

Screenshot 2023-12-14 at 12 46 02 AM

βœ… Review checklist

Please ensure the following are true before merging:

  • Code Style is consistent with the project guidelines.
  • Code is readable and well-commented.
  • No unnecessary or debugging code has been added.
  • Security considerations have been taken into account.
  • The change has been manually tested and works as expected.
  • Breaking changes and their impacts have been considered and documented.
  • Code does not introduce new technical debt or issues.

dhriaznov
dhriaznov previously approved these changes Dec 14, 2023
Copy link
Contributor

@dhriaznov dhriaznov left a comment

Choose a reason for hiding this comment

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

Good job @abdulhaseeb4239! πŸ™Œ

Not a blocker, but I left a couple of suggestions below πŸ˜‹


const fetchBtcAddressData = async () => btcClient.getAddressData(btcAddress);

let confirmedBalance: number = 0;
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
let confirmedBalance: number = 0;
let confirmedBalance = 0;

Copy link
Contributor

Choose a reason for hiding this comment

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

nit: do we need to explicitly set the type here? I think we can rely on type inference here https://www.typescriptlang.org/docs/handbook/type-inference.html

@@ -982,7 +982,9 @@
"FAILED_TO_FINALIZE": "The inscription transaction failed to finalize. Please try again or contact support.",
"SERVER_ERROR": "An unknown server error occurred. Please try again or contact support."
},
"TOO_MANY_REPEATS": "You can only create up to 24 inscriptions in a single request"
"TOO_MANY_REPEATS": "You can only create up to 24 inscriptions in a single request",
Copy link
Contributor

Choose a reason for hiding this comment

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

I think it would be good to move the 24 number to some constant (in a shared file or the component where it's used) to not change the translations in case the number needs to be changed, right? We can then pass it like this:
Monosnap index tsx β€” xverse-web-extension 2023-12-14 14-30-53

Copy link
Contributor Author

Choose a reason for hiding this comment

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

That's a valid point. Although this wasn't included in the current pull request, I've addressed it anyway πŸ˜„

dhriaznov
dhriaznov previously approved these changes Dec 15, 2023
@teebszet
Copy link
Member

@dhriaznov could you also make sure to maually test this PR and mark when tested, since @DuskaT021 won't be back until two more days

@DuskaT021
Copy link
Contributor

@dhriaznov could you also make sure to maually test this PR and mark when tested, since @DuskaT021 won't be back until two more days

@teebszet I'll pick it up today πŸ™‚

@dhriaznov
Copy link
Contributor

@abdulhaseeb4239 could you resolve the conflicts pls?

@abdulhaseeb4239
Copy link
Contributor Author

@abdulhaseeb4239 could you resolve the conflicts pls?

resolved

@dhriaznov
Copy link
Contributor

A heads-up that the build just failed @abdulhaseeb4239
Monosnap feat: add hook for confirmed btc balance, inscription callouts Β· secretkeylabs:xverse-web-extension@da3fd18 2023-12-22 10-38-37

dhriaznov
dhriaznov previously approved these changes Dec 22, 2023
m-aboelenein
m-aboelenein previously approved these changes Dec 27, 2023
Copy link
Contributor

Choose a reason for hiding this comment

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

@abdulhaseeb4239 we should revert the package-lock.json changes as they're not needed and they also cause the build failure in this PR

Copy link

@m-aboelenein m-aboelenein merged commit bcd28b0 into develop Dec 28, 2023
2 checks passed
@m-aboelenein m-aboelenein deleted the abdulhaseeb/eng-3385 branch December 28, 2023 09:33
This was referenced Dec 29, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

5 participants