-
Notifications
You must be signed in to change notification settings - Fork 72
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
Enable proxy contracts #72
Enable proxy contracts #72
Conversation
@portdeveloper is attempting to deploy a commit to the BuidlGuidl Team on Vercel. A member of the Team first needs to authorize it. |
The latest updates on your projects. Learn more about Vercel for Git ↗︎
|
@portdeveloper This looks great! I authorized the deployment and tested it, and it works great! :-) |
This is GREAT Port nice, Tysm !!! Looking at its working :
I am not sure if this was intended(because of simplicity for current PR) or if we missed but maybe I think it kind of makes more sense to direct user As I understand how proxies work is the storage lives in If we direct users to I think we might want to do something like this :
This is what I have been doing in here checkout YT link on build. And on UI we can show something similar to : Something similar is done is on etherscan where they have "Read as proxy" btn. Really sorry If I missed something please feel free to correct me !! Because the things which I said above are in context which I have from UUPS proxy but I think in-general all proxies works same but I might be completely wrong so please correct me !!! |
@technophile-04 |
Just got a basic working demo, some code duplication exist and the styling could be better |
Nice !! Thanks !! Just noticed a bug, that when you go back and select other contract (non proxy contract) it doesn't reset the Implementation address state : Demo Video :Screen.Recording.2024-03-25.at.5.38.10.PM.movAlso just noticed that for NounsDAO contract (EIP897-delegate proxy) I see we have added We could use that library directly since we have import { AlchemyProvider } from "@ethersproject/providers";
import detectProxyTarget from "evm-proxy-detection";
// ...
const alchemyProvider = new AlchemyProvider(parseInt(network), scaffoldConfig.alchemyApiKey);
const requestFunc = ({ method, params }: { method: string; params: any }) =>
alchemyProvider.send(method, params);
const implementationAddress = await detectProxyTarget(verifiedContractAddress, requestFunc); I just tried it out and it nicely resolves NounsDAO contract (EIP897-delegate proxy) too, so lets use the library directly 🙌 Btw @portdeveloper , any reason for keeping this after #71 ? I think maybe we could get this in first because I think the changes are minimal once we use the |
@technophile-04 Thanks! I am sorry that I sometimes miss the most basic things like a state reset. Need to get better at this honestly. I thought you would want me to use viem so that's why I tried to convert it all to use viem instead of ethers! But yes, let's use evm-proxy-detection ! Let me change the code accordingly and we can merge this first I think. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is great @portdeveloper !! Everything is working nicely now, TSYM !!
That "Implemntation Address" text stands out too much not sure what would be best maybe keep it same as contract name ? Not completely sure
Please don't change anything, lets hear from @carletex and @Pabl0cks if they have any suggestion for UI 🙌
Also not sure if its a noticeable difference but when you click "Gitcoin" on home screen (or any other contract which is not proxy) it takes a bit more time because irrespective of if it's a proxy or not we are assuming it's proxy and making 4-5 eth_getStroageAt
rpc calls.
One way might be we ask the user when he input's the address to weather he need's to read it as proxy or not but lol it decreases the UX a lot , but yeah let's leave this issue for different PR / issue 🙌
This PR looks awesome!! Testing it this morning 🙌 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Awesome job Port!! 🔥 Adding proxy support to abi.ninja is so cool!
After some testing here is a couple of notes:
- I miss some feedback to the user to know what's getting loaded / got loaded. We only got the "Implementation address" right now but user may be confused.
- For this proxy contract (0xF74B146ce44CC162b601deC3BE331784DB111DC1) the implementation address that gets loaded sometimes changes (when first load or when refresh the web, seems random).
- Implementation address 1: 0x932a29dbfc1b8c3bdfc763ef53f113486a5b5e7d (good)
- Implementation address 2: 0x8237f421357f87a23ed0cff3a5586172f210a21b (bad)
@technophile-04 @Pabl0cks About the problem you noticed, Pablo, that would require that we open an issue on the lib that we use: "evm-proxy-detection". But I am not exactly sure if it is maintained currently. Maybe someone needs to take it and rewrite it, better... :D |
Alright, experimented a little with the To avoid that, I copied the code from evm-proxy-detection to a new file in utils folder and changed this part of the code:
` This way we are checking the type of the proxy before we do a call to the impl method, and avoid the misinterpretation. |
Wouldn't import { createClient, http } from 'viem'
import detectProxyTarget from 'evm-proxy-detection'
const client = createClient({
transport: http('https://cloudflare-eth.com'),
})
const target = await detectProxyTarget(
'0xA7AeFeaD2F25972D80516628417ac46b3F2604Af',
client.request
) |
With the last changes to fix ZoraNFTCreatorProxy contract detection (which is working good to me now!) other contracts detection got broken => NounsDAO contract (EIP897-delegate proxy) is not working properly now. Here is the console log
I'm a bit afraid of rewriting Maybe I'm too conservative 😅 |
Yeah actually our first intuition was to pass 'string' is not assignable to type '"wallet_watchAsset"'I think this is because the callback request function from evm-detect-proxy passes string for method name instead of literal method name, but maybe I might have missing something Since we already had
Yup agree with this, maybe @portdeveloper for simplicity of this PR lets use We can create an issue on Thanks, @portdeveloper and also @Pabl0cks for testing 🙌 !! |
Thanks for the feedback! Strangely, I did not get any notifications for your comments... Will be reverting the changes ASAP to use evm-proxy-detection for now! |
This reverts commit 5ce5f12.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks @portdeveloper ! Merging this 🚀
Description
This PR adds the ability to work with proxy contracts to abi.ninja.
Fixes #8 .
Let's merge this after #70 and #71 . I am opening this to be able to iterate on it together, since the logic is mostly in proxyContracts.ts.
Untitled.mov
ToDo - Improvements
Additional Information
Related Issues
Closes #{issue number}
Note: If your changes are small and straightforward, you may skip the creation of an issue beforehand and remove this section. However, for medium-to-large changes, it is recommended to have an open issue for discussion and approval prior to submitting a pull request.
Your ENS/address: