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
amaury1093
left a comment
There was a problem hiding this comment.
When importing an existing account, the address is truncated by the window. Maybe we can use the AddressShort component to render this?
| 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
|
|
||
| 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[]
Tbaut
left a comment
There was a problem hiding this comment.
Thanks for the pr! Just a nit from my side.
This should not be addressed by this pr but we should rethink the way we present errors and warning message. The errors at least should be more visible.
| 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.
| this.setState({ isLoading: true }); | ||
|
|
||
| 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. |
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