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

Pubkey validation added to WatchAccountPopover #217

Merged

Conversation

sammanadh
Copy link
Contributor

@sammanadh sammanadh commented Jul 13, 2022

Mistakeny added a invalid public key in watchAccountPopover which caused the entire app to break.
Screenshot from 2022-07-13 17-50-22
I guess when I added the invalid address to watch, it got stored and is causing this issue even after I restart the app. Next, I will clear stored address from memory, and figure our any measures we can take if such address somehow gets stored. But, for now, this PR should prevent user from entering the invalid address.

@sammanadh
Copy link
Contributor Author

sammanadh commented Jul 13, 2022

Ohh. It uses localstorage. For now the best place to validate public key seemes to the the pinAccount function. But I think implementing a redux middleware that makes sure the all the states that store addresses gets valid public key would be the best approach.

@nathanleclaire
Copy link
Contributor

Looks great, thanks.

I think implementing a redux middleware that makes sure the all the states that store addresses gets valid public key would be the best approach.

Agreed, that might be a good follow up to work on if you're interested

One change around typing requested then LGTM

@nathanleclaire
Copy link
Contributor

Almost there! Just one request to get rid of the !!validationError which was the idea behind making the type string | undefined

@nathanleclaire
Copy link
Contributor

Oh nice, I can just edit your repo 🤯

LGTM

@nathanleclaire nathanleclaire merged commit eb9ed1e into workbenchapp:main Jul 19, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants