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

feat: use viem when creating burner wallet #576

Merged
merged 4 commits into from
Apr 4, 2023
Merged

Conversation

holic
Copy link
Member

@holic holic commented Apr 3, 2023

see #582

const cachedPrivateKey = localStorage.getItem(cacheKey);
const subject = cachedPrivateKey
const subject = isHex(cachedPrivateKey)
Copy link
Member Author

@holic holic Apr 3, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

are we okay with this being a destructive action? it used to just add a burner wallet if no value was set for the given cache key, but now it checks if it's hex and will recreate one if not

which means if someone has a different value stored at this key, we will end up overriding it with a new burner wallet private key

alternatively, we could just throw/bail if we detect a non-hex value at this cache key

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think throwing if it's an invalid value would be better bc it would tell users who are trying to override the burner wallet (but put an invalid value) why it fails

@holic holic marked this pull request as ready for review April 3, 2023 14:57
@holic holic requested a review from alvrs as a code owner April 3, 2023 14:57
alvrs
alvrs previously approved these changes Apr 4, 2023
Copy link
Member

@alvrs alvrs left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

lgtm 👍 (just one comment about throwing)

@holic holic merged commit d5d22e0 into main Apr 4, 2023
@holic holic deleted the holic/burner-wallet-viem branch April 4, 2023 17:06
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.

2 participants