Safe-react: Safe settings: Manage owners

[tschubotz]

Story
As a user I am able to add and remove owners to/from my Safe in order to account for changing access rights.

Acceptance criteria
_Owner list_

• User can see list of all Safe owners
• List is sorted alphabetically by owner name
• Hovering over an item displays action buttons for
  
  
  
  • Change the owner's name
  
  
  • Replace the owner with a new one
  
  
  • Remove the owner
  
  
• Users can add a new owner

_Change owner name_

• User cannot enter an empty name
• Clicking the address opens Etherscan
• New name is saved on clicking "Save"

_Add new owner_

• Owner name is required, cannot be empty.
• Address is validated to be a valid Ethereum address

• User can click addresses to open Etherscan

_Remove owner_

_Replace owner_

• When replacing an owner, the user cannot change the confirmation threshold since this would require 2 transactions (or multi send)

[tschubotz]

Found some smaller UX issues:

(1)
When changing the name of an owner and I hit "save", it takes ~2-3s until the name is actually updated. This confused me the first time as I thought it did not save correctly.

(2)

Safe owners are not sorted alphabetically. EDIT: At some point they magically where sorted oO.

(3)
How do are ETH addresses validated?

• Put in checksummed address (0x0e329fa8d6Fcd1ba0Cda495431F1f7CA24F442C2) -> all good.
• Change last char to 3 -> Invalid
• Put in non-checksummed address (0x0e329fa8d6fcd1ba0cda495431f1f7ca24f442c2) -> all good.
• Change last char to 3 -> Valid
  Does that mean we detect if a user entered a checksummed address? Or why is the outcome of case 2 and 4 different?

(4)

When replacing an owner, seems like the threshold is not filled in properly on the left side.

(5)

My nice emoji owner turned into an UNKNOWN magically at some point. I think this happened after a replace or remove owner tx.

[mikheevm]

@tschubotz
About 3)
We use isAddress function from web3.js: https://web3js.readthedocs.io/en/1.0/web3-utils.html#isaddress

  web3.utils.isAddress('0x0e329fa8d6fcd1ba0cda495431f1f7ca24f442c2') -> true
  web3.utils.isAddress('0x0e329fa8d6Fcd1ba0Cda495431F1f7CA24F442C2') -> true
  web3.utils.isAddress('0x0e329fa8d6fcd1ba0cda495431f1f7ca24f442c3') -> true
  web3.utils.isAddress('0x0e329fa8d6Fcd1ba0Cda495431F1f7CA24F442C3') -> false

From the documentation:

It will also check the checksum, if the address has upper and lowercase letters.

web3.utils.toChecksumAddress('0x0e329fa8d6fcd1ba0cda495431f1f7ca24f442c3') -> '0x0E329Fa8d6fCd1BA0cDA495431F1F7ca24F442c3'

So the address 0x0e329fa8d6Fcd1ba0Cda495431F1f7CA24F442C3 has uppercase letters, so it also checks the checksum, but it's not valid, hence it's not a valid address

Hope I got it right 😄

[lukasschor]

Some things that I wrote down as possible improvements:

• Show owner structure also in read mode (so one does not have to go to etherscan to check which wallet should be used to interact with this safe in case I have multiple wallets / safes)
• Show "removing/replacing/adding in progress state" while action is performed on the blockchain
• Not allow to replace with address which is already an owner (currently its possible but the tx eventually fails and owner gets reset to previous one with UNKNOWN name) -> Rather show an error when user inputs a duplicate owner as a replacement (we already show an error when adding new owners that are duplicates)

However, I don't think we have to implement them right now. So I have added them to the backlog to be prioritised.

@tschubotz Regarding 5), I guess this happens when you try to replace with an address thats already in the owner list. It replaces with the duplicate address and overwrites the locally stored name with the new one. But because its not possible to replace with a duplicate address, it gets reset after 1-2 seconds with the old owner but does not have the name stored.

[mikheevm]

@lukasschor I think we can do 1 and 3 before the PR gets merged, it's pretty easy to implement.

[lukasschor]

@mikheevm Sure, if it doesn't take much time to implement, it would definitely be cool to have it. :)

[lukasschor]

@germartinez Another improvement we could make for this menu option is to add the following explanation text: "Add, remove and replace owners or rename existing owners. Owner names are only stored locally and never shared with Gnosis or any third parties."

If it's still feasible to add this in this PR feel free to do so, otherwise, I'll just add it to the backlog for later.