Skip to content

Conversation

martonp
Copy link
Collaborator

@martonp martonp commented Jul 1, 2022

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.

@martonp
Copy link
Collaborator Author

martonp commented Jul 1, 2022

This should go in before #1638.

Copy link
Member

@chappjc chappjc left a comment

Choose a reason for hiding this comment

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

Concept ACK. Untested.

}

// If our estimate is higher than the calculated limit
gasLimit := g.Redeem * n
var gasLimit, gasFeeCap uint64 = g.Redeem * n, form.FeeSuggestion
Copy link
Member

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.
@martonp martonp force-pushed the redeemFeeUpdate branch from e824bdb to d2402e1 Compare July 7, 2022 11:49
@chappjc chappjc requested a review from JoeGruffins July 8, 2022 00:13
Copy link
Member

@chappjc chappjc left a 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.

Copy link
Member

@JoeGruffins JoeGruffins left a 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)
Copy link
Member

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))
Copy link
Member

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.

Copy link
Member

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

@chappjc chappjc added this to the 0.5 milestone Aug 26, 2022
@chappjc chappjc added the ETH label Aug 26, 2022
@martonp martonp deleted the redeemFeeUpdate branch January 20, 2024 13:16
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.

3 participants