feat(connext): multi currency support

ExchangeUnion / xud

Exchange Union Daemon 🔁 ⚡️

https://exchangeunion.com

GNU Affero General Public License v3.0

115 stars 44 forks source link

feat(connext): multi currency support #2043

Closed ghost closed 3 years ago

ghost commented 3 years ago

This PR adds support for withdrawing from on-chain address. In the process, ethers dependency is added to make development flow with the eth provider considerably faster. Previously, we were using the json-rpc API directly.

I'm planning to extract Connext swap client into a separate module so that the dependency will be opt-in (only for those who activate the swap client).

ghost commented 3 years ago

This PR has evolved into:

adding support for xucli walletwithdraw for ETH/ERC20
monitoring channel for ERC20 deposits -> creating a new commitment transaction so that ERC20 tokens show up under channel balance"

Everything in the ethprovider.ts will be converted to a separate module. This in preparation for extracting ConnextClient into a separate module.

The PR is reviewable commit-by-commit so I'm not squashing the commits, yet.

sangaman commented 3 years ago

utACK

Fixing those lint warnings would be nice though

I left those warnings in when working on the eslint rules, they weren't trivial to fix but they also seemed like things that made sense to change (like redeclaring variables, which can be confusing and lead to some hard to find bugs imo), so I figured the warnings would be a good reminder to address later and to prevent new code from violating the rule. I didn't realize how annoying they'd be in the PR code diffs, but I guess that's a good motivator to actually fix these soonish.

That said, it doesn't look like this PR is introducing any new warnings, so I wouldn't let that hold this up.

sangaman commented 3 years ago

Let me know if/when I should review this again @erkarl .

ghost commented 3 years ago

Let me know if/when I should review this again @erkarl .

Will do. I'm waiting for a new Connext release to test that everything is working correctly in this branch.

ghost commented 3 years ago

That said, it doesn't look like this PR is introducing any new warnings, so I wouldn't let that hold this up.

Think it's not worth addressing them right now because once we've extracted ConnextClient into a separate module all code from current ConnextClient.ts should be ported over. Provided I don't introduce new warnings - these warning should disappear.

ghost commented 3 years ago

I think more comments/method block comments in the ethProvider file would be good to explain what everything is does, so it's more comprehensible in the future for others who might not be so familiar either.

@sangaman I added comments to ethProvider file - hope it is clearer now. Think this is ready for review again. I won't be adding more functionality to this PR.