Skip to content

Conversation

@katspaugh
Copy link
Contributor

@katspaugh katspaugh commented May 26, 2022

Description

Resolves #1020 and #948.

As outlined in #1020, it's sometimes necessary to pass a custom (typically public) RPC when adding a new chain to the user's wallet.

Checklist

  • The version field in package.json is incremented following semantic versioning
  • The box that allows repo maintainers to update this PR is checked
  • I tested locally to make sure this feature/fix works
  • This PR passes the Circle CI checks

@katspaugh
Copy link
Contributor Author

@aaronbarnardsound @Adamj1232 @taylorjdawson gm! Any feedback for this?

@lnbc1QWFyb24
Copy link
Contributor

Thanks for this PR @katspaugh. Another approach that I think might be more clear naming wise, is to add an optional publicRpcUrl parameter to the Chain object and use that in the add chain method. That way the Chain info can be set in one place and then there's no need to think about it when setting the chain. I also think it will help distinguish between the two rpcUrl parameters. What do you think?

@lnbc1QWFyb24
Copy link
Contributor

Just adding a note @taylorjdawson while we are here we should also add an option blockExplorerUrl parameter as requested in #948

@katspaugh
Copy link
Contributor Author

Makes sense!

I'll update the PR to extend the Chain object then 👍

@katspaugh katspaugh changed the title Fix: pass optional RPC when switching/adding chains Feat: optional public RPC and block explorer Chain params Jun 12, 2022
@taylorjdawson
Copy link
Contributor

Hey @katspaugh! I see some commits here, is this ready for re-review?

@katspaugh
Copy link
Contributor Author

I haven't fully tested this version yet but a review would be appreciated, yes!

@taylorjdawson
Copy link
Contributor

@katspaugh This looks good! Have you had a chance to test?

@katspaugh
Copy link
Contributor Author

Still wrapping my head around testing monorepos, I'll get back to you on the weekend!

@taylorjdawson
Copy link
Contributor

@katspaugh I can also test if you want, was there anything else you wanted to add to the PR?

@katspaugh
Copy link
Contributor Author

Oh, that'd be awesome. Let me just fix those git conflicts in the package.json's.

@mahmud-bn mahmud-bn changed the title Feat: optional public RPC and block explorer Chain params Feature: optional public RPC and block explorer Chain params Jun 16, 2022
@mahmud-bn mahmud-bn merged commit c0cc508 into thirdweb-dev:v2-web3-onboard-develop Jun 16, 2022
@mahmud-bn
Copy link
Contributor

Hi @katspaugh
Thanks for creating the PR. This has been merged

@katspaugh
Copy link
Contributor Author

Amazing, thank you guys!

@Adamj1232 Adamj1232 mentioned this pull request Jun 21, 2022
4 tasks
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.

[Bug]: addNewChain always sets the app's provider RPC URL

4 participants