-
Notifications
You must be signed in to change notification settings - Fork 85
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
chore(networkConfig): remove 1 place where manual testing and code searching would be needed to add chains #4857
Conversation
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## main #4857 +/- ##
=======================================
Coverage 85.36% 85.36%
=======================================
Files 716 716
Lines 29229 29232 +3
Branches 5086 5086
=======================================
+ Hits 24951 24954 +3
Misses 4039 4039
Partials 239 239
Continue to review full report in Codecov by Sentry.
|
'eip155:421614': Network.Arbitrum, | ||
'eip155:10': Network.Optimism, | ||
'eip155:11155420': Network.Optimism, | ||
export const walletConnectChainIdToNetwork: Record<string, Network> = {} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I spent a sec trying to code golf this into a single line using a Object.entries(walletConnectChainIdToNetworkId).reduce(...)
, but then realized that it's prolly less readable than the current approach 😆
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think you could do Object.fromEntries(Object.entries(walletConnectChainIdToNetworkId).map(([chainId, networkId]) => [networkId, chainId])))
. But i went with the for loop because it seemed more readable
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
i may have gotten network id and chain id backwards in my comment^ but whatever
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
TY for the catch, looks good. recently Jean was floating the idea of moving away from lodash, we didn't end up discussing at eng sync like we intended but seeing it used here made me remember this.
This comment was marked as outdated.
This comment was marked as outdated.
This comment was marked as duplicate.
This comment was marked as duplicate.
This comment was marked as duplicate.
This comment was marked as duplicate.
…arching would be needed to add chains (valora-inc#4857) ### Description we should try to maintain a status quo where all we have to do in every repo to add a chain is update a "NetworkId" enum and fix all compilation errors that result from it ### Test plan added spot checks for the two affected configs ### Related issues cc https://linear.app/valora/issue/ACT-1060/add-optimism-arbitrum-to-wallet ### Backwards compatibility na --------- Co-authored-by: Tom McGuire <Mcgtom10@gmail.com>
Description
we should try to maintain a status quo where all we have to do in every repo to add a chain is update a "NetworkId" enum and fix all compilation errors that result from it
Test plan
added spot checks for the two affected configs
Related issues
cc https://linear.app/valora/issue/ACT-1060/add-optimism-arbitrum-to-wallet
Backwards compatibility
na