Skip to content

feat(utils): Utility functions for computing and estimating the max_fee of a proof - #1045

Closed
PatStiles wants to merge 23 commits into
stagingfrom
feat/max_fee_estimate
Closed

feat(utils): Utility functions for computing and estimating the max_fee of a proof#1045
PatStiles wants to merge 23 commits into
stagingfrom
feat/max_fee_estimate

Conversation

@PatStiles

Copy link
Copy Markdown
Contributor

This pr adds three functions to aligned_sdk:

  • estimate_max_fee: Returns to the user there estimated max_fee: Min, Default, Instant based on a theoretical batch of 32 proofs. 32 was decided upon as a suggested arbitrary large batch size.
  • fee_per_proof: Returns the fee_per_proof of a batch size of size num_proofs_per_batch
  • compute_max_feee: returns the computing max_fee for a proof based on (num_proofs / num_proofs_per_batch).

It deduplicates constants originally found in aligned-batcher that can be imported once #964 is completed.

@MauroToscanoMauroToscano left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Decouple calculation from the fetching of the GasCost, and add tests assuming you have the GasCost. It can be just checking that one is bigger than another one

@PatStiles

Copy link
Copy Markdown
ContributorAuthor

@MauroToscano tests added

@MauroToscanoMauroToscano left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Mention this function in the one that is used to send a task, with ///, so the user knows this one should be used to get the right estimation

Comment threadbatcher/aligned-sdk/src/sdk.rs Outdated
Comment threadbatcher/aligned-sdk/src/sdk.rs Outdated
Comment threadbatcher/aligned-sdk/src/sdk.rs Outdated
Comment threadbatcher/aligned-sdk/src/sdk.rs Outdated
Comment threadbatcher/aligned-sdk/src/sdk.rs
Comment threadbatcher/aligned-sdk/src/sdk.rs Outdated
Comment on lines +171 to +189
/// Retrieves the `gas_price` from an rpc nodes. Specifically, it returns the max priority gas price.
async fn fetch_gas_price(eth_rpc_url: &str) -> Result<U256, errors::SubmitError> {
let eth_rpc_provider =
Provider::<Http>::try_from(eth_rpc_url).map_err(|e: url::ParseError| {
errors::VerificationError::EthereumProviderError(e.to_string())
})?;

let gas_price = match eth_rpc_provider.get_gas_price().await {
Ok(price) => price,
Err(e) => {
return Err(SubmitError::GenericError(format!(
"Failed to fetch gas price: {:?}",
e
)))
}
};

Ok(gas_price)
}

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

I think it doesn't make much sense to define a function on our SDK just to call the rpc provider's get_gas_price method. Maybe a better pattern will be to instantiate a provider only once, and ask for the gas price directly to it. This data can be used directly in the other functions without the need need to instantiate a provider every time.
@Oppen if you have any thoughts on this please feel free to share them

@PatStilesPatStilesSep 26, 2024

Copy link
Copy Markdown
ContributorAuthor

Choose a reason for hiding this comment

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

Decoupling fetching the gas cost was done in response to this comment above #1045 (review) in 514ca8d . Happy to revert the change if that is the consensus.

Copy link
Copy Markdown
ContributorAuthor

Choose a reason for hiding this comment

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

Now passes provider

PatStilesand others added 7 commits September 30, 2024 19:26
Co-authored-by: Urix <43704209+uri-99@users.noreply.github.com>
Co-authored-by: Urix <43704209+uri-99@users.noreply.github.com>
Co-authored-by: JuArce <52429267+JuArce@users.noreply.github.com>
Co-authored-by: JuArce <52429267+JuArce@users.noreply.github.com>
@PatStiles
PatStiles deleted the feat/max_fee_estimate branch October 1, 2024 15:03
@PatStiles
PatStiles restored the feat/max_fee_estimate branch October 1, 2024 15:04
@PatStilesPatStiles reopened this Oct 1, 2024
@MauroToscano
MauroToscano deleted the feat/max_fee_estimate branch January 2, 2025 18:21
@MauroToscano
MauroToscano restored the feat/max_fee_estimate branch January 2, 2025 18:21
@MauroToscano
MauroToscano deleted the feat/max_fee_estimate branch January 2, 2025 18:21
Sign up for freeto 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.

8 participants

@PatStiles@Oppen@MauroToscano@entropidelic@JuArce@IAvecilla@uri-99@MarcosNicolau