-
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
US-1964 WalletConnect sessions don't get erased when the wallet is reset #900
Conversation
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 PR is changing a few things out of the scope of the ticket, like transforming all the address references from the wallet to the addressToUse function.
I guess this is okay, only one thing that has to be addressed. What I suggest is that we don't modify anything related to the mmkv storage, instead, focus on deleting the wallet connect mmkv storage when the wallet is reset.
3c09570
to
d2135f6
Compare
@jessgusclark @Freshenext |
|
||
export const getCurrentChainId: () => ChainID = () => | ||
ChainStorage.get('chainId') || 31 | ||
MainStorage.get('chainId') || 31 |
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.
Why is this ChainStorage being changed to MainStorage? 🤔
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.
Because there was no need to do a separate instance just for chainId
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.
Have you tested to make sure that testnet and mainnet switch are working ok?
And also wallet reset on testnet
And also wallet reset on mainnet
I ask because changing it could potentially alter the behavior of the wallet
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.
We're calling getCurrentChainId
everytime when we need to fetch chainId
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.
Video proof
Simulator.Screen.Recording.-.iPhone.15.Pro.-.2024-03-06.at.12.36.21.mp4
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.
Looks good.
Tested and the WC session is deleted.
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.
works as expected ✅
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 made one change to where a variable was being pulled from, but it works as expected.
@@ -21,7 +22,8 @@ import { | |||
import { useAppDispatch, useAppSelector } from 'store/storeUtils' | |||
import { selectChainId } from 'store/slices/settingsSlice' | |||
import { addPendingTransaction } from 'store/slices/transactionsSlice' | |||
import { createPendingTxFromTxResponse } from 'src/lib/utils' | |||
import { Wallet } from 'shared/wallet' | |||
import { addressToUse } from 'shared/hooks' |
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.
import { addressToUse } from 'shared/wallet'
No description provided.