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

feat: make chain list extendable #59

Merged
merged 1 commit into from
Aug 23, 2022

Conversation

re2005
Copy link
Collaborator

@re2005 re2005 commented Jun 18, 2022

The available network list is essential on every application, but they are so many, so better reduce it to a few and let the user extend it as it needs.

Please share ideas here.

@netlify
Copy link

netlify bot commented Jun 18, 2022

Deploy Preview for vue-dapp ready!

Name Link
🔨 Latest commit d9e47c9
🔍 Latest deploy log https://app.netlify.com/sites/vue-dapp/deploys/63048f887f875d00082c51a6
😎 Deploy Preview https://deploy-preview-59--vue-dapp.netlify.app
📱 Preview on mobile
Toggle QR Code...

QR Code

Use your smartphone camera to open QR code link.

To edit notification comments on pull requests, go to your Netlify site settings.

@netlify
Copy link

netlify bot commented Jun 18, 2022

Deploy Preview for vue-dapp-docs ready!

Name Link
🔨 Latest commit d9e47c9
🔍 Latest deploy log https://app.netlify.com/sites/vue-dapp-docs/deploys/63048f887f875d00082c51ab
😎 Deploy Preview https://deploy-preview-59--vue-dapp-docs.netlify.app
📱 Preview on mobile
Toggle QR Code...

QR Code

Use your smartphone camera to open QR code link.

To edit notification comments on pull requests, go to your Netlify site settings.

@johnson86tw
Copy link
Member

It looks good, seems that you just remain mainnet and goerli as the default networks, so other networks should be added by users, right?

@re2005
Copy link
Collaborator Author

re2005 commented Jun 21, 2022

It's just a suggestion, maybe this plugin shouldn't take the responsibility of managing the networks, also because there are too many and on each project they will be different.

But also the information, per chain that devs needs will be different for their needs.

So you can let users to just input their networks/chains and don't deal with that.

@johnson86tw
Copy link
Member

johnson86tw commented Jun 22, 2022

Make sense, I like this extendable feature, but I prefer to remain Hardhat, Arbitrum, Rinkarby, and Optimism

@johnson86tw
Copy link
Member

@re2005 Hey, I changed my mind. Just remaining mainnet and goerli is good. Can you help me write down some documentation to tell users how to add a custom network?

@tempe-techie
Copy link

Yes, this makes sense. I can help with the docs if @re2005 can't. The best would be to have a .js or .json template file that has some of the most common custom networks already added (Polygon, Optimism, Arbitrum, BSC), so that devs can see how to add new networks.

It makes sense to have Ethereum and its testnets (Goerli) added by default in the plugin (because they use wallet_switchEthereumChain to switch). And the custom ones (added by devs) would then use wallet_addEthereumChain. Which makes the implementation simpler.

@re2005
Copy link
Collaborator Author

re2005 commented Jul 21, 2022

Something other lib do provide a list of most common chains. So you can import everything or just the ones you need.

Check:
https://github.com/wagmi-dev/wagmi/blob/main/packages/core/src/constants/chains.ts

it's all in there.

@re2005 re2005 force-pushed the feature/init-parameters-for-chains branch from c9196f3 to 685b655 Compare August 21, 2022 11:26
@re2005
Copy link
Collaborator Author

re2005 commented Aug 21, 2022

Hey @chnejohnson So finally i'm running on a valid/good use case.

My application supports some networks that are not "inside" vue-dapp so when I try to connect to some of those, and the network is not yet on Metamask than vue-dapp looks to it's internal list of networks in order to add it, but because the new network it is not there it will fail with an error. The user at this point can't add a new network to metamask using vue-dapp

Check:
https://github.com/chnejohnson/vue-dapp/blob/main/src/wallets/metaMask.ts#L196

This Pr will also fix that allowing the extension of the network list.

I've added some documentation as well. (@tempe-techie please help out if you think documentation needs more details)

Please review and let's merge it.

@re2005 re2005 force-pushed the feature/init-parameters-for-chains branch from dab4cf1 to ecf088a Compare August 21, 2022 12:05
@tempe-techie
Copy link

I've added some documentation as well. (@tempe-techie please help out if you think documentation needs more details)

Please review and let's merge it.

Looks good! Thanks! 🎉 🤘 😎

Copy link
Member

@johnson86tw johnson86tw left a comment

Choose a reason for hiding this comment

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

Hey @re2005, thanks for your good work!

I reviewed the code and found a few bugs (probably made by my hand), but it's fine to fix it later. The main problem is that you have to drop the last commit:d74597( Merge branch main...) and run git rebase -i main on commit:ecf088 and fix a simple conflict. So it will be more clear in the git history.

In addition to what's mentioned above, it looks good to me 👍

src/constants/chainId.ts Outdated Show resolved Hide resolved
src/constants/chainId.ts Outdated Show resolved Hide resolved
src/constants/chainId.ts Outdated Show resolved Hide resolved
@re2005 re2005 force-pushed the feature/init-parameters-for-chains branch 2 times, most recently from 121a839 to ccacec6 Compare August 22, 2022 17:25
@re2005
Copy link
Collaborator Author

re2005 commented Aug 22, 2022

@chnejohnson Thanks for the review 🙏

I hope It's all fixed now.

@re2005 re2005 force-pushed the feature/init-parameters-for-chains branch from ccacec6 to 34e3802 Compare August 22, 2022 18:30
Update Board.vue

Update useEthers.ts

Update chainId.ts

Update App.vue

chore: extendable networks option

chore: update docs

Update index.md

Update README.md

fix: url and symbol

Update README.md

Update index.md
@re2005 re2005 force-pushed the feature/init-parameters-for-chains branch from 34e3802 to d9e47c9 Compare August 23, 2022 08:27
@johnson86tw johnson86tw merged commit 86dc7cc into vu3th:main Aug 23, 2022
@johnson86tw
Copy link
Member

@re2005 Perfect!

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.

None yet

3 participants