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

Lw 9170 multi wallet integration #892

Merged
merged 27 commits into from
Feb 28, 2024
Merged

Conversation

mkazlauskas
Copy link
Member

@mkazlauskas mkazlauskas commented Feb 15, 2024

Checklist

  • JIRA - LW-9170
  • Proper tests implemented
  • Screenshots added.

Proposed solution

Integrate multi-wallet/account UI components with SDK utilities

Out of scope: enter password/connect hardware modal when enabling accounts (LW-9709)

Testing

Requires USE_MULTI_WALLET=true

Screenshots

image
image

@mkazlauskas mkazlauskas requested a review from a team as a code owner February 15, 2024 10:15
@github-actions github-actions bot added browser Changes to the browser application. ui-toolkit labels Feb 15, 2024
Copy link

github-actions bot commented Feb 15, 2024

Allure report

allure-report-publisher generated test report!

smokeTests: ✅ test report for bd11e2ef

passed failed skipped flaky total result
Total 32 0 0 0 32

Copy link
Member

@DominikGuzei DominikGuzei left a comment

Choose a reason for hiding this comment

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

outstanding work 🎉 love the descriptive and clean commits + it's working smoothly with multiple wallets and accounts!

I left some code suggestions / questions

@coveralls
Copy link

coveralls commented Feb 15, 2024

Coverage Status

Changes unknown
when pulling bd11e2e on LW-9170-multi-wallet-integration
into ** on main**.

Copy link
Contributor

@renanvalentin renanvalentin left a comment

Choose a reason for hiding this comment

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

I have only found one issue related to hooks, everything else looks good. I need to run some tests locally

@DominikGuzei
Copy link
Member

@mkazlauskas thanks for the updates, all green from my side 🚢

@mkazlauskas mkazlauskas force-pushed the LW-9170-multi-wallet-integration branch 2 times, most recently from 9337d2f to c2d6a83 Compare February 19, 2024 08:40
Copy link
Contributor

@renanvalentin renanvalentin left a comment

Choose a reason for hiding this comment

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

@mkazlauskas approved!

@mkazlauskas mkazlauskas force-pushed the LW-9170-multi-wallet-integration branch 2 times, most recently from 282e3ce to 7a410ef Compare February 19, 2024 15:52
@github-actions github-actions bot added the staking Changes to the staking package. label Feb 23, 2024
@tomislavhoracek
Copy link
Contributor

If it takes more than some time (probably 50ms), Chrome will
reject hardware device connection with an error that says
WebHID connections must be initiated from user gesture.

Apparently, fetching wallets from repository can sometimes
take longer. This fix updates useWalletManager addAccount
method to take in the entire AnyBip32Wallet object instead
of just walletId, so that it doesn't have to fetch the
wallet object from indexeddb in service worker.

Consequently, some validations from addAccount can be removed,
because it's signature no longer accepts walletId of a script
wallet. Also it has to trust that wallet actually exists,
otherwise it will reject with an error that comes from the
WalletRepository (it is no longer a responsibility of this
method to check the existence of the wallet).
clicking on a wallet in dropdown menu activates it

removing copy feature makes the behavior more predictable
show tooltip that explains the behavior
also decouple button color scheme from size scheme

and set minWidth for ExtraSmall button, which is
currently only used for profile dropdown account item
temporary design until we apply a larger re-design
toast is a temporary solution, to be replaced
onboarding, adding wallet, adding accounts and dapp transacations are now working
required for correctly bundling trezor
after removing imports from dist/cjs, service worker no longer loads

this is a temporary solution
@tomislavhoracek tomislavhoracek force-pushed the LW-9170-multi-wallet-integration branch from daa2543 to e4ed066 Compare February 27, 2024 08:57
@tomislavhoracek
Copy link
Contributor

Rebased and fixed "abs / relative paths" conflicts ( kept changes Martynas introduced ).
DO NOT MERGE flag removed!

@tomislavhoracek
Copy link
Contributor

tomislavhoracek commented Feb 27, 2024

@pczeglik-iohk I Checked 5b2832a Approved! 💯 🚀
@DominikGuzei @renanvalentin could you check it as well? 🙏

@pczeglik-iohk pczeglik-iohk force-pushed the LW-9170-multi-wallet-integration branch from 5b2832a to bd11e2e Compare February 27, 2024 12:02
Copy link

sonarcloud bot commented Feb 27, 2024

Quality Gate Failed Quality Gate failed

Failed conditions
B Reliability Rating on New Code (required ≥ A)

See analysis details on SonarCloud

idea Catch issues before they fail your Quality Gate with our IDE extension SonarLint SonarLint

@mkazlauskas mkazlauskas merged commit 472d7d5 into main Feb 28, 2024
12 of 17 checks passed
@mkazlauskas mkazlauskas deleted the LW-9170-multi-wallet-integration branch February 28, 2024 11:22
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
browser Changes to the browser application. staking Changes to the staking package.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

7 participants