[Wallet] Improve error handling around account creation and keystore ops #1497
Conversation
Improve logging in keystore utils Send keystore errors to Sentry
Codecov Report
@@ Coverage Diff @@
## master #1497 +/- ##
=========================================
Coverage ? 73.69%
=========================================
Files ? 277
Lines ? 7432
Branches ? 660
=========================================
Hits ? 5477
Misses ? 1843
Partials ? 112
Continue to review full report at Codecov.
|
| } else { | ||
| Logger.error(TAG + '@assignAccountFromPrivateKey', 'Error importing raw key') | ||
| throw e | ||
| } | ||
| } | ||
| yield call(web3.eth.personal.unlockAccount, account, pincode, UNLOCK_DURATION) | ||
| web3.eth.defaultAccount = account |
There was a problem hiding this comment.
Do you know why this was previously done only when !zeroSyncMode?
There was a problem hiding this comment.
It's still only done in !zerosyncmode, I just improved the organization.
It's think it's because we don't use web3 to sign txs in zero sync mode, not 100% sure tho
| try { | ||
| Logger.debug(TAG, `Setting key ${key} in keystore`) | ||
| await RNSecureKeyStore.set(key, value, { | ||
| accessible: ACCESSIBLE.AFTER_FIRST_UNLOCK_THIS_DEVICE_ONLY, |
There was a problem hiding this comment.
Why changing the accessible option now?
There was a problem hiding this comment.
I was just experimenting before I realized this only affects iOS. This option seems right to me, WDYT?
There was a problem hiding this comment.
This has a subtle implication when restoring the phone from a backup.
If using one of the options with the _THIS_DEVICE_ONLY suffix, the keychain item won't be backed up. Which means users will lose their mnemonic if they restore their phone (though I know they should have manually backed up their key). But the AsyncStorage content will be restored.
This doesn't sound right to me as is and we should make a careful consideration about this (and same for the geth data folder and AsyncStorage content).
More about this:
https://developer.apple.com/documentation/security/ksecattraccessibleafterfirstunlockthisdeviceonly
There was a problem hiding this comment.
Yep that was intentional, I felt that the backup key should not be synced to Apple's servers.
There was a problem hiding this comment.
Will circle back on this directly on Slack.
jmrossy
added
automerge
jmrossy
force-pushed
the
rossy/wa-improve-accout-error-handling
branch
from
cf31f06
to
a7bcef9
Compare
jmrossy
force-pushed
the
rossy/wa-improve-accout-error-handling
branch
from
59f8546
to
cb3be64
Compare
* master: (62 commits) Fix e2e on CI (#1537) Allow a specified address to disable/enable the Exchange (#1467) Avoid re-encrypting key files with yarn keys:encrypt command (#1560) Support protocol hotfixing (#613) Point e2e tests back (#1562) Refactor to Accounts.sol (#1392) Add selectIssuers Transaction (#1327) [Wallet] Get React Native Hot Reloading Working (#1551) Unify to prefix messages for signing (#1473) [Wallet] Improve error handling around account creation and keystore ops (#1497) Add CI test for checking licenses and misc package.json cleanup (#1550) [Wallet] Implement SMS invite on iOS (#1541) CI: brings back to master (#1554) Validators: uses Ethereum address for proof of possession (#1494) Validate Attestation Requests (#1468) Rename hosted node references to forno (#1511) Bump rubyzip from 1.2.3 to 1.3.0 in /packages/mobile (#1508) Added txpool family to geth apis. Sorted geth cmd options (#1462) [Wallet] Fix yarn dev command for running android (#1534) [Wallet] iOS info plist changes and version bump (#1539) ... # Conflicts: # yarn.lock
Description
Tested
Ran through invite flow and import flow, no regressions
Issues
Backwards compatibility
Yes