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

fix: increase gas multiple on each resend attempt #746

Merged
merged 9 commits into from
Dec 8, 2024

Conversation

arcoraven
Copy link
Contributor

@arcoraven arcoraven commented Oct 31, 2024

PR-Codex overview

This PR focuses on enhancing the sendTransactionWorker functionality by implementing a new method, _updateGasFees, which dynamically adjusts gas fees based on the resend attempt count. It also introduces unit tests to validate the behavior of this method.

Detailed summary

  • Added a new function _updateGasFees in src/worker/tasks/sendTransactionWorker.ts to compute gas fees based on resend attempts.
  • Refactored gas fee handling in the _resendTransaction function to utilize _updateGasFees.
  • Created unit tests for _updateGasFees in src/tests/sendTransactionWorker.test.ts to cover various scenarios, including:
    • Returning original transaction values on first send.
    • Doubling gasPrice for legacy transactions.
    • Capping gasPrice multiplier at 10x.
    • Updating maxPriorityFeePerGas and maxFeePerGas for EIP-1559 transactions.
    • Respecting overrides for both maxPriorityFeePerGas and maxFeePerGas.
    • Handling cases where only one of maxPriorityFeePerGas or maxFeePerGas is set.

✨ Ask PR-Codex anything about this PR by commenting with /codex {your question}

src/utils/math.ts Outdated Show resolved Hide resolved
}
if (
populatedTransaction.maxPriorityFeePerGas &&
!overrides?.maxPriorityFeePerGas
) {
populatedTransaction.maxPriorityFeePerGas *= 2n;
populatedTransaction.maxPriorityFeePerGas *= gasMultiple;
Copy link
Member

Choose a reason for hiding this comment

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

nit:

maxFeePerGas = maxBaseFeePerGas + maxPriorityFeePerGas

Since maxBaseFeePerGas is uniform across all transactions in a block, setting it significantly above the current base fee provides no benefit. Any excess above the actual block's base fee cannot be collected.

Suggest focusing on increasing maxPriorityFeePerGas instead, as this:

  1. Directly incentivizes validators to include the transaction
  2. Can actually be collected (unlike excess base fee)

Consider reducing maxBaseFeePerGas multiplier and redistributing the margin to maxPriorityFeePerGas for more effective fee management.


// For EIP-1559 transactions
if (populatedTransaction.maxPriorityFeePerGas && !overrides?.maxPriorityFeePerGas) {
 const originalPriorityFee = populatedTransaction.maxPriorityFeePerGas;
 populatedTransaction.maxPriorityFeePerGas *= priorityFeeMultiple;
 
 // Adjust maxFeePerGas to accommodate the increased priority fee
 if (populatedTransaction.maxFeePerGas && !overrides?.maxFeePerGas) {
   const originalBaseFee = populatedTransaction.maxFeePerGas - originalPriorityFee;
   populatedTransaction.maxFeePerGas = originalBaseFee + populatedTransaction.maxPriorityFeePerGas;
 }
}

// reset it to a QueuedTransaction to try again.
// No transaction hashes means the transaction is not in-flight.
if (
transaction.status === "errored" &&
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Fixes a wrong assumption where resendCount == 0 means the tx was never sent. ResendCount = 0 just means the tx was never "resent". It may have been sent once.

Copy link

This PR is stale because it has been open for 7 days with no activity. Remove stale label or comment or this PR will be closed in 3 days.

@arcoraven arcoraven reopened this Dec 7, 2024
@arcoraven arcoraven merged commit a802d87 into main Dec 8, 2024
5 checks passed
@arcoraven arcoraven deleted the ph/increaseGasOnResend branch December 8, 2024 02:27
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants