Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Dynamic Unfurl while sharing link #90

Merged
merged 14 commits into from
Apr 30, 2024
Merged

Dynamic Unfurl while sharing link #90

merged 14 commits into from
Apr 30, 2024

Conversation

technophile-04
Copy link
Member

@technophile-04 technophile-04 commented Apr 24, 2024

Description

Solves #89, with patch to @noble/hashes as it was mentioned in wevm/viem#368

The patch makes sense but not 100% sure.

Incase we plan to merge this, we should remove this when we update wagmi / viem version + we may need to update nextjs version too (not app router but just next version)...(I think we are already planning to update versions, right?)

Or else maybe we can wait until we have wagmi / viem version upgraded and then this should work out of box without patch

Also lol we should add: Co-authored-by: port <108868128+portdeveloper@users.noreply.github.com> while doing squash and merge I just copy pasted his code from #90 while tinkering.

Co-authored-by: port <108868128+portdeveloper@users.noreply.github.com>
Copy link

vercel bot commented Apr 24, 2024

The latest updates on your projects. Learn more about Vercel for Git ↗︎

Name Status Preview Comments Updated (UTC)
abi-ninja-v2 ✅ Ready (Inspect) Visit Preview 💬 Add feedback Apr 29, 2024 7:07pm

@carletex
Copy link
Collaborator

The patch makes sense but not 100% sure.

Seems good to me!

I think this might be a good option until we have an updated version (which I think we should do at some point, but I don't think is super urgent).

I let other review this too

@carletex
Copy link
Collaborator

BTW, this also exists in NextJS (https://nextjs.org/docs/app/api-reference/file-conventions/metadata/opengraph-image#generate-images-using-code-js-ts-tsx)

You can create a dynamic OG image with opengraph-image.tsx, without a API route.

But maybe only in NextJS 14 + App router?

@portdeveloper
Copy link
Member

BTW, this also exists in NextJS (https://nextjs.org/docs/app/api-reference/file-conventions/metadata/opengraph-image#generate-images-using-code-js-ts-tsx)

You can create a dynamic OG image with opengraph-image.tsx, without a API route.

But maybe only in NextJS 14 + App router?

Yes, I think you are right, I couldn't find the same thing for pages directory.

@technophile-04
Copy link
Member Author

Tysm @portdeveloper, just tested it out, and works great !!

To test, you should test it locally because if you open deployment URL from vercel it won't work, since we have hardcode https://abi.ninja domain their in MetaHeader

Here are the tests :

http://localhost:3000/api/og?contractAddress=0x50c5725949A6F0c72E6C4a641F24049A917DB0Cb&network=8453

http://localhost:3000/api/og?contractAddress=0x66496d0d93A77d6360d083f0D80AAEDbd96Dc84f&network=8453

http://localhost:3000/api/og?contractAddress=0x5AbB833336905dE17d13B9615eA3265c18aBb725&network=10 (Not an ERC 20 token contract so won't display contract name)

http://localhost:3000/api/og?contractAddress=0xda10009cbd5d07dd0cecc66161fc93d7c9000da1&network=10

http://localhost:3000/api/og?contractAddress=0x07380A04eD702B668DBb7ea38163CcE5097bbE1E&network=1

http://localhost:3000/api/og?contractAddress=0x9009d498404d32bd8C99D218D31B71b490f2521A&network=11155111


Just noticed a bug, not related to this PR but also affects this PR is how we are getting Contract Names.

The logic which we have in place for both this PR and [networks].tsx page, doesn't work for every contract since "name" function is not exposed by every contract (I initially thought whenever you deploy the contract it creates a getter name function automatically which returns ContractName)

^ But this is not the case, we felt the above logic was working fine but we were mostly testing contracts which were using ERC20 interface / other similar interfaces who exposes "name" function.

For example checkout https://abi.ninja/0x9009d498404d32bd8C99D218D31B71b490f2521A/11155111 <= this is SE-2 "YourContract" deployed on sepolia and see it doesn't able to get contract name and fallback's to just saying "Contract".

Example :

Screenshot 2024-04-25 at 2 35 56 PM

Not sure what's the best approach is to get ContractName.

@Pabl0cks
Copy link
Member

Pabl0cks commented Apr 25, 2024

Was going to suggest a silly improvement when we got no ContractName, adding a placeholder text like: Interact with, Contract Address or some variation.

Wanted to test a real environment in vercel first, but I'm running with problems with yarn vercel, have you ran throught this error before?

Vercel deployment error
[00:44:20.269] Running build in Washington, D.C., USA (East) – iad1
[00:44:20.415] Retrieving list of deployment files...
[00:44:20.915] Downloading 129 deployment files...
[00:44:20.969] Previous build caches not available
[00:44:21.681] Running "vercel build"
[00:44:22.117] Vercel CLI 34.1.3
[00:44:22.485] Installing dependencies...
[00:46:24.860] npm WARN deprecated @npmcli/move-file@1.1.2: This functionality has been moved to @npmcli/fs
[00:46:25.055] npm WARN deprecated rollup-plugin-inject@3.0.2: This package has been deprecated and is no longer maintained. Please use @rollup/plugin-inject.
[00:46:25.423] npm WARN deprecated sourcemap-codec@1.4.8: Please use @jridgewell/sourcemap-codec instead
[00:46:29.868] npm WARN deprecated vm2@3.9.19: The library contains critical security issues and should not be used for production! The maintenance of the project has been discontinued. Consider migrating your code to isolated-vm.
[00:46:30.748] npm WARN deprecated @motionone/vue@10.16.4: Motion One for Vue is deprecated. Use Oku Motion instead https://oku-ui.com/motion
[00:46:46.859] 
[00:46:46.859] added 1534 packages in 2m
[00:46:46.860] 
[00:46:46.860] 342 packages are looking for funding
[00:46:46.860]   run `npm fund` for details
[00:46:47.007] Detected Next.js version: 13.3.4
[00:46:47.011] Detected `package-lock.json` generated by npm 7+
[00:46:47.012] Running "npm run build"
[00:46:47.322] 
[00:46:47.323] > @se-2/nextjs@0.1.0 build
[00:46:47.323] > next build
[00:46:47.323] 
[00:46:47.811] Attention: Next.js now collects completely anonymous telemetry regarding usage.
[00:46:47.812] This information is used to shape Next.js' roadmap and prioritize features.
[00:46:47.812] You can learn more, including how to opt-out if you'd not like to participate in this anonymous program, by visiting the following URL:
[00:46:47.813] https://nextjs.org/telemetry
[00:46:47.813] 
[00:46:47.929] info  - Linting and checking validity of types...
[00:47:03.906] info  - Creating an optimized production build...
[00:47:26.292] <w> [webpack.cache.PackFileCacheStrategy] Serializing big strings (695kiB) impacts deserialization performance (consider using Buffer instead and decode when needed)
[00:47:26.757] Failed to compile.
[00:47:26.757] 
[00:47:26.757] node:crypto
[00:47:26.758] Module build failed: UnhandledSchemeError: Reading from "node:crypto" is not handled by plugins (Unhandled scheme).
[00:47:26.758] Webpack supports "data:" and "file:" URIs by default.
[00:47:26.758] You may need an additional plugin to handle "node:" URIs.
[00:47:26.758]     at /vercel/path0/node_modules/next/dist/compiled/webpack/bundle5.js:28:395974
[00:47:26.759]     at Hook.eval [as callAsync] (eval at create (/vercel/path0/node_modules/next/dist/compiled/webpack/bundle5.js:13:28771), <anonymous>:6:1)
[00:47:26.759]     at Hook.CALL_ASYNC_DELEGATE [as _callAsync] (/vercel/path0/node_modules/next/dist/compiled/webpack/bundle5.js:13:25925)
[00:47:26.759]     at Object.processResource (/vercel/path0/node_modules/next/dist/compiled/webpack/bundle5.js:28:395899)
[00:47:26.759]     at processResource (/vercel/path0/node_modules/next/dist/compiled/loader-runner/LoaderRunner.js:1:5308)
[00:47:26.759]     at iteratePitchingLoaders (/vercel/path0/node_modules/next/dist/compiled/loader-runner/LoaderRunner.js:1:4667)
[00:47:26.759]     at runLoaders (/vercel/path0/node_modules/next/dist/compiled/loader-runner/LoaderRunner.js:1:8590)
[00:47:26.760]     at NormalModule._doBuild (/vercel/path0/node_modules/next/dist/compiled/webpack/bundle5.js:28:395761)
[00:47:26.760]     at NormalModule.build (/vercel/path0/node_modules/next/dist/compiled/webpack/bundle5.js:28:397789)
[00:47:26.760]     at /vercel/path0/node_modules/next/dist/compiled/webpack/bundle5.js:28:81243
[00:47:26.760] 
[00:47:26.760] Import trace for requested module:
[00:47:26.760] node:crypto
[00:47:26.761] ./node_modules/@noble/hashes/esm/cryptoNode.js
[00:47:26.761] ./node_modules/@noble/hashes/esm/utils.js
[00:47:26.761] ./node_modules/@noble/hashes/esm/sha3.js
[00:47:26.761] ./node_modules/viem/_esm/utils/hash/keccak256.js
[00:47:26.761] ./node_modules/viem/_esm/index.js
[00:47:26.762] 
[00:47:26.762] 
[00:47:26.762] > Build failed because of webpack errors
[00:47:26.810] Error: Command "npm run build" exited with 1
[00:47:27.808] 
Placeholder test 1 Placeholder test 2
image image

@portdeveloper
Copy link
Member

portdeveloper commented Apr 26, 2024

@Pabl0cks I think that is an awesome idea! I am in the favor of option 2. About the error, I think it should be fixed thanks to @technophile-04 's patching!
https://abi-ninja-v2-3hd2rxxjs-buidlguidldao.vercel.app/api/og?contractAddress=0xde30da39c46104798bb5aa3fe8b9e0e1f348163f

check out this link for example

@technophile-04
Copy link
Member Author

I am in the favor of option 2.

Yup option 2. looks good, I think we should update it, Thanks @Pabl0cks 🙌

Wanted to test a real environment in vercel first, but I'm running with problems with yarn vercel,

Yup yarn vercel (deploying through CLI would fail) because when deploying through CLI we are only uploading nextjs and for patch to work properly we need to upload the whole monorepo to vercel (which is kinda not possible through vercel CLI)

checkout out scaffold-eth/scaffold-eth-2#706 (comment)

For testing you could hit (our latest deployment) : https://abi-ninja-v2-3hd2rxxjs-buidlguidldao.vercel.app/api/og?contractAddress=0xde30da39c46104798bb5aa3fe8b9e0e1f348163f (we should hit /api/og? ) for now for testing,

hitting https://abi-ninja-v2-3hd2rxxjs-buidlguidldao.vercel.app/0xde30da39c46104798bb5aa3fe8b9e0e1f348163f/1 won't work for now since we have hardcoded https://abi.ninja in MetaHeader and it doesn't have this changes yet so it won't resolve the OG image, but once we merge this PR it should be working nicely

@carletex
Copy link
Collaborator

This is looking great!! Thanks all.

Could we add the chain logo next to the chain name? It should be straightforward since we have them for the homepage selector.

@Pabl0cks
Copy link
Member

Pabl0cks commented Apr 29, 2024

Could we add the chain logo next to the chain name? It should be straightforward since we have them for the homepage selector.

Added it, changed background to look better with current chain icon image assets.
Also made the icons a bit bigger than I wanted because they were looking pixelated otherwise (tried changing the size either using h- or passing a width and height params, but I was getting the same pixelated result.

We probably should change the image assets to SVGs with no background to fix both problems in a future PR (or current if it looks too bad for you!).

Here are a few examples with the same contract and manually changing network:

@portdeveloper
Copy link
Member

portdeveloper commented Apr 29, 2024

That looks amazing! @Pabl0cks
Thanks 🤝 🫡

Updated the svg images for chains: arbitrum, base, scroll, zksync:
Used some random CA just to show off the logos.

og
og
og
og

I think this change is minor so we can include it here.
Let me know if it's OK or not, I will add the commit here but feel free to roll it back.

@technophile-04
Copy link
Member Author

This is looking great !!!! Tysm @portdeveloper and @Pabl0cks , Merging this 🙌 !!

@technophile-04 technophile-04 merged commit 41a975d into main Apr 30, 2024
3 checks passed
@technophile-04 technophile-04 mentioned this pull request Apr 30, 2024
@portdeveloper portdeveloper deleted the local-test branch June 18, 2024 16:56
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants