Change Password flow #679
Change Password flow #679
Conversation
|
📦 build.zip [updated at Apr 25, 5:59:44 PM UTC] |
everdimension
force-pushed
the
feature/reset-password-glowup
branch
2 times, most recently
from
b6e3d67 to
40a057b
Compare
|
New change password flow allows to create a password without warnings/checks for password strength. May be it is worth adding there too |
everdimension
force-pushed
the
feature/reset-password-glowup
branch
2 times, most recently
from
185e2db to
252f660
Compare
| ...props | ||
| }: React.InputHTMLAttributes<HTMLInputElement> & { customValidity: string }) { | ||
| const ref = useRef<HTMLInputElement>(null); | ||
| useLayoutEffect(() => { |
There was a problem hiding this comment.
Can we just use useCustomValidity?
There was a problem hiding this comment.
We can, but the point is to test the abstraction at a component level :)
| newPassword, | ||
| }); | ||
| }, | ||
| onSuccess: async () => { |
| </VStack> | ||
| </VStack> | ||
| </label> | ||
| <label style={{ all: 'unset' }}> |
There was a problem hiding this comment.
Can we refactor all Unstyled*** components with this?
Not for this task, just interesting :)
| this.notificationWindow = notificationWindow; | ||
| this.wallet = new Wallet(TEMPORARY_ID, null, this.notificationWindow); | ||
| this.on('authenticated', () => { | ||
| // TODO: Call Account.writeCurrentUser() here, too? |
There was a problem hiding this comment.
Should we do anything with this TODO?
| newCredentials: SessionCredentials; | ||
| }) { | ||
| const { mnemonic: encryptedMnemonic } = this.getFirstWallet(); | ||
| invariant(encryptedMnemonic, 'Must be a Mnemonic WalletContainer'); |
There was a problem hiding this comment.
the message just don't make sense to me... we have smth similar in static async restoreWalletContainer, however not that important :)
everdimension
force-pushed
the
feature/reset-password-glowup
branch
from
ebc9f36 to
5a0732f
Compare
Implement backup and restore mechanics
everdimension
force-pushed
the
feature/reset-password-glowup
branch
from
5a0732f to
9248c0c
Compare
| return super.getState(); | ||
| } | ||
|
|
||
| setState(...args: Parameters<Store<T>['setState']>) { |
There was a problem hiding this comment.
Should we specify setState as async here?
There was a problem hiding this comment.
It wouldn't change the type though
There was a problem hiding this comment.
Yes! But will make it a bit more obvious :)
| ); | ||
| } | ||
| } | ||
| super.setState(...args); |
There was a problem hiding this comment.
As I can see here, we have both sync and async state updates. Can this cause the racecondition when sync updates will be called in one order, but async saves will be executed in another order which will cause incorrect state after store reload?
| user: PublicUser; | ||
| password: string; | ||
| }>): Promise<PublicUser | null> { | ||
| invariant(!this.account.isWriteLocked, 'Atomic operation in progress'); |
There was a problem hiding this comment.
shouldn't logout also be blocked while updating is in progress?
There was a problem hiding this comment.
Also it looks like we should lock all Account-write operations while update is in progress
| newCredentials: SessionCredentials; | ||
| }) { | ||
| await this.ready(); | ||
| console.log('reading', { id, credentials, state: this.getState() }); |
There was a problem hiding this comment.
Looks dangerous to expose credentials :)
| private async encryptAndSave( | ||
| id: string, | ||
| encryptionKey: string, | ||
| credentials: Credentials, |
There was a problem hiding this comment.
do we need to pass all Credentials here?
There was a problem hiding this comment.
No, but it makes it more consistent with the other signatures. I think it's more proper to pass credentials, it's easier to abstract them this way
| await this.encryptAndSave(id, credentials, record); | ||
| } | ||
|
|
||
| async createBackup(id: string) { |
There was a problem hiding this comment.
what do you think about calling this param userId to make to purpose more clear?
| name: 'internal_error', | ||
| message: 'Atomic wallet update failed. Restore from backup failed.', | ||
| }); | ||
| throw new InternalBackupError(getError(error), { didRestore: false }); |
There was a problem hiding this comment.
Do we need this param for this Error? It looks like we only use it with didRestore: false
No description provided.