fix: permit pages breaking if address user is interacting is invalid integer address

[jpuri]

Description

fix: permit pages breaking if address user is interacting is invalid integer address

Related issues

Fixes: #25733

Manual testing steps

1. Go to test dapp
2. Submit confirmation Malicious permit with integer address
3. Metamask should not throw error

Screenshots/Recordings

Pre-merge author checklist

• I've followed MetaMask Contributor Docs and MetaMask Extension Coding Standards.
• I've completed the PR template to the best of my ability
• I’ve included tests if applicable
• I’ve documented my code using JSDoc format if applicable
• I’ve applied the right labels on the PR (see labeling guidelines). Not required for external contributors.

Pre-merge reviewer checklist

• I've manually tested the PR (e.g. pull and build branch, run the app, test code being changed).
• I confirm that this PR addresses all acceptance criteria described in the ticket it closes and includes the necessary testing evidence such as recordings and or screenshots.

[github-actions]

CLA Signature Action: All authors have signed the CLA. You may need to manually re-run the blocking PR check if it doesn't pass in a few minutes.

[sleepytanya]

@jpuri
All PPOM actions work great including Malicious permit with integer address.

[matthewwalsh0]

We now have toChecksumAddress, toChecksumHexAddress, and getChecksumAddress.

Could we re-use or extend toChecksumHexAddress?

Or at least place our new util in the same folder which is shared/modules/hexstring-utils.ts?

[jpuri]

toChecksumAddress is from ethjs-util lobrary. This function is different from toChecksumHexAddress and I can not categorise it as hex utillity

[matthewwalsh0]

My concern is adding to a generic util that's already almost a thousand lines, as opposed to a targeted file so we can keep our logic modular and organised.

Even if they're different, we now have three very similar functions without obvious differences.

I'd argue that hextring-utils.ts is more meaningful than util.js since the checksummed address (if successful) is is a hexadecimal string.

[jpuri]

We can add it to a more specific utility class.

But I am not sure if that should be hex utility, is every method returning a hex value candidate for hextring-utils.ts ?

[matthewwalsh0]

Was the intent of this fix and util not to convert the integer address to hexadecimal rather than just displaying the original value?

[jpuri]

ethUtil.isValidAddress throws error for invalid addresses thus this change keeps from metamask breaking

[jpuri]

we still want to display invalid address to user with error from blockaid to show its wrong

[matthewwalsh0]

The Name component is designed to show Ethereum addresses as hexadecimal strings, not whatever unformatted input is given to it.

We want the UX to be consistent, so it seems the best two choices are to reject the request outright if an address is in a bad format, or normalize it so we can render it in a consistent format, plus this would ensure Blockaid can validate it correctly rather than throwing.

[jpuri]

For a wrong address, I think we should show users that address value as it is so its more obvious to user's what is wrong and not change that.

[matthewwalsh0]

But it's not the user's problem, or even a problem at all if we can just normalize it to hexadecimal then process it as normal.

The average user won't know what to do with an address in a different format to what they're used to, so why should we sacrifice the UX consistency if we can just convert it and then it's no longer an issue?

[jpuri]

Let's discuss this more with product team to reach on decision here.

[digiwand]

A couple of concerns:

1. We should add/update tests for name, name-details, and shortenAddress
2. Resulting UI with shortened invalid addresses is not shown in the PR description. Here are a couple of screenshots of the current UI:
   
   
3. Have we discussed handling invalid addresses with design? It might be confusing to the user because we treat the invalid address as an address despite knowing it is an invalid address. This may not be a blocker for the PR, but I believe we should follow-up on this.

[digiwand]

Interesting that if it is not a valid address, we may still shorten and truncate the value. Can we include tests for these cases for name and name-details?

also, now that shortedAddress may accept an invalid value, I think we should include a test for this. file: ui/helpers/utils/util.test.js

    it('should return the shortened string if it is not a valid address', () => {
      expect(
        util.shortenAddress('1234567890123456789012345678901234567890'),
      ).toStrictEqual('1234567...67890');
    });

[jpuri]

I added it 👍

[jpuri]

@digiwand : invalid address is shown along with blockaid error. Showing invalid address is better than not showing address at all.

I do not think its blocker as we never handled it even before.

[digiwand]

I agree with you. This is why I also mentioned it would not be a blocker.

I do question if it would be an improvement to include more indicators that it is an invalid address rather than an "unknown address". Currently, users need to click "See details" in the banner alert to see it is. This is hidden. This could be a question for @SayaGT

[sonarqubecloud]

Quality Gate passed

Issues
0 New issues
0 Accepted issues

Measures
0 Security Hotspots
0.0% Coverage on New Code
0.0% Duplication on New Code

See analysis details on SonarCloud

[jpuri]

As discussed, we will reject signature like this. Thus closing this PR.