fix: address book undefined entry address #1338

DiogoSoaress · 2022-12-09T10:44:25Z

What it solves

Resolves #1283

How this PR fixes it

Validates the address book in the selector

How to test it

I've added a unit test for it.
Alternatively, I was testing it locally by setting a faulty addressBookSlice initialSate.

const initialState: AddressBookState = {
  5: {
    '0x744aaf04ad770895ce469300771d2ca38463cfa0': 'legit',
    undefined: 'bug',
  },
}

github-actions · 2022-12-09T10:44:42Z

Branch preview

✅ Deploy successful!

https://fix_undefined_address_input--webcore.review-web-core.5afe.dev

github-actions · 2022-12-09T10:47:05Z

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	0	N/A

Result: ✅ success
Annotations: 0 total

^{Report generated by eslint-plus-action}

schmanu

I'm not sure if we should even overwrite the addressBook with the valid entries instead of always filtering them out.
But as this should not exist outside of dev / QA Safes it's probably also not too important.

The validation is not checking for correct checksums though which is required for our address book entries.

schmanu · 2022-12-09T10:57:27Z

src/store/addressBookSlice.ts

@@ -45,6 +47,8 @@ export const selectAllAddressBooks = (state: RootState): AddressBookState => {
 export const selectAddressBookByChain = createSelector(
  [selectAllAddressBooks, (_, chainId: string) => chainId],
  (allAddressBooks, chainId): AddressBook => {
-    return chainId ? allAddressBooks[chainId] || {} : {}
+    const chainAddresses = allAddressBooks[chainId]
+    const validAddresses = pickBy(chainAddresses, (_, key) => ADDRESS_RE.test(key))


We have a function to validate addresses. This additionally validates that addresses are checksummed, which we also expect in our addressbook.

Suggested change

const validAddresses = pickBy(chainAddresses, (_, key) => ADDRESS_RE.test(key))

const validAddresses = pickBy(chainAddresses, (_, key) => validateAddress(key) === undefined)

It will not validate the address if it isn't checksummed. I didn't use it because it was filtering out the non-checksummed addresses

schmanu · 2022-12-09T10:59:18Z

src/utils/validation.ts

@@ -2,9 +2,8 @@ import chains from '@/config/chains'
 import { parsePrefixedAddress, sameAddress, isChecksummedAddress } from './addresses'
 import { safeFormatUnits, safeParseUnits } from './formatters'

+export const ADDRESS_RE = /^0x[0-9a-f]{40}$/i


I'd move it back into validateAddress because that is the function which always should be used IMO.

If we need a helper that returns a boolean we should add something like

export const isValidAddress = (address: string) => { return validateAddress(address) === undefined }

Then we should probably split validateAddress function because it is doing two things at the moment:

validating the address format

validating if checksum

Initially I was using that function but it would filter out any address not checksummed and I think we don't want to go that strict in the AB, right?

If we need a helper that returns a boolean

I think we can use the validation as it is -> returning undefined if passed all checks

We checksum every address which gets added to the address book. And if its incorrectly checksummed it will not let you add that address. So I think we should go quite strict.
Some APIs only work with checksummed addresses including our tx service for instance.

If we validate them when added to the AB, and in address inputs, validating also in the selector seems like an overkill. And it's computationally not the fastest.

I've included the checksum validation. Reason being an unchecksummed address might be introduced during a development stage, the same way the corrupted AB with an undefined entry was.

And it's computationally not the fastest.

I didn't experience any degradation of performance but to be fair my AB is not extensively populated.

schmanu · 2022-12-09T11:02:28Z

src/store/__tests__/addressBookSlice.test.ts

@@ -54,4 +60,17 @@ describe('addressBookSlice', () => {
      '4': { '0x0': 'Charlie', '0x1': 'Dave' },
    })
  })
+
+  it('should not return entries with invalid address format', () => {
+    const initialState = {


Kudos for adding a test :)

src/utils/validation.ts

katspaugh

The code looks good!

Did you test in the UI with a bogus AB in LS?

DiogoSoaress · 2022-12-09T11:46:05Z

The validation is not checking for correct checksums though which is required for our address book entries.

@schmanu so should the selector return only checksummed valid addresses?

DiogoSoaress · 2022-12-09T11:46:54Z

Did you test in the UI with a bogus AB in LS?

Yes! I was able to reproduce the original error with the slice state I included in the PR description :) but added a unit test nevertheless

katspaugh · 2022-12-09T12:37:16Z

src/utils/validation.ts

-export const validateAddress = (address: string) => {
-  const ADDRESS_RE = /^0x[0-9a-f]{40}$/i
+const ADDRESS_RE = /^0x[0-9a-f]{40}$/i
+export const isAddress = (address: string) => ADDRESS_RE.test(address)

-  if (!ADDRESS_RE.test(address)) {


Please restore it how it was if it's not used.

schmanu

Looking good now! 🥳

iamacook · 2022-12-19T09:09:14Z

Using the example initial state (which includes one valid address), the crash no longer happens. However, it does not return any entries in the address book or send asset modal. I think it should at least show the valid address? I tested this in Firefox and Chrome.

schmanu · 2022-12-19T09:16:19Z

@iamacook I think the address from the example 0x744aaf04ad770895ce469300771d2ca38463cfa0 is not accepted because it is not checksummed. So I guess the example is not correct. Usually addresses should also always be checksummed in the addressbook.
If I checksum the address first to 0x744aAF04AD770895Ce469300771D2CA38463cFa0 it works.

gitpoap-bot · 2022-12-19T11:25:23Z

Congrats, your important contribution to this open-source project has earned you a GitPOAP!

GitPOAP: 2022 Safe Web Core Contributor:

Head to gitpoap.io & connect your GitHub account to mint!

Learn more about GitPOAPs here.

DiogoSoaress added 2 commits December 9, 2022 11:15

fix: validate the address book entries in the selector

aca50fc

add a test for the selector

0d6089c

DiogoSoaress requested review from schmanu and katspaugh December 9, 2022 10:45

iamacook approved these changes Dec 9, 2022

View reviewed changes

schmanu requested changes Dec 9, 2022

View reviewed changes

schmanu reviewed Dec 9, 2022

View reviewed changes

katspaugh reviewed Dec 9, 2022

View reviewed changes

src/utils/validation.ts Outdated Show resolved Hide resolved

create util isAddress instead of exporting regex

c940445

katspaugh approved these changes Dec 9, 2022

View reviewed changes

filter out unchecksummed addresses in the AB selector

d646e80

DiogoSoaress requested a review from schmanu December 9, 2022 11:59

katspaugh reviewed Dec 9, 2022

View reviewed changes

restore previous validateAddress util

4dfffd5

schmanu approved these changes Dec 9, 2022

View reviewed changes

Merge branch 'dev' into fix-undefined-address-input

56e921e

katspaugh merged commit 07fc3b3 into dev Dec 19, 2022

katspaugh deleted the fix-undefined-address-input branch December 19, 2022 11:25

github-actions bot locked and limited conversation to collaborators Dec 19, 2022

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

fix: address book undefined entry address #1338

fix: address book undefined entry address #1338

DiogoSoaress commented Dec 9, 2022

github-actions bot commented Dec 9, 2022 •

edited

Loading

github-actions bot commented Dec 9, 2022 •

edited

Loading

schmanu left a comment

schmanu Dec 9, 2022

DiogoSoaress Dec 9, 2022

schmanu Dec 9, 2022

DiogoSoaress Dec 9, 2022

DiogoSoaress Dec 9, 2022

schmanu Dec 9, 2022

katspaugh Dec 9, 2022

DiogoSoaress Dec 9, 2022

DiogoSoaress Dec 9, 2022

schmanu Dec 9, 2022

katspaugh left a comment

DiogoSoaress commented Dec 9, 2022

DiogoSoaress commented Dec 9, 2022

katspaugh Dec 9, 2022

DiogoSoaress Dec 9, 2022

schmanu left a comment

iamacook commented Dec 19, 2022 •

edited

Loading

schmanu commented Dec 19, 2022

gitpoap-bot bot commented Dec 19, 2022

	const validAddresses = pickBy(chainAddresses, (_, key) => ADDRESS_RE.test(key))
	const validAddresses = pickBy(chainAddresses, (_, key) => validateAddress(key) === undefined)

fix: address book undefined entry address #1338

fix: address book undefined entry address #1338

Conversation

DiogoSoaress commented Dec 9, 2022

What it solves

How this PR fixes it

How to test it

github-actions bot commented Dec 9, 2022 • edited Loading

Branch preview

github-actions bot commented Dec 9, 2022 • edited Loading

ESLint Summary View Full Report

schmanu 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

katspaugh left a comment

Choose a reason for hiding this comment

DiogoSoaress commented Dec 9, 2022

DiogoSoaress commented Dec 9, 2022

Choose a reason for hiding this comment

Choose a reason for hiding this comment

schmanu left a comment

Choose a reason for hiding this comment

iamacook commented Dec 19, 2022 • edited Loading

schmanu commented Dec 19, 2022

gitpoap-bot bot commented Dec 19, 2022

github-actions bot commented Dec 9, 2022 •

edited

Loading

github-actions bot commented Dec 9, 2022 •

edited

Loading

iamacook commented Dec 19, 2022 •

edited

Loading