-
Notifications
You must be signed in to change notification settings - Fork 5
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 10170] Store own addresses from all wallets and accounts #1033
[LW 10170] Store own addresses from all wallets and accounts #1033
Conversation
143c19f
to
e2f2126
Compare
Allure report
smokeTests: ✅ test report for 1b6b1ae6
|
e2f2126
to
f980550
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nice work! Left a couple of comments but it works great.
.../browser-extension-wallet/src/lib/scripts/background/__tests__/cache-wallets-address.test.ts
Show resolved
Hide resolved
apps/browser-extension-wallet/src/lib/scripts/background/cache-wallets-address.ts
Show resolved
Hide resolved
walletId: activeWallet.walletId, | ||
metadata: { | ||
...wallet.metadata, | ||
addresses: [...addresses] |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Maybe addresses
is a bit too generic for the metadata section of the active wallet? (i think this could easily be misinterpreted as only the active wallets addresses). What about allWalletsAddresses
or something more descriptive?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Makes sense, updated here: 3d29a04
581cc5e
to
95b43a8
Compare
25e5f76
to
29f1e84
Compare
efc5083
to
07aac21
Compare
3931828
to
fb4f957
Compare
This looks good to me @lucas-barros , great work! |
}); | ||
}); | ||
|
||
it('should not save duplicated addresses', () => { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
isn't this already tested in the previous case?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes, you're right!
re-approved 👍 |
fb4f957
to
58a32a4
Compare
3e39c9e
to
b1f7f45
Compare
58a32a4
to
e28823f
Compare
e28823f
to
1b6b1ae
Compare
Quality Gate passedIssues Measures |
Checklist
Proposed solution
Store all wallets addresses in each wallet repository metadata
Testing
Account or wallet needs to be activated at least once before making a transaction.
Make transactions between different accounts or wallets and see that receiving addresses is correctly tagged as "own".
Screenshots
Attach screenshots here if implementation involves some UI changes