Skip to content
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.

Sign up for GitHub

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

Jump to bottom

feat: add connext vector support #1938

Merged
1 commit merged into from
Dec 7, 2020
Merged

feat: add connext vector support #1938

1 commit merged into from
Dec 7, 2020

Conversation

ghost
Copy link

@ghost ghost commented Oct 19, 2020 •
edited by ghost
Loading

@ghost ghost self-assigned this Oct 19, 2020
@ghost ghost added connext breaking A breaking or non-backwards compatible change swaps labels Oct 19, 2020
@kilrau kilrau requested a review from sangaman October 21, 2020 14:36
@ghost ghost force-pushed the feat/connext-vector branch from 0f79a94 to 44a7ab5 Compare October 28, 2020 10:21
@ghost ghost mentioned this pull request Nov 2, 2020
Merged
@ghost ghost force-pushed the feat/connext-vector branch 4 times, most recently from ea659e0 to 6c9a12a Compare November 26, 2020 19:09
@ghost ghost marked this pull request as ready for review November 26, 2020 20:05
@ghost ghost force-pushed the feat/connext-vector branch from 6c9a12a to eb40068 Compare November 26, 2020 20:05
@ghost ghost requested review from sangaman and michael1011 and removed request for sangaman November 26, 2020 20:26
@ghost ghost force-pushed the feat/connext-vector branch from eb40068 to 2644404 Compare November 27, 2020 09:23
michael1011
michael1011 reviewed Nov 30, 2020
View reviewed changes
Copy link
Contributor

@michael1011 michael1011 left a comment

Choose a reason for hiding this comment

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

And I have one more architectural question: who pays for the gas of the withdrawals from channels?

lib/connextclient/ConnextClient.ts Show resolved Hide resolved
lib/connextclient/ConnextClient.ts Outdated Show resolved Hide resolved
lib/connextclient/ConnextClient.ts Outdated
const blockHeight = await this.getHeight();
const timelock = expiry - blockHeight;
// The timelock can be up to 10 blocks less than the agreed upon value
const TIMELOCK_BUFFER = 10;
Copy link
Contributor

Choose a reason for hiding this comment

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

Why 10?

Copy link
Author

Choose a reason for hiding this comment

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

It's arbitrary. Can you recommend a better value?

lib/connextclient/ConnextClient.ts
amount: amount.toLocaleString('fullwide', { useGrouping: false }),
assetId: this.tokenAddresses.get(currency),
recipient: destination,
fee: '120', // TODO: estimate fee
Copy link
Contributor

Choose a reason for hiding this comment

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

Can be done with a call to the eth provider: eth_gasPrice

What is the default value when it is not set here?

Copy link
Author

Choose a reason for hiding this comment

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

Thanks, will address this in a separate PR. Static "120" works for simnet.

@ghost ghost force-pushed the feat/connext-vector branch from 76c3a11 to 67da614 Compare December 1, 2020 14:20
sangaman
sangaman reviewed Dec 1, 2020
View reviewed changes
Copy link
Collaborator

@sangaman sangaman left a comment

Choose a reason for hiding this comment

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

I haven't looked closely through the changes to the test code yet but I can give that a review as well after tests are passing or if we're having trouble getting tests to pass.

lib/Xud.ts Outdated Show resolved Hide resolved
lib/connextclient/errors.ts Show resolved Hide resolved
lib/connextclient/types.ts Outdated Show resolved Hide resolved
lib/connextclient/types.ts Show resolved Hide resolved
lib/swaps/SwapClient.ts Outdated Show resolved Hide resolved
lib/swaps/SwapRecovery.ts Outdated Show resolved Hide resolved
package.json Show resolved Hide resolved
proto/xudrpc.proto Outdated Show resolved Hide resolved
@ghost ghost force-pushed the feat/connext-vector branch 3 times, most recently from ce1bac6 to b2fce21 Compare December 2, 2020 21:56
@ghost ghost requested review from michael1011 and sangaman December 2, 2020 22:03
@ghost
Copy link
Author

ghost commented Dec 2, 2020

And I have one more architectural question: who pays for the gas of the withdrawals from channels?

The user, it's deducted from the channel balance.

@ghost ghost force-pushed the feat/connext-vector branch from b2fce21 to 914f281 Compare December 3, 2020 09:51
@ghost ghost force-pushed the feat/connext-vector branch from 914f281 to d850f7e Compare December 3, 2020 09:57
@sangaman sangaman mentioned this pull request Dec 4, 2020
Open
sangaman
sangaman reviewed Dec 4, 2020
View reviewed changes
Copy link
Collaborator

@sangaman sangaman left a comment

Choose a reason for hiding this comment

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

The code looks good to me. What is the plan for re-enabling the simulation tests that have been commented out? And would it make sense to comment out the connext related instability tests rather than simply deleting them, if we plan to re-enable them in the near future?

@ghost
Copy link
Author

ghost commented Dec 4, 2020

What is the plan for re-enabling the simulation tests that have been commented out?

We definitely want to get simulation tests going for v2 as well, but not in this PR. I'll create follow up issues from xud-docker/xud side once the current PRs are ready to merge.

And would it make sense to comment out the connext related instability tests rather than simply deleting them, if we plan to re-enable them in the near future?

I did remove/disable v1 connext simtests in a separate PR (#2008) so that if we get around to enable v2 we can easily see the history of what was deleted (instead of going through git history).

@kilrau kilrau mentioned this pull request Dec 4, 2020
Closed
sangaman
sangaman approved these changes Dec 4, 2020
View reviewed changes
Copy link
Collaborator

@sangaman sangaman left a comment

Choose a reason for hiding this comment

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

Looking forward to having this all work! Thanks.

@ghost ghost merged commit 2df5867 into master Dec 7, 2020
@ghost ghost deleted the feat/connext-vector branch December 7, 2020 10:17
This pull request was closed.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@michael1011 michael1011 Awaiting requested review from michael1011

Assignees
No one assigned
Labels
breaking A breaking or non-backwards compatible change connext swaps
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
2 participants
@sangaman @michael1011