Support any network

[anxolin]

Closes #52.

This PR generalize the type ChainId, adding support for any network. Also adds xDAI as a Chain (enum)

As suggested in #53 by @moodysalem , I took a more generalist approach to support any network.

Basically, now ChainId is a number (a ChainId | number in TS becomes number since one is a subset of the other). It also makes sense to model the chainId as a number.

The enum with some convenient known chains it's now called Chain.
I had to adapt in several places the use of the enum (i.e. ChainId.MAINNET ---> Chain.MAINNET)

export enum Chain {
  MAINNET = 1,
  ROPSTEN = 3,
  ...
}	

export type ChainId = number

Also WETH is typed as Partial<Record<ChainId, Token>>, so we might find networks where we don't know the WETH contract. That's good, because reflects reality, there's even networks where WETH is not used (xDAI has "Wrapped xDAI" not WETH). I needed to account in a few places.

export const WETH: Partial<Record<ChainId, Token>> = {  
  [Chain.MAINNET]: WETH_MAINNET,
  [Chain.ROPSTEN]: WETH_ROPSTEN,
   ...
}

[anxolin]

@moodysalem I implemented what you suggested in your comment.
https://github.com/Uniswap/uniswap-sdk/pull/53#pullrequestreview-571403955

Cheers,

[AdamREQ]

@moodysalem @anxolin any update on this?

[anxolin]

@adamdowson I'm waiting for the review of @moodysalem

If it's just for xDAI it might be faster to start adding #53. Anyways, I think is a good move to make the SDK more generic accepting any network. In my opinion, the SDK shouldn't be constrained to a hardcoded enum of accepted networks. It's even limiting the possibilities on local ganache networks, future side chains, or testnets.

[Skankhunt69420]

Hello fellow developers!
When can I use uniswap on the xdai layer?

[moodysalem]

Adding the xDAI enum and a factory/router address not deployed by Uniswap is not going to work, since we didn't deploy it/verify it. Also, it would be better if the types accepted in place of ChainId were ChainId | number and it might make sense for ChainId to have string values to differentiate from arbitrary chain IDs

Also, will just add that it's not a good idea to call it Uniswap/show Uniswap branding in the interface if we didn't deploy it and verify it, since it will not be clear to users that it is not deployed by Uniswap and not a malicious fork

[anxolin]

Adding the xDAI enum and a factory/router address not deployed by Uniswap is not going to work, since we didn't deploy it/verify it

I understand, but maybe you would feel more comfortable if you deploy the factory in xDAI yourself? I can happily change it to a Uniswap factory.

Also, it would be better if the types accepted in place of ChainId were ChainId | number

ChainId is a "subset" of number type already, so doing ChainId | number is effectively setting it to a number. See image:

https://www.typescriptlang.org/play?#code/KYDwDg9gTgLgBMAdgVwLZwMIAsCGBLROAbwCg44BZAQQEkA5OgUQBU4BeOARgBoBIc6vSasOPMnABKAeQAKAZWaM67OAGY+5afMXKO68RPoBpRgCEAmioAsGycbOWON8QHEAaxIAyNFQFZb7l4+HP7iRlIAalS6cFYATPxw4VEx8dziABoAIrQqnAAM+SQAvrzFJCQwAJ5gwJi4BDQAJirY+IQAPnAoqABGwFAVQA

I can change that, but I think it would be equivalent.

[moodysalem]

I understand, but maybe you would feel more comfortable if you deploy the factory in xDAI yourself? I can happily change it to a Uniswap factory.

We'll talk about it internally, but I can't give any guarantees on timelines since xDAI is imo a less optimal scaling solution than the L2s coming up

I can change that, but I think it would be equivalent.

Sorry, this is why I suggested changing ChainId to be an enum with string values, e.g.

enum ChainId {
  MAINNET = 'MAINNET',
  ROPSTEN = 'ROPSTEN'
}

[anxolin]

We'll talk about it internally, but I can't give any guarantees on timelines since xDAI is imo a less optimal scaling solution than the L2s coming up

Hi @moodysalem did you have time for discussing this internally? I think adding xDAI is a big win for Uniswap, specially given the current ether prices. It can be an additional option you give to your users. Given the effort and benefit, it feels very advantageous.
xDAI is 100% compatible

Sorry, this is why I suggested changing ChainId to be an enum with string values, e.g.

Happy to change that, if it helps to get the PR merged. Let me know the factory I can use, and I'll fix both things together.

[stale]

Is this still relevant? If so, what is blocking it? Is there anything you can do to help move it forward?

This issue has been automatically marked as stale because it has not had recent activity. It will be closed if no further activity occurs.