[ENG-3436] chore: Make some code refactoring for RBF in the extension #712

dhriaznov · 2023-12-13T20:00:58Z

🔘 PR Type

What kind of change does this PR introduce?

Refactoring (no functional changes, no API changes)

📜 Background

After the RBF UI & logic were added to the web-extension, we also developed it for the mobile app. And some things could be improved in its code like constants, typing, etc. (kudos to @fedeerbes for the mobile app PR review)

This PR is made to address those things for the web extension as well.

Issue Link: #[ENG-3436]
Context Link (if applicable):

🔄 Changes

Moved Speed Up tx custom fee component to a separate folder and created a separate file for its styles
Removed the initialFeeRate property of the custom fee component that had the same value as minimumRbfFee
Fixed typing for the Speed Up tx screen
Moved the handleKeyDownFeeRateInput function to helpers to reuse it
Added EMPTY_LABEL constant
Used the FiatAmountText component instead of duplicating the getFiatAmountString function
Moved the copy related to estimated RBF completion time to translations

Impact:

This PR makes the RBF code more clean and robust, and shouldn't break any logic

🖼 Screenshot / 📹 Video

Include screenshots or a video demonstrating the changes. This is especially helpful for UI changes.

✅ 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.

…-for-rbf-in-the-extension

teebszet · 2023-12-14T03:29:11Z

src/app/screens/speedUpTransaction/customFee/index.tsx

+  const fetchTotalFee = async () => {
+    const response = await calculateTotalFee(feeRateInput);
+
+    if (response) {
+      setTotalFee(response.toString());
+    }
+  };
+
+  useEffect(() => {
+    fetchTotalFee();
+  }, [feeRateInput]);


Suggested change

const fetchTotalFee = async () => {

const response = await calculateTotalFee(feeRateInput);

if (response) {

setTotalFee(response.toString());

}

};

useEffect(() => {

fetchTotalFee();

}, [feeRateInput]);

useEffect(() => {

const fetchTotalFee = async () => {

const response = await calculateTotalFee(feeRateInput);

if (response) {

setTotalFee(response.toString());

}

};

fetchTotalFee();

}, [feeRateInput, calculateTotalFee]);

nit: I think if fetchTotalFee is not a function used anywhere but this useEffect, it should go inside. then it will only recreate this function on change of the useEffect deps array

When I apply the code you suggested, it starts lagging for some reason 🤔 let's probably keep the original version for now

teebszet · 2023-12-14T03:44:28Z

src/app/screens/speedUpTransaction/customFee/index.styled.ts

+export const ControlsContainer = styled.div`
+  display: flex;
+  gap: 12px;
+  margin: 24px 16px 40px;
+`;


fyi: I have started moving some common layouts (defined in zeroheight) into common.styled.ts 😄

teebszet · 2023-12-14T03:45:19Z

src/app/screens/speedUpTransaction/customFee/index.styled.ts

very clean, thank you!

teebszet · 2023-12-14T03:45:48Z

src/app/screens/speedUpTransaction/customFee/index.tsx

teebszet · 2023-12-14T03:47:56Z

src/app/screens/speedUpTransaction/index.tsx

@@ -195,7 +206,7 @@ function SpeedUpTransactionScreen() {
    } catch (err: any) {
      console.error(err);

-      if (err?.response?.data && err?.response?.data.includes('insufficient fee')) {
+      if (err?.response?.data && err.response.data.includes('insufficient fee')) {


Suggested change

if (err?.response?.data && err.response.data.includes('insufficient fee')) {

if (err?.response?.data?.includes('insufficient fee')) {

yeah or this 😄
a matter of code aesthetics I guess

teebszet · 2023-12-14T03:56:23Z

src/app/screens/speedUpTransaction/index.tsx

@@ -128,7 +131,7 @@ function SpeedUpTransactionScreen() {
    } finally {


L124-125: is there a hook we can use for mempoolApi.getRecommendedFees?

L130: I wonder if we need a toast error here too?

L108-128: could abstract all this logic into a hook like:

const useRbfTransactionData: (transaction: ReturnType<useTransaction>) => { rbfTransaction?: rbf.RbfTransaction, rbfTxSummary?: { currentFee: number; currentFeeRate: number; minimumRbfFee: number; minimumRbfFeeRate: number; }, rbfRecommendedFees?: RbfRecommendedFees } = (transaction) => { const { selectedAccount, accountType, network } = useWalletSelector(); const seedVault = useSeedVault(); const btcClient = useBtcClient(); // logic here } // in SpeedUpTransaction: const {rbfTransaction, rbfTxSummary, rbfRecommendedFees} = useRbfTransactionData(transaction)

or even wrap it in a useQuery, and get the isLoading and error states too

makes sense! I added a todo comment for this

teebszet · 2023-12-14T04:27:42Z

src/app/screens/speedUpTransaction/index.tsx

                    const priorityOrder = ['highest', 'higher', 'high', 'medium'];
                    return priorityOrder.indexOf(a[0]) - priorityOrder.indexOf(b[0]);
                  })


nit: maybe you could sort this before setRbfRecommendedFees()? here it will sort again on every render

teebszet

thanks for taking the time to tidy the code! I had some more code style suggestions, but only suggestions.

and a couple of nits on (probably insignificant) react optimisations

dhriaznov · 2023-12-14T12:18:17Z

Thanks for the review @teebszet! I updated the PR, and also included the fix from #713 you created since it's related and would be better to test those 2 PRs together. Could you approve it again pls? 🙂

…-for-rbf-in-the-extension

dhriaznov · 2023-12-22T09:22:24Z

It would make sense to test and merge this PR together with #727 in the next release (0.28.0)

…-for-rbf-in-the-extension

github-actions · 2023-12-29T13:51:27Z

Test with build here: https://github.com/secretkeylabs/xverse-web-extension/actions/runs/7357186386

DuskaT021 · 2024-01-09T12:41:50Z

@dhriaznov this can be merged, tested per agreement

…re not unique across ledger devices (#712)

dhriaznov added 2 commits December 13, 2023 20:41

[ENG-3436] chore: Make some code refactoring for RBF in the extension

12033bc

Make some code fixes

51c6e0e

dhriaznov self-assigned this Dec 13, 2023

dhriaznov added the chore label Dec 13, 2023

dhriaznov requested a review from teebszet December 13, 2023 20:11

dhriaznov and others added 2 commits December 13, 2023 21:13

Make some code fixes

36244aa

Merge branch 'develop' into denys/eng-3436-make-some-code-refactoring…

55ad8d5

…-for-rbf-in-the-extension

dhriaznov marked this pull request as ready for review December 13, 2023 20:15

teebszet reviewed Dec 14, 2023

View reviewed changes

src/app/screens/speedUpTransaction/customFee/index.styled.ts

Copy link

Member

teebszet Dec 14, 2023

Choose a reason for hiding this comment

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

very clean, thank you!

teebszet reviewed Dec 14, 2023

View reviewed changes

src/app/screens/speedUpTransaction/customFee/index.tsx

Copy link

Member

teebszet Dec 14, 2023

Choose a reason for hiding this comment

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

👏 👏

teebszet reviewed Dec 14, 2023

View reviewed changes

teebszet previously approved these changes Dec 14, 2023

View reviewed changes

teebszet and others added 2 commits December 14, 2023 12:57

fix: put speed up screen in another scrollable container

57a1931

Make some code tweaks according to PR review comments

d63b1b3

dhriaznov dismissed teebszet’s stale review via d63b1b3 December 14, 2023 12:14

dhriaznov mentioned this pull request Dec 14, 2023

fix: put speed up screen in another scrollable container #713

Closed

15 tasks

dhriaznov requested a review from teebszet December 14, 2023 12:19

Remove the unused import

db7fc3e

teebszet approved these changes Dec 14, 2023

View reviewed changes

dhriaznov added the ready for test label Dec 14, 2023

dhriaznov added 2 commits December 18, 2023 09:59

Merge branch 'develop' into denys/eng-3436-make-some-code-refactoring…

54b83ec

…-for-rbf-in-the-extension

Merge branch 'develop' into denys/eng-3436-make-some-code-refactoring…

827a9d7

…-for-rbf-in-the-extension

dhriaznov added Release 0.27.0 and removed Release 0.27.0 labels Dec 21, 2023

Merge branch 'develop' into denys/eng-3436-make-some-code-refactoring…

351e520

…-for-rbf-in-the-extension

dhriaznov added the release 0.28.0 label Jan 5, 2024

DuskaT021 added testing QA testing in progress and removed ready for test labels Jan 9, 2024

DuskaT021 added ready for release and removed testing QA testing in progress labels Jan 9, 2024

DuskaT021 merged commit e279511 into develop Jan 9, 2024
2 checks passed

teebszet deleted the denys/eng-3436-make-some-code-refactoring-for-rbf-in-the-extension branch January 10, 2024 07:37

This was referenced Jan 11, 2024

release: v0.28.0 to main #741

Merged

release: v0.28.0 to develop #742

Merged

teebszet added a commit that referenced this pull request Nov 5, 2024

fix: account balance key should not use deviceAccountIndex, as they a…

8e55f9c

…re not unique across ledger devices (#712)

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[ENG-3436] chore: Make some code refactoring for RBF in the extension #712

[ENG-3436] chore: Make some code refactoring for RBF in the extension #712

dhriaznov commented Dec 13, 2023 •

edited

Loading

teebszet Dec 14, 2023 •

edited

Loading

dhriaznov Dec 14, 2023

teebszet Dec 14, 2023 •

edited

Loading

teebszet Dec 14, 2023

teebszet Dec 14, 2023

teebszet Dec 14, 2023 •

edited

Loading

teebszet Dec 14, 2023 •

edited

Loading

dhriaznov Dec 14, 2023

teebszet Dec 14, 2023 •

edited

Loading

teebszet left a comment •

edited

Loading

dhriaznov commented Dec 14, 2023

dhriaznov commented Dec 22, 2023

github-actions bot commented Dec 29, 2023

DuskaT021 commented Jan 9, 2024

	if (err?.response?.data && err.response.data.includes('insufficient fee')) {
	if (err?.response?.data?.includes('insufficient fee')) {

		@@ -128,7 +131,7 @@ function SpeedUpTransactionScreen() {
		} finally {

[ENG-3436] chore: Make some code refactoring for RBF in the extension #712

[ENG-3436] chore: Make some code refactoring for RBF in the extension #712

Conversation

dhriaznov commented Dec 13, 2023 • edited Loading

🔘 PR Type

📜 Background

🔄 Changes

🖼 Screenshot / 📹 Video

✅ Review checklist

teebszet Dec 14, 2023 • edited Loading

Choose a reason for hiding this comment

dhriaznov Dec 14, 2023

Choose a reason for hiding this comment

teebszet Dec 14, 2023 • edited Loading

Choose a reason for hiding this comment

teebszet Dec 14, 2023

Choose a reason for hiding this comment

teebszet Dec 14, 2023

Choose a reason for hiding this comment

teebszet Dec 14, 2023 • edited Loading

Choose a reason for hiding this comment

teebszet Dec 14, 2023 • edited Loading

Choose a reason for hiding this comment

dhriaznov Dec 14, 2023

Choose a reason for hiding this comment

teebszet Dec 14, 2023 • edited Loading

Choose a reason for hiding this comment

teebszet left a comment • edited Loading

Choose a reason for hiding this comment

dhriaznov commented Dec 14, 2023

dhriaznov commented Dec 22, 2023

github-actions bot commented Dec 29, 2023

DuskaT021 commented Jan 9, 2024

dhriaznov commented Dec 13, 2023 •

edited

Loading

teebszet Dec 14, 2023 •

edited

Loading

teebszet Dec 14, 2023 •

edited

Loading

teebszet Dec 14, 2023 •

edited

Loading

teebszet Dec 14, 2023 •

edited

Loading

teebszet Dec 14, 2023 •

edited

Loading

teebszet left a comment •

edited

Loading