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: remove bignumber from the redux store #610

Conversation

victorkirov
Copy link
Member

πŸ”˜ PR Type

What kind of change does this PR introduce?

  • Bugfix

πŸ“œ Background

When changing settings, if you have multiple tabs open, the saved settings are overwritten by the last tab that is closed, and settings are not propagated across tabs. This is due to the store sync not syncing everything, which was disabled because we had class instances in the store.

πŸ”„ Changes

All BigNumber instances were removed from the store and the values were cast to BigNumber where needed throughout the code. A number of other components were also changed to fix any linting and typing issues that were blocking commits.

Impact:

  • This changes stx and btc balances and fiat rates in the store, so it would affect any component which uses those
  • Since we updated the store sync, this will also affect onboarding and the data that was synced between tabs during onboarding

πŸ–Ό Screenshot / πŸ“Ή Video

Before:

Screen.Recording.2023-10-05.at.14.18.30.mov

After:

Screen.Recording.2023-10-05.at.14.19.55.mov

βœ… 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.

@victorkirov victorkirov self-assigned this Oct 6, 2023
@victorkirov victorkirov requested a review from yknl October 9, 2023 08:25
yknl
yknl previously approved these changes Oct 11, 2023
Copy link
Contributor

@yknl yknl left a comment

Choose a reason for hiding this comment

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

Let's make sure to test the Ledger send BRC-20 flow given the typing issue.

@DuskaT021
Copy link
Contributor

@victorkirov can we resolve this conflict, tnx

@blehitsahmad
Copy link

@victorkirov kindly resolve the conflicts so this PR can be picked up for testing too

…-extension into victor/eng-2981-power-user-with-over-3000-ordinals-is-no-longer-able-to
@blehitsahmad
Copy link

This PR will be picked up after the seed vault PR is merged -- as per the discussion with @victorkirov

@blehitsahmad
Copy link

Hey @victorkirov could you resolve this conflicts so the testing for this PR can be started?

@blehitsahmad blehitsahmad reopened this Oct 30, 2023
@DuskaT021 DuskaT021 added help wanted Extra attention is needed returned Tested by the QA, didn't pass checks and removed ready for test labels Oct 31, 2023
@DuskaT021 DuskaT021 removed the help wanted Extra attention is needed label Oct 31, 2023
…-extension into victor/eng-2981-power-user-with-over-3000-ordinals-is-no-longer-able-to
@blehitsahmad blehitsahmad added testing QA testing in progress and removed returned Tested by the QA, didn't pass checks labels Nov 1, 2023
@teebszet
Copy link
Member

@blehitsahmad this is ready for retest

@blehitsahmad
Copy link

@DuskaT021 could you kindly take a look at the ledger BRC20 send flow for this PR -- I have tested the rest. The test cases can be found here

Copy link

@DuskaT021
Copy link
Contributor

@victorkirov this is tested and can be merged

@DuskaT021 DuskaT021 added ready for release and removed testing QA testing in progress labels Nov 20, 2023
@teebszet teebszet merged commit 306be06 into develop Nov 20, 2023
2 checks passed
@teebszet teebszet deleted the victor/eng-2981-power-user-with-over-3000-ordinals-is-no-longer-able-to branch November 20, 2023 08:38
@teebszet teebszet mentioned this pull request Nov 21, 2023
1 task
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