-
Notifications
You must be signed in to change notification settings - Fork 393
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
Allow setting custom auto-lock timer #3477
Conversation
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.
Haven't tested yet, some quick comments
e9100f5
to
b95c535
Compare
Hmm, Github isn't running checks for PRs to non-main branches, there is a problem with nonexisting variable reference, I'll fix it quickly EDIT Ok now checks are running, they broke because of force push I guess... |
if we are dropping a flag for private keys then I think we can release this thing in the same release as well and we can omit the feature flag 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.
Updated with the newest main changes so we can QA it here. Overall looks wonderful! I think we can drop the flag 🏳️
298bcd4
to
403bc54
Compare
background/main.ts
Outdated
await this.preferenceService.updateAutoLockTimer(newTimerValue) | ||
await this.internalSignerService.updateAutoLockTimer() |
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.
We discussed some of this elsewhere, but let's update the PreferenceService
and then let InternalSignerService
observe the change via that service, rather than acting on both of them. Similarly, let's let that PreferenceService
change dispatch the setAutoLockTimer
action on the store. Otherwise we can end up with UI/backend drift that could trigger some weird edge cases.
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.
Left a couple of notes around naming, typing, and data flow.
@@ -324,7 +397,7 @@ export default function Settings(): ReactElement { | |||
), | |||
} | |||
|
|||
const settings = Object.values({ | |||
const settings: SettingsList = { |
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.
Any reason this isn't just an array? Doesn't look like we're using the key for anything.
It actually feels like we could straightforwardly use the key for i18n purposes, iterating Object.entries
and skipping the title
field, so we have a { [titleKey: string]: SettingsItem[] }
.
Not blocking ofc.
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.
No reason but also didn't consider it blocking. We could use Object.entries
but we'd have to do some casting since translations are typed and expect a valid dictionary key.
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 already changed here and on Flashbots PR as it was causing conflicts, I think we should unify it separately from both PRs to make them all compatible
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.
We could use Object.entries but we'd have to do some casting since translations are typed and expect a valid dictionary key.
I hate this about Object.entries
🙃
0fb2a97
to
6824f06
Compare
@hyphenized could you resolve conflicts? I can test it one more time and I think it will be good to go |
I realized we are not mentioning what unit we are using here, pretty obvious that these are minutes but maybe not 🤔 cc @VladUXUI |
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.
Works fine ✅
## What's Changed * Add private key onboarding flow by @jagodarybacka in #3119 * Private key JSON import by @jagodarybacka in #3177 * Allow export of private keys and mnemonics by @jagodarybacka in #3248 * Export private key form by @jagodarybacka in #3255 * Unlock screen for the account backup by @kkosiorowska in #3257 * Show mnemonic menu by @jagodarybacka in #3259 * Fix background blur issue by @jagodarybacka in #3265 * Account backup UI fixes by @jagodarybacka in #3270 * Fix unhiding removed accounts by @jagodarybacka in #3282 * New error for incorrectly decrypted JSON file by @jagodarybacka in #3293 * Export private keys from HD wallet addresses by @jagodarybacka in #3253 * Refactor keyring redux slice to remove `importing` field by @jagodarybacka in #3309 * 📚 Accounts backup by @kkosiorowska in #3252 * Catch Enter keypress on Unlock screen by @jagodarybacka in #3355 * Rename `keyring` to `internal signer` and other improvements by @jagodarybacka in #3331 * 🗝 QA - Accounts backup and private key import by @jagodarybacka in #3266 * Remove private key signers if they are replaced by accounts from HD wallet by @jagodarybacka in #3377 * RFB 4: One-Off Keyring Design by @Shadowfiend in #3372 * Copy to clipboard warning by @kkosiorowska in #3488 * Allow setting custom auto-lock timer by @hyphenized in #3477 * Use Argon2 for encrypted vaults by @jagodarybacka in #3502 * 👑 Private keys import and accounts backup by @jagodarybacka in #3089 * Untrusted assets should not block the addition of custom tokens by @kkosiorowska in #3491 * Flip updated dApp connections flag by @Shadowfiend in #3492 * v0.41.0 by @Shadowfiend in #3531 * Switch to a given network if adding a network that is already added. by @0xDaedalus in #3154 * Remove waiting for Loading Doggo component in E2E tests by @jagodarybacka in #3541 * Squeeze content to better fit on Swaps page by @jagodarybacka in #3542 * Refactor of terms for verified/unverified assets by @kkosiorowska in #3528 * Fix ChainList styling by @fulldecent in #3547 * Update release checklist by @jagodarybacka in #3548 * Fix custom asset price fetching by @hyphenized in #3508 * Sticky Defaults: Make Taho-as-default replace MetaMask in almost all cases by @Shadowfiend in #3546 ## New Contributors * @fulldecent made their first contribution in #3547 **Full Changelog**: v0.41.0...v0.42.0 Latest build: [extension-builds-3549](https://github.com/tahowallet/extension/suites/14268975651/artifacts/801826435) (as of Thu, 13 Jul 2023 09:51:56 GMT).
Closes #3471
Adds a dropdown on settings to allow changing the maximum amount of time the wallet can remain idle before locking signing. This new setting is updated through an async thunk which triggers an event to also handle the change on the keyring service. The data is persisted on preferences db and exposed through redux to the UI.
Testing Env
To Test
Latest build: extension-builds-3477 (as of Thu, 22 Jun 2023 16:11:15 GMT).