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: improve Gas Fee Estimation by Integrating Filecoin's EIP-1559-Compatible APIs #1182

Open
wants to merge 4 commits into
base: main
Choose a base branch
from

Conversation

karlem
Copy link
Contributor

@karlem karlem commented Oct 23, 2024

Close #1175
Close #982

This PR updates our gas fee estimation to leverage the eth_maxPriorityFeePerGas and eth_getBlockByNumber (to retrieve the current base_fee_per_gas from the latest mined block) API calls, using Eth-standard APIs available through Filecoin. Since Filecoin supports EIP-1559, these calls are fully compatible with Filecoin's implementation, aligning with standard EVM practices and ensuring interoperability across EVM chains.

Key Changes:

  • The max_priority_fee_per_gas() and base_fee_per_gas() functions retrieve gas fee estimates based on Filecoin's adherence to EIP-1559.
  • Gas fee calculation buffers the base fee with a multiplier of 2 to accommodate potential cumulative increases, aligning with the 12.5% maximum cap on base fee growth per block. By doubling the base fee, we account for possible surges in consecutive blocks, ensuring that estimates remain responsive to rapid market changes while providing a buffer for handling block-to-block volatility.

This approach simplifies our fee estimation logic by utilizing available EIP-1559-compatible APIs, improving chances of capturing better gas market dynamics on specific networks (Filecoin in this case) to increase chance of fast transaction inclusion within a block.

@karlem karlem changed the title feat: use estimation from Alloy feat: align wi Oct 25, 2024
@karlem karlem changed the title feat: align wi feat: align with Filecoin implementation Oct 25, 2024
@karlem karlem changed the title feat: align with Filecoin implementation Improve Gas Fee Estimation by Integrating Filecoin's EIP-1559-Compatible APIs Oct 25, 2024
@karlem karlem changed the title Improve Gas Fee Estimation by Integrating Filecoin's EIP-1559-Compatible APIs fix: improve Gas Fee Estimation by Integrating Filecoin's EIP-1559-Compatible APIs Oct 28, 2024
@karlem karlem marked this pull request as ready for review October 28, 2024 13:22
@karlem karlem requested a review from a team as a code owner October 28, 2024 13:22
@karlem karlem self-assigned this Oct 28, 2024

*tx = TypedTransaction::Eip1559(tx_req);
} else {
return Err(Eip1559GasEstimatorError::failed_to_estimate_gas_not_supported());
Copy link
Contributor

Choose a reason for hiding this comment

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

why not just skip the error so that other types of txns can go through so that it's more general. Or the purpose is only supporting EIP1559?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Or the purpose is only supporting EIP1559?

Yes. As these endpoints are only available from EIP1559 upgrade.


// Set the gas fees directly on the transaction.
let tx_req = inner
.clone()
Copy link
Contributor

Choose a reason for hiding this comment

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

why need to clone here instead of directly assign?


// Delegate to the inner middleware for filling remaining transaction fields.
self.inner()
.fill_transaction(tx, block)
Copy link
Contributor

Choose a reason for hiding this comment

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

will the gas be overwritten by inner fill transaction? Will call inner first then overwrite the gas params be safer?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I think - from what I have seen, the gas parameter is only set internally when it's not defined. But we define it here.


// Proceed to send the transaction with the inner middleware.
self.inner()
.send_transaction(tx, block)
Copy link
Contributor

Choose a reason for hiding this comment

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

same question here, the inner will not overwrite the gas params updated right?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Nope it should not. Answer above.👆🏻

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
Status: Backlog
Development

Successfully merging this pull request may close these issues.

Review ipc-cli gas parameters ipc relayer uses fixed gas parameters
2 participants