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

chore(bolt-sidecar): add params in err message #660

Merged

Conversation

faheelsattar
Copy link
Contributor

Closes #647

@thedevbirb
Copy link
Contributor

Hey @faheelsattar thanks for the contribution! I've taken a look at it and the return type of validate_min_priority_fee made me realize that there is other refactoring that maybe should be done. Sorry about that.

@estensen I think that the enum PricingError should at least absorb the ValidationError::MaxPriorityFeePerGasTooLow error since they're logically related. But at that point some fee-related errors are in PricingError and others in ValidationError. How would you handle that?

@estensen
Copy link
Contributor

@estensen I think that the enum PricingError should at least absorb the ValidationError::MaxPriorityFeePerGasTooLow error since they're logically related. But at that point some fee-related errors are in PricingError and others in ValidationError. How would you handle that?

Map internal errors like PricingError to ValidationError before returning to the user?

@@ -185,24 +185,24 @@ impl InclusionRequest {
preconfirmed_gas: u64,
min_inclusion_profit: u64,
max_base_fee: u128,
) -> Result<bool, PricingError> {
) -> Result<(bool, u128, u128), PricingError> {
Copy link
Contributor

Choose a reason for hiding this comment

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

Are you able to keep the signature like before? Result<bool, PricingError> and embed the values in the error instead?

Copy link
Contributor

Choose a reason for hiding this comment

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

Let us know if you get stuck!

Copy link
Contributor Author

Choose a reason for hiding this comment

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

updated

Copy link
Contributor

@0ex-d 0ex-d left a comment

Choose a reason for hiding this comment

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

Great work @faheelsattar , i suggested some changes that could help, feel free to let me know what you think.
Also, I think there is already a From impl PricingError for ValidationError?


let tip = tx.effective_tip_per_gas(max_base_fee).unwrap_or_default();
if tip < min_priority_fee as u128 {
return Ok(false);
return Ok((false, tip, min_priority_fee as u128));
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
return Ok((false, tip, min_priority_fee as u128));
return Ok(false);

}
// Increment the preconfirmed gas for the next transaction in the bundle
local_preconfirmed_gas = local_preconfirmed_gas.saturating_add(tx.gas_limit());
}
Ok(true)
Ok((true, 0, 0))
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
Ok((true, 0, 0))
Ok(true)

@@ -185,24 +185,24 @@ impl InclusionRequest {
preconfirmed_gas: u64,
min_inclusion_profit: u64,
max_base_fee: u128,
) -> Result<bool, PricingError> {
) -> Result<(bool, u128, u128), PricingError> {
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
) -> Result<(bool, u128, u128), PricingError> {
) -> Result<bool, PricingError> {

if !req.validate_min_priority_fee(
// Ensure max_priority_fee_per_gas is greater than or equal to the calculated
// min_priority_fee
let (validated, tip, min_priority_fee) = req.validate_min_priority_fee(
Copy link
Contributor

Choose a reason for hiding this comment

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

if let Err(err) = req.validate_min_priority_fee(

Comment on lines 329 to 331
)?;
if !validated {
return Err(ValidationError::MaxPriorityFeePerGasTooLow(tip, min_priority_fee));
Copy link
Contributor

@0ex-d 0ex-d Jan 13, 2025

Choose a reason for hiding this comment

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

Suggested change
)?;
if !validated {
return Err(ValidationError::MaxPriorityFeePerGasTooLow(tip, min_priority_fee));
) {
return Err(err.into());

@thedevbirb thedevbirb self-requested a review January 14, 2025 09:24
@faheelsattar
Copy link
Contributor Author

hey guys thanks for leaving the comments. Will try to take care of it today

@estensen estensen changed the title add params in err message chore(bolt-sidecar): add params in err message Jan 15, 2025
Copy link
Contributor

@estensen estensen left a comment

Choose a reason for hiding this comment

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

LGTM, thank you for your contribution!

@estensen
Copy link
Contributor

next time would you mind rebasing instead of merging unstable into your feature branch?

@estensen estensen requested review from merklefruit and removed request for 0ex-d January 15, 2025 09:38
@faheelsattar
Copy link
Contributor Author

next time would you mind rebasing instead of merging unstable into your feature branch?

oops, yeah for sure will definitely keep in mind for next PR's

Copy link
Contributor

@thedevbirb thedevbirb left a comment

Choose a reason for hiding this comment

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

Thanks!

@thedevbirb thedevbirb merged commit bb3287a into chainbound:unstable Jan 15, 2025
3 checks passed
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.

Return fee in MaxPriorityFeePerGasTooLow to make it more useful for consumers
5 participants