-
Notifications
You must be signed in to change notification settings - Fork 127
client/eth: Increase gas fee cap for redemptions when base fee is too high #1692
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
Conversation
This should go in before #1638. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Concept ACK. Untested.
client/asset/eth/eth.go
Outdated
} | ||
|
||
// If our estimate is higher than the calculated limit | ||
gasLimit := g.Redeem * n | ||
var gasLimit, gasFeeCap uint64 = g.Redeem * n, form.FeeSuggestion |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Shorthand is fine with this multiple assignment.
This diff increases the gas fee cap for redemptions to 2x base fee if the base fee > the fee suggestion. If the wallet does not have enough available funds for this, then the funds that are available will be used.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM. A simple check and updated that's worth doing to avoid a stuck txn.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Sorry, was in the middle of looking at this so leaving what I had. Looks good to me.
if err != nil { | ||
return nil, nil, 0, fmt.Errorf("error getting redemption estimate: %v", err) | ||
return nil, nil, 0, fmt.Errorf("error getting balance in excessive gas fee recovery: %v", err) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
could use return fail(~error~)
// funds we have available. | ||
baseFee, _, err := w.node.currentFees(w.ctx) | ||
if err != nil { | ||
return fail(fmt.Errorf("Error getting net fee state: %w", err)) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Don't need to capitalize Error
unless logging full sentences.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Good points @JoeGruffins. Sorry to interrupt your review. @martonp can fold these revisions into #1638
This diff increases the gas fee cap for redemptions to 2x base fee if the base fee > the fee suggestion. If the wallet does not have enough available funds for this, then the funds that are available will be used.