🐛 Persist active wallet #657
Conversation
|
The latest updates on your projects. Learn more about Vercel for Git ↗︎
|
|
|
||
| // Set Active Wallet when page renders | ||
| useEffect(() => { | ||
| const activeWallet = JSON.parse(localStorage.getItem('activeWallet')) |
There was a problem hiding this comment.
Does JSON.parse work on a null string?
There was a problem hiding this comment.
No, it doesn't work on null string
There was a problem hiding this comment.
In this case, if there are no activeWallet, it just returns null. Nothing to set
There was a problem hiding this comment.
@IYTEC I like what you are doing but I am going to hold off merging this in because I do see some edge-cases with this implementation. Perhaps next week we can get hop on a call and talk about it. But for one thing, setActiveWallet() will throw an error some of the time because the actual hook is setting the wallet on the Algodex Api instance. Since it is internally going through our API it will be subject to AJV schema validation and will throw errors when passed an empty object.
|
|
||
| // Set Active Wallet when page renders | ||
| useEffect(() => { | ||
| const activeWallet = JSON.parse(localStorage.getItem('activeWallet')) |
There was a problem hiding this comment.
@IYTEC I like what you are doing but I am going to hold off merging this in because I do see some edge-cases with this implementation. Perhaps next week we can get hop on a call and talk about it. But for one thing, setActiveWallet() will throw an error some of the time because the actual hook is setting the wallet on the Algodex Api instance. Since it is internally going through our API it will be subject to AJV schema validation and will throw errors when passed an empty object.
| useEffect(() => { | ||
| const activeWallet = JSON.parse(localStorage.getItem('activeWallet')) | ||
| if (activeWallet) { | ||
| setActiveWallet(activeWallet) |
There was a problem hiding this comment.
@IYTEC I actually think that all we would need to do to avoid the edge case would be to check the return
const activeWallet = JSON.parse(localStorage.getItem('activeWallet')) and only call setActiveWallet(activeWallet) when it isn't an empty object. Great work either way! Lets take the weekend to think about it but we can probably merge this in with minimal tweaking on Monday 🕺
There was a problem hiding this comment.
Awesome! No problem, we will go through it on Monday
|
@IYTEC @ericsharma what's the status on this? |
|
@atrefonas I'm working on this. Had a meeting with @ericsharma on what the issue was, working on it today. |
|
Is this PR still needed or is it old and should be closed? |
|
@atrefonas It is old but is a feature that we will eventually want to include in next3. In a prior meeting we discussed this issue and ranked it as low priority for the merge with development. It is something we will work on afterwards so I think it is best to keep it open for now. |
|
@atrefonas, we plan to add this after the merge, just like @ericsharma mentioned. |
There was a problem hiding this comment.
As mentioned in the other PR, before making further wallet changes, we first need to:
- Move all wallet hooks into algodex-react
- Switch to a useReducer and do all the state changes in one place
The wallet code is currently unmanageable in the current state IMO.
ℹ Overview
Persist active wallet address
📝 Related Issues
🔐 Acceptance:
yarn testpasses