-
Notifications
You must be signed in to change notification settings - Fork 921
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
Show inherited functions in Debug (when deploying with hardhat
)
#564
Show inherited functions in Debug (when deploying with hardhat
)
#564
Conversation
I still need to add this to the But I started the PR so I can get some feedback or tips on the UI. |
The first two commits are implemented with function-origin2.mp4 |
Also, I don't think this will work when overriding functions. |
hardhat
)
I made some modifications now. This is how it looks: function-origin3.mp4 |
I don't know if I should split Also, the tooltip looks the same for all three (vars, readFunc and writeFunc) should I make that to a seperate component or keep them where they are now? |
packages/nextjs/components/scaffold-eth/Contract/DisplayVariable.tsx
Outdated
Show resolved
Hide resolved
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 looking great @FilipHarald !!
Left some minor comments. Also tagging Shiv & Rinat so the can test and review.
Thanks!
packages/nextjs/components/scaffold-eth/Contract/InheritanceTip.tsx
Outdated
Show resolved
Hide resolved
packages/nextjs/components/scaffold-eth/Contract/InheritanceTip.tsx
Outdated
Show resolved
Hide resolved
packages/nextjs/components/scaffold-eth/Contract/InheritanceTip.tsx
Outdated
Show resolved
Hide resolved
packages/nextjs/components/scaffold-eth/Contract/InheritanceTip.tsx
Outdated
Show resolved
Hide resolved
Thank you for pr, @FilipHarald ! Noticed that color of tooltip in light mode the same as bg of methods list. And I'm not sure that we want to sort methods: own in alphabetical order -> inherited it alphabetical order. It's good but I think often inherited methods are main. You don't need to change it right now, I wrote it just to discuss Other than that looks good, gj! |
Thanks for the review @rin-st !
Oh, you're right. I can see if I there is an easy fix.
OK, I would be happy to discuss this. I can tell you my reasoning and I'd be happy to hear yours! So for me this is a problem because I'm still new to Solidity. So when I extend some interface, like |
In some cases you need to use inherited methods often. But I think sorted is ok, at least it's easy to find what you need. One more thing. Noticed that when you override inherited methods, it's still marked as inherited. Is it possible to mark that methods "overridden" or "inherited, overridden"? And if possible sort: own -> overridden -> other inherited |
packages/nextjs/components/scaffold-eth/Contract/ContractVariables.tsx
Outdated
Show resolved
Hide resolved
Also find a kind of edge case which is kidof related to above comments, when you just import a contract but don't inherit it it still gives inherited : Example Contract :
Also one thing I think it would be really great if we make Because there might a slight case where we couldn't get this data in also if we people want to add external contracts which they already have deployed it might be hard from them to get this Tysm @FilipHarald great job!! |
Replying to both of these:
The problem is that I have not yet found any linking of what is an inherited function and not. The only thing I'm doing is looking at the imported contracts, and then cross-reference the functions in their contracts with the ones in the current one. But the current contract will not show if the function in the ABI if it is overridden or not. Maybe it's possible to get from when compiling the Solidity, but I have not yet found a solution to this. |
Currently, the only "blocker" is the edge-cases that I summarized above. (#564 (comment)) |
I think this looks almost ready for merge 🙌
Yup to summarize, we get unwanted
Even I tried debugging it but no luck finding a solution with all the available / generated / compiled files :( Nevertheless, I think this is ready to merge since 1st edge case feels not that deal breaker and 2nd one is very less likely to happen But let's see what others have to say 🙌, Tysm @FilipHarald for proposing and tackling it !! |
Looks much better! I found one more edge case, sorry 😄 . It's very rare and I don't know if someone will ever meet it. See solidity-by-example . I copied contracts and everything works as expected, except contract E - it should show "inherited from contracts/B.sol" or "inherited from contracts/C.sol, contracts/B.sol" I'm not sure if we need to consider this, but if it's fixable I think we need to do it. |
Tysm @rin-st great find 🙌 Created FilipHarald#1 which solves : |
get contract sources from contract definition
I think everything is merged now. Thanks again @rin-st and @technophile-04 for help with edge-cases. |
…at/contract_function_origins
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.
lgtm!
packages/nextjs/components/scaffold-eth/Contract/ContractVariables.tsx
Outdated
Show resolved
Hide resolved
Tysm @FilipHarald 🙌, I think this is really nice !! Next steps :
Thanks again !! |
Description
This adds information in the Debug-view. It's now possible to see which functions are inherited, and from where they are inherited. Unfortunately, functions that use
override
show up with inherited tooltip.Additional Information
Related Issues
Related discussion: #558
Your ENS/address: harmont.eth