-
Notifications
You must be signed in to change notification settings - Fork 2.8k
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
feat(gas_price_service_v1): include max_da_gas_price
to control volatility
#2541
feat(gas_price_service_v1): include max_da_gas_price
to control volatility
#2541
Conversation
0108782
to
4fd0ee2
Compare
@@ -540,6 +548,12 @@ impl AlgorithmUpdaterV1 { | |||
.saturating_mul(self.gas_price_factor.into()) | |||
} | |||
|
|||
fn max_scaled_da_gas_price(&self) -> u64 { | |||
// note: here we make sure that a correct maximum is used; hacky :( | |||
max(self.max_da_gas_price, self.min_da_gas_price + 1) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Probably can just use min_da_gas_price
without the + 1
, but maybe there is a weird edge case I missing.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
yup, detected and fixed
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
addressed in 4c42380
max( | ||
self.min_scaled_da_gas_price(), | ||
maybe_new_scaled_da_gas_price, | ||
), | ||
self.max_scaled_da_gas_price(), |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Let's add some tests for this.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
addressed in 4c42380
crates/fuel-gas-price-algorithm/src/v1/tests/update_da_record_data_tests.rs
Outdated
Show resolved
Hide resolved
Co-authored-by: Brandon Kite <[email protected]>
…data_tests.rs Co-authored-by: Mitchell Turner <[email protected]>
7ad61f7
to
261045f
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM 👍
ae40ee4
into
chore/add-tests-for-v1-gas-service
Linked Issues/PRs
Description
Adds a new config param
max_da_gas_price
, whichnew_scaled_da_gas_price
depends upon while mutating it.Checklist
Before requesting review
After merging, notify other teams
[Add or remove entries as needed]