-
Notifications
You must be signed in to change notification settings - Fork 354
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: LIVE-15628: Convert account address to wallet api id which is the value expected by the dapp browser V2 #8858
base: develop
Are you sure you want to change the base?
Conversation
…by the dapp browser V2
The latest updates on your projects. Learn more about Vercel for Git ↗︎
1 Skipped Deployment
|
apps/ledger-live-desktop/src/renderer/families/evm/AccountHeaderManageActions.ts
Outdated
Show resolved
Hide resolved
…r than just the id
@@ -87,13 +87,13 @@ export function EthStakingModalBody({ | |||
|
|||
const checkBoxOnChange = useCallback(() => { | |||
const value = !doNotShowAgain; | |||
global.localStorage.setItem(`${LOCAL_STORAGE_KEY_PREFIX}${account?.currency?.id}`, `${value}`); |
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.
I think it's important to use the currency id here and not account id as we want the checkBox to work on all accounts for a given currency
I think you should be able to use account?.currency
for this
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.
This is a deprecated file, i checked on the codebase, it's not actually used anywhere. However I changed to account id as it's a key to store something on the local storage so it should in theory be unique, account?.currency
wouldn't be unique as more accounts can be having the same currency. But let me know if you still want me to do the change, i don't mind
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.
Not sure why we still have it then but I would prefer to keep it correct for the logic
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.
yeah but then "However I changed to account id as it's a key to store something on the local storage so it should in theory be unique, account?.currency wouldn't be unique as more accounts can be having the same currency. But let me know if you still want me to do the change, i don't mind" ? if i change it to currency is it not gonna be unique i guess ? it's just a key for the local storage
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.
I think we can revert the changes (or most of them) in this file too now that we only use accountToWalletAPIAccount
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.
I thought the function wouldn't hurt. I used it on the same file, and it could be useful for that conversion on the outside. Do you still want me to revert it ?
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.
I suppose we could keep it
✅ Checklist
npx changeset
was attached.📝 Description
This PR converts the account address selected on LL to the wallet api id which is the value expected by the dapp browser V2
❓ Context
🧐 Checklist for the PR Reviewers