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

Decompile ABI using a heimdall-rs backend #71

Merged
merged 27 commits into from
Apr 21, 2024
Merged

Conversation

portdeveloper
Copy link
Member

@portdeveloper portdeveloper commented Mar 10, 2024

Description

This PR:

  • Calls a simple heimdall-rs backend to get the abi of a contract
  • Implements steps instead of tabs in the ui
  • Enriches notifications to provide better UX.
chrome_HtHKY3NP0p.mp4

ToDo - Improvements

  • go back button looks bad on smaller viewports, also its placement can be better
  • code cleanup

Additional Information

Related Issues

_Closes #63

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:

Copy link

vercel bot commented Mar 10, 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 19, 2024 4:45pm

@portdeveloper portdeveloper mentioned this pull request Mar 12, 2024
2 tasks
@portdeveloper
Copy link
Member Author

portdeveloper commented Mar 12, 2024

linkt to the commit
The way I made the height of the component here is tricky... I basically used the same div with an empty char instead of the go back button.

chrome_f7qVMPHw5d.mp4

I am aware this is a bad practice, and will try to fix it asap!

Also, lmk if you know any easier/cleaner way of doing this, I feel like I have one in my head somewhere but can't recall it...

@Pabl0cks
Copy link
Member

Good job @portdeveloper incorporating heimdall-rs into abi.ninja!! 👾

Last week in London we reviewed the new workflow a bit, and I think we could get some designer help to simplify the UI to make it a bit smoother. Asked Andrea for help and she is going to play a bit in figma to propose a way to simplify it.

As soon as we get the design I'll send it to review 🙌

@Pabl0cks
Copy link
Member

Andrea sent us the new UI/UX proposal, let me know what do you think @portdeveloper ! 🙌

The workflow would be something like this:

  • User access abi.ninja, we delete the tabs and only leave the contract address input (+ quick access)
  • User enters an address and clicks "Load Contract"
    • If contract is verified on the selected network => ContractDetailPage loads with the ABI data
    • If contract is not verified, we let the user choose between Decompyle (beta) and Manually import

We have those design proposals for the "contract not verified" window.

abi-ninja-Decompile-ManuallyImport

I personally like option 1 and 3, probably 1 is the simplest. I think we can leave network dropdown styles as they are right now for simplicity, don't need to follow the design 100%.

@portdeveloper
Copy link
Member Author

portdeveloper commented Mar 28, 2024

wBQO95Y5d8.mp4

Updated the UI according to Andrea's design, but merging the current styles with hers.
Honestly, the new UI looks much better, I am really excited for it!

Copy link
Member

@Pabl0cks Pabl0cks left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nice job @portdeveloper !! With the new UI this PR is looking awesome. Did a first round of testing, these are my notes:

  • Check alignment of the header section from the new screen (Heimdall & Import ABI), specially on mobile. "Go back", small logo and network dropdown should be aligned.
  • Network dropdown in the new screen should be the same you selected in the homepage search.
  • I think we should disable network dropdown in the new screen, does not make sense to change it there. I see it just informative of what you just searched in the previous screen.
  • Remove notification error message when not verified, we already giving all the feedback with the new screen.
  • When we access a contract through a URL (/contractAddress/chainId), and the contract is not verified, should we load the new screen instead of showing a not verified error? => If we see this interesting, can create a new issue to tackle it in a future PR, I wouldn't tackle now.

Tomorrow I'll try to review more in depth 🙌.

My test contract in case it's useful for anyone: sepoliaUnverifiedContract.txt

@portdeveloper
Copy link
Member Author

@technophile-04
image
Yes, you are right, heimdall-rs currently cannot decompile that contract and returns with an error, but my backend does not handle it gracefully.
For now I am adding a check for abi array length which returns if the array length is 0.
Tomorrow I will fix the backend to handle this kind of errors better.
Thanks for pointing this out!

@technophile-04
Copy link
Member

technophile-04 commented Apr 12, 2024

should we put Heimdall backend in a BG account? Maybe we can deploy a new instance from Port's gulltoppr or just adding admin@bg to the existing one. For the sake of paying (if needed), security and not having a single person behind it (as we do with all the BG servers)

I think we are just only waiting on this right ? Because it looks really great for first iteration !

Also @portdeveloper could you please resolve conflicts in packages/nextjs/page/index.tsx

@portdeveloper
Copy link
Member Author

Thanks for reminding me the conflicts!

Here is the repo for the backend, I use fly.io for deployment so a simple flyctl launch is enough for deploying the backend.

@carletex
Copy link
Collaborator

Here is the repo for the backend, I use fly.io for deployment so a simple flyctl launch is enough for deploying the backend.

🫡 Thanks Port!

I'll do this during the week. I'll hit you up If I have any issues.


There is still a conflict in index.tsx

@portdeveloper
Copy link
Member Author

Sorry about the conflicts 😢 , fixed!

Copy link
Collaborator

@carletex carletex left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Just deployed the BG instance of gulltoppr to fly.dev!

https://heimdall-abi-ninja.fly.dev/1/0x6B175474E89094C44Da98b954EedeAC495271d0F

Could you check that everything is working as expected?

Also left a review comment.

packages/nextjs/pages/index.tsx Outdated Show resolved Hide resolved
@carletex
Copy link
Collaborator

Just added the envar to Vercel.

When you guys feel that is ready.... feel free to merge!!

@technophile-04
Copy link
Member

Tested it nicely and working great !!!!

Unverfied contract on baseSepolia : 0xFB30C0790128b97e3aC540E6124e512E37c47D00 (Heimdall Working)
Unverfied contract on base mainnet : 0x3A6AE1e6aD00518bDcd485a6B4d428fb9c9a3120 (Heimdall not working)
Unverfied contract on base mainnet : 0xD7c77458e33502f2E5eC04b481463aeCdDd4acd0 (Heimdall working)

Obviously, I think it won't be able to decode each and every contract but its 60% of time able to do so on my random testing

Will, merge this probably tomorrow 🙌

@Pabl0cks
Copy link
Member

Been testing it, looks good to me!!

  • Is working normally on verified contracts
  • Unverified with ABI is still working (can test my sepolia contract 0x60b99996e28c40d5e78ea217e897b93a6fabcdae with the ABI on the txt file from my previous comments)
  • Unverified with decompile is working quite well, checked with my contract and some random contracts:
    • My sepolia contract 0x60b99996e28c40d5e78ea217e897b93a6fabcdae is still working nicely
    • Unverified on Base (working): 0x9ba6d87ce0c721e9e7b8d8cac12a5334c8898f59
    • Unverified on Mainnet (+ proxy) (working): 0x758ed0650bdf2ac3bf4a48440c3eb2f6d2bb42a5
    • Unverified on OP mainnet (not working): 0x2e42f214467f647fe687fd9a2bf3baddfa737465
    • Unverified on OP mainnet (working): 0x38edf0a0d32b5c8e23284da87d078062a3f772a7
    • Unverified on OP mainnet (not working): 0x1c8317517070c197ccaaa047662468f1b4055baa

@technophile-04 technophile-04 merged commit 1ebabdc into main Apr 21, 2024
3 checks passed
@portdeveloper portdeveloper deleted the ui/heimdallrs-steps branch April 25, 2024 05:35
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