-
Notifications
You must be signed in to change notification settings - Fork 80
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(keychain): fix private key missing error #5277
Conversation
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #5277 +/- ##
==========================================
+ Coverage 85.91% 86.00% +0.09%
==========================================
Files 745 745
Lines 30487 30498 +11
Branches 5288 5289 +1
==========================================
+ Hits 26192 26229 +37
+ Misses 4059 4037 -22
+ Partials 236 232 -4
... and 7 files with indirect coverage changes Continue to review full report in Codecov by Sentry.
|
@@ -135,7 +135,7 @@ export function* getViemWallet(chain: Chain) { | |||
if (!account) { | |||
throw new Error(`Account ${walletAddress} not found in Keychain`) | |||
} | |||
const password = yield* call(getPasswordSaga, walletAddress, false, true) | |||
const password = yield* call(getPasswordSaga, walletAddress, true, false) |
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 the fix for new users. everything else is fixing the bad leftover state for existing users.
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.
could be in a follow up, but should we change the arg type of getPasswordSaga to an object, so its explicit on what booleans are set?. E.g.,
const password = yield* call(getPasswordSaga, { account: walletAddress, withVerification: true })
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.
yes absolutely (although i'd prefer to refactor that function to remove these arguments / code paths altogether, they should be in separate functions that are called predictably) but wanted to keep this fix isolated here
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.
Great! Thanks so much for digging all the details. 🚀
Like Satish mentionned, I'm sure we all have a few ideas to make this part of the code base more robust given its importance.
Silas was asking if we should maybe do a small post mortem to learn from this and agree on next steps.
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.
💫 Amazing job!
I agree that we have to refactor this to make it as dumb as possible, avoiding side effects and obscure params like this withVerification
one. I guess part of the problem is not only the form in which it's supplied to the function but the reason for its very existence.
### Description As the title. I've done a detailed writeup [here](https://linear.app/valora/issue/RET-1067/error-while-swapping-private-key-not-found#comment-3729279a). ### Test plan As a user who knows their pincode, I should always be able to perform a transaction and see my recovery phrase. ### Related issues - Fixes RET-1067 ### Backwards compatibility Y ### Network scalability Y
### Description As the title. I've done a detailed writeup [here](https://linear.app/valora/issue/RET-1067/error-while-swapping-private-key-not-found#comment-3729279a). ### Test plan As a user who knows their pincode, I should always be able to perform a transaction and see my recovery phrase. ### Related issues - Fixes RET-1067 ### Backwards compatibility Y ### Network scalability Y
Description
As the title. I've done a detailed writeup here.
Test plan
As a user who knows their pincode, I should always be able to perform a transaction and see my recovery phrase.
Related issues
Backwards compatibility
Y
Network scalability
Y