Skip to content

[REACT] Fix/unable to override chain #708

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

Merged

Conversation

ThianHooi
Copy link
Contributor

Issue

  • When trying to override certain value (example: RPC URL) when passing props to the activeChain in ThirdwebProvider component, I found that the RPC calls are not done through the custom RPC URL provided, instead default thirdweb RPC URL is used
  • Found out that the issue happens here. When merging supportedChains and activeChain, activeChain with overridden value should replace the respective chain config in supportedChain. This causes the RPC URLs mapping passed into Wagmi uses the default RPC URL.

What's this PR do?

  • feat: added uniqueBy util function that remove duplicated element in an array
  • feat: merge supportedChain and activeChain by removing duplicated chain (comparing chainId)

@ThianHooi ThianHooi requested a review from a team as a code owner March 10, 2023 14:26
@ThianHooi ThianHooi requested a review from a team March 10, 2023 14:26
@changeset-bot
Copy link

changeset-bot bot commented Mar 10, 2023

🦋 Changeset detected

Latest commit: 1459f85

The changes in this PR will be included in the next version bump.

This PR includes changesets to release 6 packages
Name Type
@thirdweb-dev/react Patch
@thirdweb-dev/sdk Patch
@thirdweb-dev/react-core Patch
thirdweb Patch
@thirdweb-dev/react-native Patch
@thirdweb-dev/unity-js-bridge Patch

Not sure what this means? Click here to learn what changesets are.

Click here if you're a maintainer who wants to add another changeset to this PR

return [...supportedChains, activeChain] as Readonly<Chain[]>;
return uniqueBy([activeChain, ...supportedChains], "chainId") as Readonly<
Chain[]
>;
Copy link
Contributor

Choose a reason for hiding this comment

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

Thanks or contributing to our repo! This is indeed an issue. What do you think if we do the following to filter out the activeChain from the supportedChains array? This way we don't need to add the uniqueBy function and can use a tested array function.

return [
      ...supportedChains.filter((c) => c.chainId !== activeChain.chainId),
      activeChain,
    ] as Readonly<Chain[]>;

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@iketw Sure, makes sense to use simpler implementation. Will make the change

@ThianHooi ThianHooi requested a review from iketw March 10, 2023 15:35
@codecov
Copy link

codecov bot commented Mar 10, 2023

Codecov Report

Patch and project coverage have no change.

Comparison is base (2ec2802) 69.76% compared to head (1459f85) 69.76%.

Additional details and impacted files
@@           Coverage Diff           @@
##             main     #708   +/-   ##
=======================================
  Coverage   69.76%   69.76%           
=======================================
  Files         219      219           
  Lines        9559     9559           
  Branches     1154     1154           
=======================================
  Hits         6669     6669           
  Misses       2291     2291           
  Partials      599      599           

Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here.

☔ View full report in Codecov by Sentry.
📢 Do you have feedback about the report comment? Let us know in this issue.

@iketw iketw added this pull request to the merge queue Mar 10, 2023
Merged via the queue into thirdweb-dev:main with commit aad57a5 Mar 10, 2023
@github-actions github-actions bot mentioned this pull request Mar 10, 2023
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.

2 participants