-
Notifications
You must be signed in to change notification settings - Fork 2.9k
Implement worst case scenario for price algorithm v1 #2219
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
Changes from 7 commits
dc3a670
7693ecd
334bd3f
3db5157
adffd99
dd6bf4d
512613a
4337b14
d1672d3
6cb3161
8875466
c55aa65
94dc8b7
e2346f1
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change | ||||||||||||||||||||||||
|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
|
|
@@ -4,3 +4,24 @@ | |||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||
| pub mod v0; | ||||||||||||||||||||||||||
| pub mod v1; | ||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||
| #[allow(clippy::cast_possible_truncation)] | ||||||||||||||||||||||||||
| pub(crate) fn cumulative_percentage_change( | ||||||||||||||||||||||||||
| new_exec_price: u64, | ||||||||||||||||||||||||||
| for_height: u32, | ||||||||||||||||||||||||||
| percentage: u64, | ||||||||||||||||||||||||||
| height: u32, | ||||||||||||||||||||||||||
| ) -> u64 { | ||||||||||||||||||||||||||
| let blocks = height.saturating_sub(for_height) as f64; | ||||||||||||||||||||||||||
| let percentage_as_decimal = percentage as f64 / 100.0; | ||||||||||||||||||||||||||
| let multiple = (1.0f64 + percentage_as_decimal).powf(blocks); | ||||||||||||||||||||||||||
| let mut approx = new_exec_price as f64 * multiple; | ||||||||||||||||||||||||||
| // account for rounding errors and take a slightly higher value | ||||||||||||||||||||||||||
| const ARB_CUTOFF: f64 = 16948547188989277.0; | ||||||||||||||||||||||||||
| if approx > ARB_CUTOFF { | ||||||||||||||||||||||||||
| const ARB_ADDITION: f64 = 2000.0; | ||||||||||||||||||||||||||
| approx += ARB_ADDITION; | ||||||||||||||||||||||||||
| } | ||||||||||||||||||||||||||
|
||||||||||||||||||||||||||
| // account for rounding errors and take a slightly higher value | |
| const ARB_CUTOFF: f64 = 16948547188989277.0; | |
| if approx > ARB_CUTOFF { | |
| const ARB_ADDITION: f64 = 2000.0; | |
| approx += ARB_ADDITION; | |
| } | |
| // account for rounding errors and take a slightly higher value | |
| const ROUNDING_ERROR_CUTOFF: f64 = 16948547188989277.0; | |
| if approx > ROUNDING_ERROR_CUTOFF { | |
| const ROUNDING_ERROR_COMPENSATION: f64 = 2000.0; | |
| approx += ROUNDING_ERROR_COMPENSATION; | |
| } |
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.
Background:
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.
It doesn't sound arbitrary then. Can we rename it to something like this?
There isn't a "correct" value to choose here, that's why I called it ARB, but you're right that the reason we chose it isn't completely arbitrary and your suggestion is much clearer and helpful anyway.
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.
Variable names updated in 4337b14
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -1,4 +1,8 @@ | ||
| use crate::v1::tests::UpdaterBuilder; | ||
| use crate::v1::{ | ||
| tests::UpdaterBuilder, | ||
| AlgorithmV1, | ||
| }; | ||
| use proptest::prelude::*; | ||
|
|
||
| #[test] | ||
| fn calculate__returns_sum_of_da_and_exec_gas_prices() { | ||
|
|
@@ -28,3 +32,60 @@ fn calculate__returns_sum_of_da_and_exec_gas_prices() { | |
| let expected = starting_exec_gas_price + starting_da_gas_price; | ||
| assert_eq!(expected, actual); | ||
| } | ||
|
|
||
| fn _worst_case__correctly_calculates_value( | ||
| new_exec_price: u64, | ||
| new_da_gas_price: u64, | ||
| for_height: u32, | ||
| block_horizon: u32, | ||
| exec_price_percentage: u64, | ||
| da_gas_price_percentage: u64, | ||
| ) { | ||
| // given | ||
| let algorithm = AlgorithmV1 { | ||
| new_exec_price, | ||
| exec_price_percentage, | ||
| new_da_gas_price, | ||
| da_gas_price_percentage, | ||
| for_height, | ||
| }; | ||
|
|
||
| // when | ||
| let target_height = for_height.saturating_add(block_horizon); | ||
| let actual = algorithm.worst_case(target_height); | ||
|
|
||
| // then | ||
| let mut expected_exec_price = new_exec_price; | ||
| let mut expected_da_gas_price = new_da_gas_price; | ||
|
|
||
| for _ in 0..block_horizon { | ||
| let change_amount = expected_exec_price | ||
| .saturating_mul(exec_price_percentage) | ||
| .saturating_div(100); | ||
| expected_exec_price = expected_exec_price.saturating_add(change_amount); | ||
|
|
||
| let change_amount = expected_da_gas_price | ||
| .saturating_mul(da_gas_price_percentage) | ||
| .saturating_div(100); | ||
| expected_da_gas_price = expected_da_gas_price.saturating_add(change_amount); | ||
| } | ||
|
|
||
| let expected = expected_exec_price.saturating_add(expected_da_gas_price); | ||
|
|
||
| dbg!(actual, expected); | ||
| assert!(actual >= expected); | ||
| } | ||
|
|
||
| proptest! { | ||
| #[test] | ||
| fn worst_case__correctly_calculates_value( | ||
|
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. We should add another test to check for overflows. See the V0 test and this convo:
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Test added in 6cb3161 |
||
| exec_price: u64, | ||
| da_price: u64, | ||
| starting_height: u32, | ||
| block_horizon in 0..10_000u32, | ||
| exec_percentage: u64, | ||
| da_percentage: u64, | ||
| ) { | ||
| _worst_case__correctly_calculates_value(exec_price, da_price, starting_height, block_horizon, exec_percentage, da_percentage); | ||
| } | ||
| } | ||
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.
It feels unclean to have this function on the crate root. How about putting it in a
utilsmodule or something similar?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.
Updated in d1672d3