feat(connext): multi currency support #2043
Conversation
ghost
force-pushed
the
feat/connext-multi-currency
branch
2 times, most recently
from
dcff09a
to
720c65f
Compare
ghost
changed the title
|
This PR has evolved into:
Everything in the The PR is reviewable commit-by-commit so I'm not squashing the commits, yet. |
ghost
marked this pull request as ready for review
ghost
mentioned this pull request
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. |
There was a problem hiding this comment.
Looks good so far. I'm not familiar with ramda (but I take it it will move out of xud eventually) and only superficially familiar with currying functions so it's not exactly apparent to me what's going on in a few places. 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
added
the
code quality
|
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. |
Think it's not worth addressing them right now because once we've extracted |
ghost
force-pushed
the
feat/connext-multi-currency
branch
from
901e32d
to
ccaaa58
Compare
@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. |
| // This is the main function that has to be called before this package exposes more functions. | ||
| // Think of it as a constructor where we create the interal state of the module before | ||
| // we export more functionality to the consumer. | ||
| const getEthprovider = (host: string, port: number, name: string, chainId: number, seed: string) => { |
There was a problem hiding this comment.
The smallest of nits, I'm seeing mixed casing of Ethprovider and EthProvider. I don't know which one is "correct" but might make sense to stick to one.
There was a problem hiding this comment.
Thanks. Will address this in a follow-up PR.
This PR adds support for withdrawing from on-chain address. In the process,
ethersdependency 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).