oracle-core icon indicating copy to clipboard operation
oracle-core copied to clipboard

[100 SigmaUSD] Newtypes for epoch id/counter, datapoint, height

Open greenhat opened this issue 2 years ago • 3 comments

Motivation

To not confuse them in function parameters with each other and with other integer values.

Implementation details

Use the newtypes in the relevant structs, function parameters, etc. Underneath, epoch id/counter should be u32, datapoint - i64, height - u32.

greenhat avatar Oct 24 '22 07:10 greenhat

I wonder whether something like a builder pattern would be better for this.

So calls to this for example

#[allow(clippy::too_many_arguments)]
pub fn build_refresh_action(
    pool_box_source: &dyn PoolBoxSource,
    refresh_box_source: &dyn RefreshBoxSource,
    datapoint_stage_src: &dyn DatapointBoxesSource,
    max_deviation_percent: u32,
    min_data_points: u32,
    wallet: &dyn WalletDataSource,
    height: u32,
    change_address: Address,
    my_oracle_pk: &EcPoint,
)

could be turned into something along these lines:

let refresh_action = RefreshActionBuilder::new()
    .with_pool_box_source(&pool_box)
    .with_refresh_box_source(&update_box)
    .with_max_deviation_percent(deviation_percent)
    .with_height(height)
    .build()?

That way it's easier to verify for the caller to see if the right parameters were passed without having to wrap parameters in newtypes

SethDusek avatar Oct 24 '22 11:10 SethDusek

@SethDusek I'm not sure a builder pattern will be a better fit here. It shines when some of the parameters are optional, which is not our case (we need all of them anyway). In terms of checking the parameter types, it does help the human eye by providing a kind of "parameter names" to catch on. But nothing beats a compiler error in preventing from passing an epoch id instead of height. :)

greenhat avatar Oct 24 '22 12:10 greenhat

That's a good point yeah, there aren't really any optional parameters as well. I guess newtypes are the better fit here

SethDusek avatar Oct 25 '22 13:10 SethDusek

I will work on this

SethDusek avatar Nov 22 '22 02:11 SethDusek

The bounty is sent. Thank you!

greenhat avatar Feb 10 '23 13:02 greenhat