Editable Prefixed Address Input EIP-3770 #2954

DaniSomoza · 2021-11-03T18:02:38Z

What it solves

Resolves #2756

How this PR fixes it

Added network prefix validations in AddressInput and AddressBookInput components.

Added unit tests to AddressInput and AddressBookInput components, and getAddressWithoutNetworkPrefix and getNetworkPrefix util functions

How to test it

Places where the Address input appears:

Load Safe Form (Step two, in the load safe address input).
Create Safe Form (Step three, Owners and Confirmations, in the Owners address inputs).
Settings > Owners section, Add & Replace owners Dialogs, in the Owners address inputs.
AddressBook Section > Create Entry Dialog
New Transaction > Send Founds Dialog, Recipient input.
New Transaction > Send collectible Dialog, Recipient input.
New Transaction > Contract Interaction, Contract Address input.

Use cases

Use a valid network prefix in a address input (rin:0xC245cb45B044d66fbE8Fb33C26c0b28B4fc367B2)
Network prefixes that don't match with the current network show an error ("The current network doesn't match the given address")
Invalid network prefix shows an error ("Unsupported network prefix")
Checksum address with a valid network prefix (rin:0X9913B9180C20C6B0F21B6480C84422F6EBC4B808 -> rin:0x9913B9180C20C6b0F21B6480c84422F6ebc4B808)
Load an address with a QR code should work.
Load an address with ENS name should work.
All the validations like "duplicated owner address" should work with prefixed address

Screenshots

…ix utils fn

github-actions · 2021-11-03T18:02:54Z

CLA Assistant Lite All Contributors have signed the CLA.

github-actions · 2021-11-03T18:07:23Z

ESLint Summary View Full Report

Annotations are provided inline on the Files Changed tab. You can also see all annotations that were generated on the annotations page.

Type	Occurrences	Fixable
Errors	0	0
Warnings	0	0
Ignored	8	N/A

Result: ✅ success
Annotations: 0 total

^{Report generated by eslint-plus-action}

github-actions · 2021-11-03T18:09:15Z

Safe Rinkeby
Safe Mainnet
Safe Polygon
Safe BSC
Safe Arbitrum
Unified app on dev: https://safe-team.dev.gnosisdev.com/app/

coveralls · 2021-11-03T18:12:52Z

Pull Request Test Coverage Report for Build 1438938297

47 of 50 (94.0%) changed or added relevant lines in 9 files are covered.
12 unchanged lines in 5 files lost coverage.
Overall coverage increased (+1.06%) to 32.621%

Changes Missing Coverage	Covered Lines	Changed/Added Lines	%
src/routes/safe/components/Balances/SendModal/screens/AddressBookInput/index.tsx	10	11	90.91%
src/routes/safe/components/Settings/ManageOwners/AddOwnerModal/screens/OwnerForm/index.tsx	0	1	0.0%
src/routes/safe/components/Settings/ManageOwners/ReplaceOwnerModal/screens/OwnerForm/index.tsx	0	1	0.0%

Files with Coverage Reduction	New Missed Lines	%
src/routes/safe/components/Balances/SendModal/screens/AddressBookInput/index.tsx	1	66.67%
src/routes/safe/components/Settings/ManageOwners/AddOwnerModal/screens/OwnerForm/index.tsx	1	0%
src/routes/safe/components/Settings/ManageOwners/ReplaceOwnerModal/screens/OwnerForm/index.tsx	1	0%
src/routes/safe/components/Apps/components/ConfirmTxModal/index.tsx	2	80.56%
src/routes/safe/components/Apps/components/ConfirmTxModal/ReviewConfirm.tsx	7	64.84%

Totals
Change from base Build 1435451809:	1.06%
Covered Lines:	3072
Relevant Lines:	8447

💛 - Coveralls

github-actions · 2021-11-03T18:17:37Z

E2E Tests Failed
Check the results here: https://github.com/gnosis/safe-react-e2e-tests/actions/runs/1438971088

Failed tests:

❌ Add an existing safe Add an existing safe
❌ Address book Address book
❌ Safe Apps List Safe Apps List
❌ Safe Balances Safe Balances

…network

iamacook

Looking good. Just a few points

iamacook · 2021-11-04T15:27:35Z

src/components/forms/AddressInput/index.tsx

+        name={name}
+        inputProps={{
+          value: showNetworkPrefix ? prefixedAddress : value,
+          onChange: (e) => {


You are using two onChange events in this component. Can they be combined in one?

The onChange of the line 71 is related with the input, which contains the prefixed address value (rin:0x123... format).
The other onChange is related with the value that is stored in the react-final-form state (address without prefix 0x123... format).
It can not be combined because if we change the address formats in the state of all forms, we will need to update all the flows to adapt them to the new prefixed address format.

As per our call, you could theoretically remove the input onChange and also reliance on the state by prepending the shortName here:

inputProps={{ value: `${showPrefix ? getCurrentShortChainName() : ''}${values[name]}` }}

iamacook · 2021-11-04T15:30:03Z

src/routes/CreateSafePage/steps/OwnersAndConfirmationsNewSafeStep.tsx

@@ -142,6 +142,7 @@ function OwnersAndConfirmationsNewSafeStep(): ReactElement {
                        createSafeForm.change(nameFieldName, addressName)
                      }
                    }}
+                    value={createSafeFormValues[addressFieldName]}


Why are you passing a value here? Cannot all the logic of prepending the shortName occur inside the component?

iamacook · 2021-11-04T15:30:36Z

src/routes/LoadSafePage/steps/LoadSafeAddressStep.tsx

@@ -154,6 +155,7 @@ function LoadSafeAddressStep(): ReactElement {
                ),
              }
            }
+            value={loadSafeFormValues[FIELD_LOAD_SAFE_ADDRESS]}


Here is another instance of passing the value manually.

iamacook · 2021-11-04T15:30:40Z

src/routes/safe/components/AddressBook/CreateEditEntryModal/index.tsx

@@ -105,6 +105,7 @@ export const CreateEditEntryModal = ({
                        fieldMutator={mutators.setOwnerAddress}
                        name="address"
                        placeholder="Address*"
+                        value={formState.values.address}


Here is another instance of passing the value manually.

iamacook · 2021-11-04T15:32:42Z

src/utils/addNetworkPrefix.ts

+
+  // if no prefix is present in the provided address we return the prefixed address
+  if (!prefix && address) {
+    const prefixedAddress = `${netWorkPrefix}:${address}`


You can use getPrefixedSafeAddressSlug() for this.

Are you sure about this? it is specific for the Safe Addresses that are present in the Url. In that function Prefix argument is a enum SHORT_NAME, but in the input users can type another values for the prefix.

Why do you want to allow invalid (currently unsupported in the Safe) shortNames?

It is needed to trigger the network prefix validation in the form.
If the user types an address like: vt:0x57CB13cbef735FbDD65f5f2866638c546464E45F in Rinkeby we need to set this invalid value in the form state to make sure that the form is aware that a network prefix error is present in that address field.

src/utils/getAddressWithoutNetworkPrefix.ts

iamacook · 2021-11-04T15:33:34Z

src/utils/addNetworkPrefix.ts

+    return address
+  }
+
+  const prefix = getNetworkPrefix(address)


You can use getCurrentShortChainName() for this.

getCurrentShortChainName always returns the shortName of the current selected network, but the address that the user types in the input could be could be incorrect.

Also an oversight. Maybe it's good to use isValidShortChainName to parse it before returning it.

iamacook · 2021-11-04T15:34:20Z

src/routes/safe/components/Settings/ManageOwners/AddOwnerModal/screens/OwnerForm/index.tsx

@@ -127,6 +128,7 @@ export const OwnerForm = ({ onClose, onSubmit, initialValues }: OwnerFormProps):
                      placeholder="Owner address*"
                      testId={ADD_OWNER_ADDRESS_INPUT_TEST_ID}
                      text="Owner address*"
+                      value={formState.values.ownerAddress}


Here is another instance of passing the value manually.

iamacook · 2021-11-04T15:34:26Z

src/routes/safe/components/Settings/ManageOwners/ReplaceOwnerModal/screens/OwnerForm/index.tsx

@@ -155,6 +156,7 @@ export const OwnerForm = ({ onClose, onSubmit, owner, initialValues }: OwnerForm
                      placeholder="Owner address*"
                      testId={REPLACE_OWNER_ADDRESS_INPUT_TEST_ID}
                      text="Owner address*"
+                      value={formState.values.ownerAddress}


Here is another instance of passing the value manually.

iamacook · 2021-11-04T15:38:24Z

src/components/forms/AddressInput/index.tsx

+        inputAdornment={inputAdornment}
+        name={name}
+        inputProps={{
+          value: showNetworkPrefix ? prefixedAddress : value,


Could you not use an effect, with the user preference as a dependency to adjust the value on render/the preference changes? I'm not sure if it's the best idea to have two sources of truth. This would also remove the requirement of passing the value prop.

katspaugh · 2021-11-08T10:59:27Z

src/components/forms/AddressInput/index.tsx

+                  setPrefixedAddress(addNetworkPrefix(checkedAddress, showNetworkPrefix, prefix))
+                } else {
+                  // this is needed to avoid remove the : char and no prefix is present
+                  const containsTwoDots = prefixedAddress.includes(':')


containsTwoDots -> isPrefixed

katspaugh · 2021-11-08T11:00:55Z

src/components/forms/AddressInput/index.tsx

+                if (prefix) {
+                  setPrefixedAddress(addNetworkPrefix(checkedAddress, showNetworkPrefix, prefix))
+                } else {
+                  // this is needed to avoid remove the : char and no prefix is present


Could you rephrase this? Not clear what you mean.

yes! sorry for that!
I refactored it to be more clear:

// checksum the address in the input checkedAddress = checksumAddress(address) const prefix = getNetworkPrefix(prefixedAddress) const isPrefixed = prefix || prefixedAddress.includes(':') // we set the correct network prefix in the state if (isPrefixed) { setPrefixedAddress(addNetworkPrefix(checkedAddress, showNetworkPrefix, prefix)) } else { setPrefixedAddress(addNetworkPrefix(checkedAddress, showNetworkPrefix, getCurrentShortChainName())) }

katspaugh · 2021-11-08T11:01:34Z

src/components/forms/validator.ts

+
+  const prefix = getNetworkPrefix(value)
+
+  const isInValidPrefix = prefix && prefix !== extractShortChainName()


isInvalidPrefix (invalid is one word)

katspaugh · 2021-11-08T11:03:32Z

src/components/forms/validator.ts

@@ -91,6 +93,20 @@ export const mustBeEthereumAddress = memoize((address: string): ValidatorReturnT
  return result
 })

+export const checkNetworkPrefix = (value: string): ValidatorReturnType => {
+  const errorMessage = "The current network doesn't match the given address"


Should we have a separate error message for prefixes that aren't valid shortNames?

Done!

I added the Unsupported network prefix error label in the validator:

const errorMessage = isValidShortChainName(prefix) ? "The current network doesn't match the given address" : 'Unsupported network prefix'

And also added a unit test to this new case:

it('Validates unsupported shortNames', () => { const unsupportedPrefixedAddress = 'xxxx:0x2D42232C03C12f1dC1448f89dcE33d2d5A47Aa33' renderAddressInputWithinForm() // invalid address network prefix fireEvent.change(screen.getByTestId(fieldTestId), { target: { value: unsupportedPrefixedAddress } }) // input value with the network prefix expect((screen.getByTestId(fieldTestId) as HTMLInputElement).value).toBe(unsupportedPrefixedAddress) // show prefix error expect(screen.queryByText('Unsupported network prefix')).toBeInTheDocument() })

katspaugh · 2021-11-08T11:05:34Z

src/utils/getNetworkPrefix.ts

@@ -0,0 +1,8 @@
+function getNetworkPrefix(address = ''): string {
+  const splittedAddress = address.split(':')


Suggested change

const splittedAddress = address.split(':')

const splitAddress = address.split(':')

("splitted" isn't a word)

katspaugh · 2021-11-08T11:06:27Z

Awesome PR! Left some minor comments.

…dator

katspaugh

Thank you 👍

francovenica · 2021-11-15T03:02:37Z

In every input it was tested that the address with our without the index was a valid one. Was also tested the QR code and ENS names.
Lastly it was checked that only the prefix of the currently selected network works (so xdai: cannot work on rinkeby and viceversa)

Sections checked:
Safe creation > Owners
Add existing safe
Send funds
Send collectibles
Contract interaction
Address book add entry
Owner settings Add owner
Owner settings Replace owner
Spending Limit Add Allowance

Looks Good to me

Question:
In the Contract Interaction > Receiver input the address set doesn't show the prefix once you enter a valid address. I've seen this work fine in other inputs like the one in "Add existing safe" and Address book Add entry. So the issue is only how the address shows, but it works fine regarding if the input has the prefix or not. So, it should there the prefix for a valid address or not?
Contract interaction snapshot:

Add existing safe snapshot:

katspaugh · 2021-11-15T07:25:22Z

There's one behavior that I noticed and think is confusing:

Disable prefixes in Settings -> Appearance
Go to Address Book -> Create Entry
Enter rin:0x4f9BD57BCC68Bf7770429F137922B3afD23d83E7
It will remove the rin: part

I think we shouldn't remove prefixes if the user inputted them intentionally.

Edit: definitely not a blocker though! We can iterate on that later.

…address-input-editable

github-actions · 2021-11-16T22:08:47Z

ESLint Summary View Full Report

Annotations are provided inline on the Files Changed tab. You can also see all annotations that were generated on the annotations page.

Type	Occurrences	Fixable
Errors	0	0
Warnings	0	0
Ignored	8	N/A

Result: ✅ success
Annotations: 0 total

^{Report generated by eslint-plus-action}

DaniSomoza added 7 commits November 2, 2021 14:03

Added network prefix to the AddressInput Component

bb03056

Added network prefix to BaseAddressBookInput

033d617

reduced the complexity in AddressInput component

063a2c9

Added AddressInput unit tests

fed95c2

Added a extra test in AddressInput component

4d92082

Added AddressBookInput unit tests

c7e19e2

Added unit tests to getNetworkPrefix and getAddressWithoutNetworkPref…

343c327

…ix utils fn

Merge branch 'dev' into feature/chain-specific-address-input-editable

9ddd0e8

DaniSomoza added 2 commits November 4, 2021 12:28

Minor changes in comments th the AddressInput unit tests

aaec19a

Merge branch 'dev' into feature/chain-specific-address-input-editable

c75e9b5

DaniSomoza marked this pull request as ready for review November 4, 2021 11:59

DaniSomoza mentioned this pull request Nov 4, 2021

Prefixed Address Input component EIP-3770 #2913

Closed

Addresses without a network prefix are set by default to the current …

07bd6d1

…network

iamacook reviewed Nov 4, 2021

View reviewed changes

katspaugh reviewed Nov 8, 2021

View reviewed changes

added improvements in AddressInput and validators

e47ddb3

iamacook approved these changes Nov 8, 2021

View reviewed changes

Added Unsupported network prefix error in the checkNetworkPrefix vali…

3beac5d

…dator

katspaugh approved these changes Nov 8, 2021

View reviewed changes

DiogoSoaress mentioned this pull request Nov 9, 2021

Components support eip3770 prefixes #2896

Merged

Merge branch 'dev' into feature/chain-specific-address-input-editable

f87fc35

DiogoSoaress changed the base branch from dev to feature/eip-3770-prefixes November 16, 2021 08:43

Merge branch 'feature/eip-3770-prefixes' into feature/chain-specific-…

c422749

…address-input-editable

DiogoSoaress merged commit 2b92443 into feature/eip-3770-prefixes Nov 16, 2021

DiogoSoaress deleted the feature/chain-specific-address-input-editable branch November 16, 2021 22:17

github-actions bot locked and limited conversation to collaborators Nov 16, 2021


		const prefix = getNetworkPrefix(value)

		const isInValidPrefix = prefix && prefix !== extractShortChainName()

		@@ -0,0 +1,8 @@
		function getNetworkPrefix(address = ''): string {
		const splittedAddress = address.split(':')

	const splittedAddress = address.split(':')
	const splitAddress = address.split(':')

Editable Prefixed Address Input EIP-3770 #2954

Editable Prefixed Address Input EIP-3770 #2954

Conversation

DaniSomoza commented Nov 3, 2021 • edited Loading

What it solves

How this PR fixes it

How to test it

Use cases

Screenshots

github-actions bot commented Nov 3, 2021

github-actions bot commented Nov 3, 2021 • edited Loading

ESLint Summary View Full Report

github-actions bot commented Nov 3, 2021

coveralls commented Nov 3, 2021 • edited Loading

Pull Request Test Coverage Report for Build 1438938297

💛 - Coveralls

github-actions bot commented Nov 3, 2021 • edited Loading

iamacook left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

katspaugh commented Nov 8, 2021

katspaugh left a comment

Choose a reason for hiding this comment

francovenica commented Nov 15, 2021

katspaugh commented Nov 15, 2021 • edited Loading

github-actions bot commented Nov 16, 2021

ESLint Summary View Full Report

DaniSomoza commented Nov 3, 2021 •

edited

Loading

github-actions bot commented Nov 3, 2021 •

edited

Loading

coveralls commented Nov 3, 2021 •

edited

Loading

github-actions bot commented Nov 3, 2021 •

edited

Loading

katspaugh commented Nov 15, 2021 •

edited

Loading