-
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): define RunnableTask for GasPriceServiceV1 #2416
Changes from all commits
6dc5ed7
b6149d1
8a912c4
c9d7ed7
7a65574
d576665
b4d8855
0a358f6
49a309a
c516ea9
53291a5
2291207
8ae983f
b2a94d8
db04ae1
1e3b3f5
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 |
---|---|---|
|
@@ -270,3 +270,166 @@ fn update_da_record_data__da_block_updates_projected_total_cost_with_known_and_g | |
let expected = new_known_total_cost + guessed_part; | ||
assert_eq!(actual, expected as u128); | ||
} | ||
|
||
#[test] | ||
fn update_da_record_data__da_block_lowers_da_gas_price() { | ||
// given | ||
let da_cost_per_byte = 40; | ||
let da_recorded_block_height = 10; | ||
let l2_block_height = 11; | ||
let original_known_total_cost = 150; | ||
let unrecorded_blocks = vec![BlockBytes { | ||
height: 11, | ||
block_bytes: 3000, | ||
}]; | ||
let da_p_component = 2; | ||
let guessed_cost: u64 = unrecorded_blocks | ||
.iter() | ||
.map(|block| block.block_bytes * da_cost_per_byte) | ||
.sum(); | ||
let projected_total_cost = original_known_total_cost + guessed_cost; | ||
|
||
let mut updater = UpdaterBuilder::new() | ||
.with_da_cost_per_byte(da_cost_per_byte as u128) | ||
.with_da_p_component(da_p_component) | ||
.with_last_profit(10, 0) | ||
.with_da_recorded_block_height(da_recorded_block_height) | ||
.with_l2_block_height(l2_block_height) | ||
.with_projected_total_cost(projected_total_cost as u128) | ||
.with_known_total_cost(original_known_total_cost as u128) | ||
.with_unrecorded_blocks(unrecorded_blocks.clone()) | ||
.build(); | ||
|
||
let new_cost_per_byte = 100; | ||
let (recorded_heights, recorded_cost) = | ||
unrecorded_blocks | ||
.iter() | ||
.fold((vec![], 0), |(mut range, cost), block| { | ||
range.push(block.height); | ||
(range, cost + block.block_bytes * new_cost_per_byte) | ||
}); | ||
let min = recorded_heights.iter().min().unwrap(); | ||
let max = recorded_heights.iter().max().unwrap(); | ||
let recorded_range = *min..(max + 1); | ||
|
||
let old_da_gas_price = updater.new_scaled_da_gas_price; | ||
|
||
// when | ||
updater | ||
.update_da_record_data(recorded_range, recorded_cost as u128) | ||
.unwrap(); | ||
|
||
// then | ||
let new_da_gas_price = updater.new_scaled_da_gas_price; | ||
// because the profit is 10 and the da_p_component is 2, the new da gas price should be lesser than the previous one. | ||
assert_eq!(new_da_gas_price, 0); | ||
assert_ne!(old_da_gas_price, new_da_gas_price); | ||
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 be more specific at this level (and not just say it is different) You can copy most of the code from It feels redundant, but they are different endpoints so they change independently. |
||
} | ||
|
||
#[test] | ||
fn update_da_record_data__da_block_increases_da_gas_price() { | ||
// given | ||
let da_cost_per_byte = 40; | ||
let da_recorded_block_height = 10; | ||
let l2_block_height = 11; | ||
let original_known_total_cost = 150; | ||
let unrecorded_blocks = vec![BlockBytes { | ||
height: 11, | ||
block_bytes: 3000, | ||
}]; | ||
let da_p_component = 2; | ||
let guessed_cost: u64 = unrecorded_blocks | ||
.iter() | ||
.map(|block| block.block_bytes * da_cost_per_byte) | ||
.sum(); | ||
let projected_total_cost = original_known_total_cost + guessed_cost; | ||
|
||
let mut updater = UpdaterBuilder::new() | ||
.with_da_cost_per_byte(da_cost_per_byte as u128) | ||
.with_da_p_component(da_p_component) | ||
.with_last_profit(-10, 0) | ||
.with_da_recorded_block_height(da_recorded_block_height) | ||
.with_l2_block_height(l2_block_height) | ||
.with_projected_total_cost(projected_total_cost as u128) | ||
.with_known_total_cost(original_known_total_cost as u128) | ||
.with_unrecorded_blocks(unrecorded_blocks.clone()) | ||
.build(); | ||
|
||
let new_cost_per_byte = 100; | ||
let (recorded_heights, recorded_cost) = | ||
unrecorded_blocks | ||
.iter() | ||
.fold((vec![], 0), |(mut range, cost), block| { | ||
range.push(block.height); | ||
(range, cost + block.block_bytes * new_cost_per_byte) | ||
}); | ||
let min = recorded_heights.iter().min().unwrap(); | ||
let max = recorded_heights.iter().max().unwrap(); | ||
let recorded_range = *min..(max + 1); | ||
|
||
let old_da_gas_price = updater.new_scaled_da_gas_price; | ||
|
||
// when | ||
updater | ||
.update_da_record_data(recorded_range, recorded_cost as u128) | ||
.unwrap(); | ||
|
||
// then | ||
let new_da_gas_price = updater.new_scaled_da_gas_price; | ||
// because the profit is -10 and the da_p_component is 2, the new da gas price should be greater than the previous one. | ||
assert_eq!(new_da_gas_price, 6); | ||
assert_ne!(old_da_gas_price, new_da_gas_price); | ||
} | ||
|
||
#[test] | ||
fn update_da_record_data__da_block_will_not_change_da_gas_price() { | ||
// given | ||
let da_cost_per_byte = 40; | ||
let da_recorded_block_height = 10; | ||
let l2_block_height = 11; | ||
let original_known_total_cost = 150; | ||
let unrecorded_blocks = vec![BlockBytes { | ||
height: 11, | ||
block_bytes: 3000, | ||
}]; | ||
let da_p_component = 2; | ||
let guessed_cost: u64 = unrecorded_blocks | ||
.iter() | ||
.map(|block| block.block_bytes * da_cost_per_byte) | ||
.sum(); | ||
let projected_total_cost = original_known_total_cost + guessed_cost; | ||
|
||
let mut updater = UpdaterBuilder::new() | ||
.with_da_cost_per_byte(da_cost_per_byte as u128) | ||
.with_da_p_component(da_p_component) | ||
.with_last_profit(0, 0) | ||
.with_da_recorded_block_height(da_recorded_block_height) | ||
.with_l2_block_height(l2_block_height) | ||
.with_projected_total_cost(projected_total_cost as u128) | ||
.with_known_total_cost(original_known_total_cost as u128) | ||
.with_unrecorded_blocks(unrecorded_blocks.clone()) | ||
.build(); | ||
|
||
let new_cost_per_byte = 100; | ||
let (recorded_heights, recorded_cost) = | ||
unrecorded_blocks | ||
.iter() | ||
.fold((vec![], 0), |(mut range, cost), block| { | ||
range.push(block.height); | ||
(range, cost + block.block_bytes * new_cost_per_byte) | ||
}); | ||
let min = recorded_heights.iter().min().unwrap(); | ||
let max = recorded_heights.iter().max().unwrap(); | ||
let recorded_range = *min..(max + 1); | ||
|
||
let old_da_gas_price = updater.new_scaled_da_gas_price; | ||
|
||
// when | ||
updater | ||
.update_da_record_data(recorded_range, recorded_cost as u128) | ||
.unwrap(); | ||
|
||
// then | ||
let new_da_gas_price = updater.new_scaled_da_gas_price; | ||
assert_eq!(old_da_gas_price, new_da_gas_price); | ||
} |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -1,3 +1,4 @@ | ||
pub mod algorithm; | ||
pub mod da_source_service; | ||
pub mod metadata; | ||
pub mod service; |
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -21,12 +21,12 @@ trait BlockCommitterApi: Send + Sync { | |
/// Used to get the costs for a specific seqno | ||
async fn get_costs_by_seqno( | ||
&self, | ||
number: u64, | ||
number: u32, | ||
) -> DaBlockCostsResult<Option<RawDaBlockCosts>>; | ||
/// Used to get the costs for a range of blocks (inclusive) | ||
async fn get_cost_bundles_by_range( | ||
&self, | ||
range: core::ops::Range<u64>, | ||
range: core::ops::Range<u32>, | ||
) -> DaBlockCostsResult<Vec<Option<RawDaBlockCosts>>>; | ||
} | ||
|
||
|
@@ -40,9 +40,9 @@ pub struct BlockCommitterDaBlockCosts<BlockCommitter> { | |
#[derive(Debug, Deserialize, Serialize, Clone, Default, PartialEq)] | ||
pub struct RawDaBlockCosts { | ||
/// Sequence number (Monotonically increasing nonce) | ||
pub sequence_number: u64, | ||
pub sequence_number: u32, | ||
/// The range of blocks that the costs apply to | ||
pub blocks_range: core::ops::Range<u64>, | ||
pub blocks_range: core::ops::Range<u32>, | ||
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'll need to change this to just be a vec with these changes: |
||
/// The DA block height of the last transaction for the range of blocks | ||
pub da_block_height: DaBlockHeight, | ||
/// Rolling sum cost of posting blobs (wei) | ||
|
@@ -143,7 +143,7 @@ impl BlockCommitterApi for BlockCommitterHttpApi { | |
|
||
async fn get_costs_by_seqno( | ||
&self, | ||
number: u64, | ||
number: u32, | ||
) -> DaBlockCostsResult<Option<RawDaBlockCosts>> { | ||
let response = self | ||
.client | ||
|
@@ -157,7 +157,7 @@ impl BlockCommitterApi for BlockCommitterHttpApi { | |
|
||
async fn get_cost_bundles_by_range( | ||
&self, | ||
range: core::ops::Range<u64>, | ||
range: core::ops::Range<u32>, | ||
) -> DaBlockCostsResult<Vec<Option<RawDaBlockCosts>>> { | ||
let response = self | ||
.client | ||
|
@@ -192,23 +192,24 @@ mod tests { | |
} | ||
async fn get_costs_by_seqno( | ||
&self, | ||
seq_no: u64, | ||
seq_no: u32, | ||
) -> DaBlockCostsResult<Option<RawDaBlockCosts>> { | ||
// arbitrary logic to generate a new value | ||
let mut value = self.value.clone(); | ||
if let Some(value) = &mut value { | ||
value.sequence_number = seq_no; | ||
value.blocks_range = | ||
value.blocks_range.end * seq_no..value.blocks_range.end * seq_no + 10; | ||
value.da_block_height = value.da_block_height + (seq_no + 1).into(); | ||
value.da_block_height = | ||
value.da_block_height + ((seq_no + 1) as u64).into(); | ||
value.total_cost += 1; | ||
value.total_size_bytes += 1; | ||
} | ||
Comment on lines
197
to
207
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. There is a lot of unchecked arithmetic and casting going on here. How do you feel about switching to checked and |
||
Ok(value) | ||
} | ||
async fn get_cost_bundles_by_range( | ||
&self, | ||
_: core::ops::Range<u64>, | ||
_: core::ops::Range<u32>, | ||
) -> DaBlockCostsResult<Vec<Option<RawDaBlockCosts>>> { | ||
Ok(vec![self.value.clone()]) | ||
} | ||
|
@@ -286,7 +287,7 @@ mod tests { | |
} | ||
async fn get_costs_by_seqno( | ||
&self, | ||
seq_no: u64, | ||
seq_no: u32, | ||
) -> DaBlockCostsResult<Option<RawDaBlockCosts>> { | ||
// arbitrary logic to generate a new value | ||
let mut value = self.value.clone(); | ||
|
@@ -301,7 +302,7 @@ mod tests { | |
} | ||
async fn get_cost_bundles_by_range( | ||
&self, | ||
_: core::ops::Range<u64>, | ||
_: core::ops::Range<u32>, | ||
) -> DaBlockCostsResult<Vec<Option<RawDaBlockCosts>>> { | ||
Ok(vec![self.value.clone()]) | ||
} | ||
|
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.
Ah. Haha. I get it now... I think it makes sense either way but this probably better.
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 should have some tests in the algo 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 8ae983f