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.

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

feat(connext): estimate fee for closechannel #2026

Merged
2 commits merged into from
Dec 8, 2020
Merged

Conversation

ghost
Copy link

@ghost ghost commented Dec 7, 2020

This PR adds fee estimation logic to ConnextClient.closeChannel. Previously, a static fee was used.

Related issue: #2019

@ghost ghost requested a review from sangaman December 7, 2020 13:41
@ghost ghost self-assigned this Dec 7, 2020
@ghost ghost mentioned this pull request Dec 7, 2020
14 tasks
@raladev raladev self-requested a review December 7, 2020 21:13
raladev
raladev previously approved these changes Dec 7, 2020
@@ -900,6 +901,21 @@ class ConnextClient extends SwapClient {
return parseInt(blockNumberResponse.result, 16);
}

private getGasPrice = async () => {
Copy link
Collaborator

Choose a reason for hiding this comment

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

A comment here would be helpful too in regards to what exactly this returns - is this getting the most recent average gas price? Or is this derived from some fee estimation logic built into geth?

Copy link
Contributor

@kilrau kilrau Dec 8, 2020

Choose a reason for hiding this comment

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

Shouldn't break anything on the rpc, but here a heads up for this upcoming change in geth how gas prices are calculated (moving from the current auction model/fee market based on which geth currently estimates gas prices -> moving average): ethereum/go-ethereum#21971

thanks for pointing me at it @michael1011

Copy link
Contributor

Choose a reason for hiding this comment

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

The method call Karl is using returns data from GETHs internal fee estimation logic.

What Kilian mentioned is EIP-1559 which will be a breaking change in the way Ethereum handles gas prices and eventually be activated in a hardfork. But that won't matter for us since the REST endpoints of GETH won't change because of that.

Copy link
Author

Choose a reason for hiding this comment

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

Comment added.

@ghost ghost merged commit cac878f into master Dec 8, 2020
@ghost ghost deleted the feat/connext-estimate-fee branch December 8, 2020 13:52
This pull request was closed.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants