fix: Show approval editor for ERC721 approvals #3991

usame-algan · 2024-07-23T14:41:59Z

What it solves

Resolves #3774

How this PR fixes it

When the ERC20 token info request fails we try the ERC721 contract and fetch the symbol, if that fails we check the ERC-165 function supportsInterface with the ERC721 identifier to at least know if its an ERC721 token or not.

Caveat: If its a multisend that contains both ERC20 and ERC721 approvals all of them will be read-only now.

How to test it

Open the transaction build with an ERC721 contract address
Create an approve transaction and continue
Observe no error messages in the approval editor

Screenshots

Checklist

I've tested the branch on mobile 📱
I've documented how it affects the analytics (if at all) 📊
I've written a unit/e2e test for it (if applicable) 🧑‍💻

github-actions · 2024-07-23T14:42:15Z

Branch preview

✅ Deploy successful!

Website:
https://erc721_approval--walletweb.review.5afe.dev/home?safe=eth:0xA77DE01e157f9f57C7c4A326eeE9C4874D0598b6

Storybook:
https://erc721_approval--walletweb.review.5afe.dev/storybook/

github-actions · 2024-07-23T14:43:02Z

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}

github-actions · 2024-07-23T14:45:33Z

📦 Next.js Bundle Analysis for safe-wallet-web

This analysis was generated by the Next.js Bundle Analysis action. 🤖

⚠️ Global Bundle Size Increased

Page	Size (compressed)
`global`	`948.79 KB` (🟡 +39 B)

Details

The global bundle is the javascript bundle that loads alongside every page. It is in its own category because its impact is much higher - an increase to its size means that every page on your website loads slower, and a decrease means every page loads faster.

Any third party scripts you have added directly to your app using the <script> tag are not accounted for in this analysis

If you want further insight into what is behind the changes, give @next/bundle-analyzer a try!

usame-algan · 2024-07-23T14:46:23Z

src/components/tx/ApprovalEditor/index.tsx

+  const title = isErc721 ? 'Allow access to tokens?' : 'Allow access to tokens?'
+  const subtitle = isErc721
+    ? 'This allows the spender to transfer the specified token.'
+    : 'This allows the spender to spend the specified amount of your tokens.'


I think we can keep the title the same for both erc20 and erc721.

github-actions · 2024-07-23T14:47:57Z

Coverage report

St.^❔	Category	Percentage	Covered / Total
🟡	Statements	78.52% (-0.12% 🔻)	11709/14912
🔴	Branches	58.44% (-0.07% 🔻)	2924/5003
🟡	Functions	65.91% (-0.1% 🔻)	1875/2845
🟡	Lines	79.88% (-0.11% 🔻)	10551/13208

Show files with reduced coverage 🔻

St.^❔	File	Statements	Branches	Functions	Lines
🟢	`...` / index.tsx	82.35% (-8.82% 🔻)	66.67% (-16.67% 🔻)	50% (-12.5% 🔻)	83.87% (-9.68% 🔻)
🟢	`...` / index.tsx	94.44% (+0.9% 🔼)	77.78% (-2.22% 🔻)	100%	96.88% (+0.45% 🔼)
🔴	src/utils/tokens.ts	51.43% (-48.57% 🔻)	33.33% (-66.67% 🔻)	33.33% (-66.67% 🔻)	53.85% (-46.15% 🔻)
🟢	`...` / useApprovalInfos.ts	91.43% (-8.57% 🔻)	86.67% (-13.33% 🔻)	100%	90.91% (-9.09% 🔻)
🟢	`...` / tokenTransferParams.ts	96.15% (-3.85% 🔻)	80%	80% (-20% 🔻)	95.65% (-4.35% 🔻)

Test suite run success

1447 tests passing in 200 suites.

Report generated by 🧪jest coverage report action from 9562263

schmanu · 2024-07-26T08:11:21Z

src/components/tx/ApprovalEditor/hooks/useApprovalInfos.ts

+            try {
+              tokenInfo = await getERC20TokenInfoOnChain(approval.tokenAddress)
+            } catch (e) {
+              isErc721 = await isErc721Token(approval.tokenAddress)


We should also try to fetch some minimal token information here. At least the name or symbol of the collection for instance.

Also we need to display the id that you are giving your approval to.
Currently the modal says "This allows the spender to transfer the specified token."
But we do not specify any token underneath.

Good point, I added a call to fetch the symbol and decided to only fetch the symbol instead of the name or both because they are part of the same contract so either both exist or both don't and symbol is already part of the approval object.

I've also added a different message and included the token id. Let me know if this works for you:

github-actions · 2024-07-26T09:39:14Z

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

Thanks for adding the info 🚀

liliya-soroka · 2024-08-01T08:58:40Z

@usame-algan , I see only a collection name. Do we agree to display only it ?

liliya-soroka · 2024-08-01T09:19:26Z

Verified. We are agreed to display the collection name and TokenID
The combination of ERC20 approve and ERC721 approve for different spenders are also checked

usame-algan · 2024-08-01T09:22:42Z

@usame-algan , I see only a collection name. Do we agree to display only it ?

I would suggest for simplicity that we only display the symbol since we only load NFTs when the user visits the NFT page so they are most likely not there.

github-actions · 2024-08-01T09:28:58Z

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}

liliya-soroka · 2024-08-01T09:59:21Z

verified

fix: Show approval editor for ERC721 approvals

eeec7ed

usame-algan requested a review from schmanu July 23, 2024 14:41

usame-algan commented Jul 23, 2024

View reviewed changes

fix: Adjust title

d48b20a

schmanu requested changes Jul 26, 2024

View reviewed changes

fix: Try to fetch ERC721 symbol, remove flag and use built-in type

b9c3ca6

usame-algan requested a review from schmanu July 26, 2024 09:40

schmanu approved these changes Jul 30, 2024

View reviewed changes

fix: Add # prefix so its clearer that it is a token id

186e851

Merge remote-tracking branch 'origin/dev' into erc721-approval

9562263

usame-algan merged commit 3a4c823 into dev Aug 1, 2024
14 checks passed

usame-algan deleted the erc721-approval branch August 1, 2024 10:39

github-actions bot locked and limited conversation to collaborators Aug 1, 2024

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

fix: Show approval editor for ERC721 approvals #3991

fix: Show approval editor for ERC721 approvals #3991

usame-algan commented Jul 23, 2024 •

edited

Loading

github-actions bot commented Jul 23, 2024 •

edited

Loading

github-actions bot commented Jul 23, 2024 •

edited

Loading

github-actions bot commented Jul 23, 2024 •

edited

Loading

usame-algan Jul 23, 2024

github-actions bot commented Jul 23, 2024 •

edited

Loading

schmanu Jul 26, 2024

usame-algan Jul 26, 2024

github-actions bot commented Jul 26, 2024

schmanu left a comment

liliya-soroka commented Aug 1, 2024

liliya-soroka commented Aug 1, 2024

usame-algan commented Aug 1, 2024

github-actions bot commented Aug 1, 2024 •

edited

Loading

liliya-soroka commented Aug 1, 2024

fix: Show approval editor for ERC721 approvals #3991

fix: Show approval editor for ERC721 approvals #3991

Conversation

usame-algan commented Jul 23, 2024 • edited Loading

What it solves

How this PR fixes it

How to test it

Screenshots

Checklist

github-actions bot commented Jul 23, 2024 • edited Loading

Branch preview

github-actions bot commented Jul 23, 2024 • edited Loading

ESLint Summary View Full Report

github-actions bot commented Jul 23, 2024 • edited Loading

📦 Next.js Bundle Analysis for safe-wallet-web

⚠️ Global Bundle Size Increased

usame-algan Jul 23, 2024

Choose a reason for hiding this comment

github-actions bot commented Jul 23, 2024 • edited Loading

Coverage report

Test suite run success

schmanu Jul 26, 2024

Choose a reason for hiding this comment

usame-algan Jul 26, 2024

Choose a reason for hiding this comment

github-actions bot commented Jul 26, 2024

ESLint Summary View Full Report

schmanu left a comment

Choose a reason for hiding this comment

liliya-soroka commented Aug 1, 2024

liliya-soroka commented Aug 1, 2024

usame-algan commented Aug 1, 2024

github-actions bot commented Aug 1, 2024 • edited Loading

ESLint Summary View Full Report

liliya-soroka commented Aug 1, 2024

verified

usame-algan commented Jul 23, 2024 •

edited

Loading

github-actions bot commented Jul 23, 2024 •

edited

Loading

github-actions bot commented Jul 23, 2024 •

edited

Loading

github-actions bot commented Jul 23, 2024 •

edited

Loading

github-actions bot commented Jul 23, 2024 •

edited

Loading

github-actions bot commented Aug 1, 2024 •

edited

Loading