Skip to content

Commit

Permalink
Consistent string representation of NativePriceEstimators (#2169)
Browse files Browse the repository at this point in the history
# Description

The current default argument of `native_price_estimators` is an invalid
value. This can be seen by running the autopilot without any argument:

```
$ cargo run --bin autopilot
[...]
native_price_estimators: GenericPriceEstimator("GenericPriceEstimator(\"Baseline\")")
[...]
```

This is because the default is generated with `default_values_t`, which
"Requires std::fmt::Display that roundtrips correctly with the
[Arg::value_parser](https://docs.rs/clap/latest/clap/struct.Arg.html#method.value_parser)
or #[arg(value_enum)]"
([source](https://docs.rs/clap/latest/clap/_derive/index.html#arg-attributes)).

This PR changes the string representation to work according to the docs
remark.

A negative of this change is that we technically can't distinguish
between
`NativePriceEstimator::GenericPriceEstimator("OneInchSpotPriceApi")` and
`NativePriceEstimator::OneInchSpotPriceApi` just from looking at the
string representation.

# Changes

- Updated string representation for `NativePriceEstimators`.

## How to test

New unit test. Try to run the autopilot without arguments again:

```
[...]
native_price_estimators: Baseline
[...]
```

Also, note that `Display` is only used for printing the arguments on
startup, so this shouldn't break anything else. To see this, try to
remove the implementation for `Display` for `NativePriceEstimators`, use
`default_values` to not need `Display`, and fix the few compiling
argument to confirm that `Display` is unused otherwise.
  • Loading branch information
fedgiac authored Dec 14, 2023
1 parent ae11d56 commit c8dcee6
Showing 1 changed file with 32 additions and 1 deletion.
33 changes: 32 additions & 1 deletion crates/shared/src/price_estimation.rs
Original file line number Diff line number Diff line change
Expand Up @@ -149,6 +149,16 @@ pub enum NativePriceEstimator {
OneInchSpotPriceApi,
}

impl Display for NativePriceEstimator {
fn fmt(&self, f: &mut Formatter) -> fmt::Result {
let formatter = match self {
NativePriceEstimator::GenericPriceEstimator(s) => s,
NativePriceEstimator::OneInchSpotPriceApi => "OneInchSpotPriceApi",
};
write!(f, "{}", formatter)
}
}

impl NativePriceEstimators {
pub fn as_slice(&self) -> &[Vec<NativePriceEstimator>] {
&self.0
Expand All @@ -171,7 +181,7 @@ impl Display for NativePriceEstimators {
.map(|stage| {
stage
.iter()
.format_with(",", |estimator, f| f(&format_args!("{:?}", estimator)))
.format_with(",", |estimator, f| f(&format_args!("{estimator}")))
})
.format(";");
write!(f, "{}", formatter)
Expand Down Expand Up @@ -605,4 +615,25 @@ mod tests {
estimator(PriceEstimatorKind::BalancerSor, address(1))
);
}

#[test]
fn string_repr_round_trip_native_price_estimators() {
// We use NativePriceEstimators as one of the types used in an Arguments object
// that derives clap::Parser. Clap parsing of an argument using
// default_value_t requires that std::fmt::Display roundtrips correctly with the
// Arg::value_parser or #[arg(value_enum)]:
// https://docs.rs/clap/latest/clap/_derive/index.html#arg-attributes

let parsed = |arg: &str| NativePriceEstimators::from(arg);
let stringified = |arg: &NativePriceEstimators| format!("{arg}");

for repr in [
&NativePriceEstimator::GenericPriceEstimator("Baseline".into()).to_string(),
&NativePriceEstimator::OneInchSpotPriceApi.to_string(),
"one,two;three,four",
&format!("one,two;{},four", NativePriceEstimator::OneInchSpotPriceApi),
] {
assert_eq!(stringified(&parsed(repr)), repr);
}
}
}

0 comments on commit c8dcee6

Please sign in to comment.