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

Add custom chains #129

Merged
merged 33 commits into from
Jul 15, 2024
Merged

Add custom chains #129

merged 33 commits into from
Jul 15, 2024

Conversation

portdeveloper
Copy link
Member

@portdeveloper portdeveloper commented Jul 3, 2024

Description

This PR:

  • Adds "Add Custom Chain" button to the NetworksDropdown
  • Implements a basic form as modal to submit the custom chain
  • Lifts the wagmi client to zustand store
  • Adds custom chains to localStorage when the user adds them
  • Saves the custom abi to localStorage when the user submits a valid JSON string
  • Divides NetworksDropdown into separate pieces(component, utils file)

Known issues:

  • If the rpc for a custom chain is wrong and the user tries to load a contract on that chain the app acts weird
  • [ ]

_Closes #97 #128

Copy link

vercel bot commented Jul 3, 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 Jul 15, 2024 3:54am

@portdeveloper portdeveloper marked this pull request as ready for review July 4, 2024 16:32
@Pabl0cks
Copy link
Member

Pabl0cks commented Jul 7, 2024

Nitpick: If you add a custom chain with the same chainId of a viem chain, we showing the "delete" icon also to the viem chain.
It deletes it visually, but on reload we show the viem chain again, so no big deal.

Maybe the use case doesn't even make sense, ignore if you feel like it's no worth to fix.

Screenshot

image

@Pabl0cks
Copy link
Member

Pabl0cks commented Jul 7, 2024

I'm having problems with Heimdall in a Sepolia contract that is working fine in prod and [main] branch on local.

Heimdall is working fine with other contracts (maybe it is just working in the contracts I had ABI stored in local storage?).

  • Contract: 0xFB30C0790128b97e3aC540E6124e512E37c47D00
  • Network: Sepolia
Screenshoot

image

Also I feel maybe we should tweak the UI from this on big screens :)

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.

GJ getting all of this together Port! ❤

I'm still testing the features, but I'm feeling maybe we should have separated this in 2 PRs, one for custom chains and another for the local storage ABI (which is a great feature to have btw!!)

Maybe it was needed to do them together this time, but if it wasn't, for future PRs I think it would be better/easier to split in smaller pieces 🙌

@technophile-04
Copy link
Member

Thanks @portdeveloper!! I was just doing basic testing and when you make transaction the UI keeps showing loading notification and never updates / refetch the Contract Readonly variables, although the transaction receipt is shown :

Example image :

Notice that it, it showed transaction receipt dropdown but still the Transaction notification is in loading state :

image

To test try doing the transaction here http://localhost:300/0xE009aea21af005e6B531B5f4a8f909C64A0c596d/11155111?methods=setGreeting

@portdeveloper
Copy link
Member Author

I'm having problems with Heimdall in a Sepolia contract that is working fine in prod and [main] branch on local.

The usual route of using the home page and then clicking decompile with heimdall works when I try it:
https://github.com/BuidlGuidl/abi.ninja/assets/108868128/15c79f5b-fb5e-4b3e-ae46-b3800b765830

Maybe the commits I did above fixed it

But your question made me realize that we should probably handle the following case as well.

https://abi.ninja/0xFB30C0790128b97e3aC540E6124e512E37c47D00/11155111

when the user goes to this link s/he should have the change to decompile the contract.

@technophile-04
Copy link
Member

Thanks @portdeveloper, Looks great!! Let's wait for Pablo to test it nicely 🙌


Regarding :

1 2
Screenshot 2024-07-09 at 12 40 51 PM Screenshot 2024-07-09 at 12 39 18 PM

I think we could do better on UI / UX in [contractAddress]/[networks] page. Maybe we could start talking with Andrea on design and create a PR as soon this is merged.

The UX flow (I think) can be improved in img 1 is similar to homepage we should give the user to decompile with Heimdall or input custom ABI

For img 2 (also can be implemented in homepage add chain modal) :

As soon as the user types the chainId in first field we could try to fetch details from ethereum-lists/chains (its an well maintained repositry also used by many popular website like https://chainlist.org etc) will also mostly solve known issue mentioned in #129 (comment), We could tweak the UX flow but I see ethereum-lists/chains improving it in some way or other.

@Pabl0cks
Copy link
Member

Pabl0cks commented Jul 11, 2024

TYSM for the tweaks @portdeveloper! I think it's working nicely now and then we can focus on localstorage ABI.

I found some issue with ENS resolving:

  • Contract address field in homepage is not resolving 0x0eD8fa0aE29b6dF59f755F285667DFf5DF97bd6b ENS (with main branch its showing nicely grants.buidlguidl.eth)
  • Same for Contract field in "Contract overview" for this same contract: http://localhost:3000/0x0eD8fa0aE29b6dF59f755F285667DFf5DF97bd6b/1
  • In "contractData" it should also show "ensContract" field as reverse.ens.eth

Edit: It's also not working on input fields, for example if you try to set an ens in this address field:
http://localhost:3000/0x0eD8fa0aE29b6dF59f755F285667DFf5DF97bd6b/1?methods=addAdmin

I think after this we should be good to merge, and then we can create a separate issue to tackle #129 (comment) and whatever other small issue that may be left.

@technophile-04
Copy link
Member

TYSM @portdeveloper !! and @Pabl0cks for testing!! Also tested and everything seems to work 🙌

Just pushed a couple of small commits. Regarding 1fbd6a3, I think maybe in other PR we can also ask for blockexplorer(optional) link in "Add chains" input, so that Address component can href to it.

Merging this 🚀🚀

@technophile-04 technophile-04 merged commit 2bc9c56 into main Jul 15, 2024
3 checks passed
@portdeveloper portdeveloper deleted the feat/add-custom-chains branch July 17, 2024 08:23
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.

3 participants