fix: Relates to #138. Prevents user from importing account that is already in account list #327
Conversation
… that is already listed
…e when address already listed
| import { inject, observer } from 'mobx-react'; | ||
|
|
||
| @light({ | ||
| accountsInfo: () => accountsInfo$().pipe(withoutLoading()) |
There was a problem hiding this comment.
You can use accounts$() here: https://parity-js.github.io/light.js/api/modules/_rpc_eth_.html#accounts
| @@ -67,6 +90,22 @@ class AccountImportOptions extends Component { | |||
| } | |||
| }; | |||
|
|
|||
| hasExistingAddressForImport = addressForImport => { | |||
| const { accountsInfo } = this.props; | |||
| const isExistingAddress = Object.keys(accountsInfo) | |||
There was a problem hiding this comment.
Use account$, no need for Object.keys anymore since it returns a string[]
| if (isExistingAddress) { | ||
| this.setState({ | ||
| isLoading: false, | ||
| error: `Account already loaded. Address ${addressForImport} is already in the account list` |
There was a problem hiding this comment.
I would go for something much shorter along the line:
"Account 0x1234..1234 already listed!"
You don't have to see the whole address here.
|
@Tbaut Yes, agreed. I've just been adopting the way errors are current shown, but I personally like using colour-coded animated toasters. Should we create an issue/opportunity for this? |
|
@amaurymartiny @Tbaut I've pushed commits addressing your review comments |
| const { accounts } = this.props; | ||
| const isExistingAddress = accounts | ||
| .map(address => address && address.toLowerCase()) | ||
| .includes(addressForImport); |
There was a problem hiding this comment.
I would put .toLowecase() on this line. So that the input argument can be in any case.
| try { | ||
| const json = JSON.parse(jsonString); |
There was a problem hiding this comment.
Do not parse here.
3 lines below, the await setJsonString(jsonString); actually parses the JSON, and createAccountStore.address will hold the parsed address. Do the hasExistingAddressForImport then.
|
@ltfschoen Yes, I agree the errors are quite ugly. In TxForm, I went with react-final-form to put errors as black tooltips over fields, but that might not be ideal for all types of errors. You can create an issue for us to discuss UI options for errors. |
|
@ltfschoen can you make sure to switch the labels to "please review" and remove "grumbles" once you think you've addressed all the comments? |
Sure! |
|
I've merged latest from master |
|
@ltfschoen There are 2 grumbles left (from me) 4d ago. I'll merge this PR as soon as they are addressed. |
amaury1093
deleted the
luke-138-prevent-import-when-account-already-loaded
branch
Prevents user from importing account and show the message in the screenshot below when they go to "Import account" and select a JSON Backup Keyfile whose address is already associated with an account that is already in the account list
Prevents user from importing account and show the message in the screenshot below when they go to "Import account" and enter a Seed Phrase that corresponds to an address that is associated with an account that is already in the account list