fix(makerbot): allow more than one prices url in makerbot

[shamardy]

fixes #2016

[smk762]

Could we please leave the singular price_url for backwards compatibility in the short term? e.g. translate it into a list with single item?
During netid migration it would be optimal to avoid any maker config issues until transition is complete.

[smk762]

No error is thrown if using the old param, looks like it uses the defaults. I'll can mark it as deprecated in docs, could we also add a log message in the bot update loop when it is used to warn users of the deprecation?

[shamardy]

could we also add a log message in the bot update loop when it is used to warn users of the deprecation?

Mmm, I guess to do that I have to keep price_url in addition to price_urls to check if it's used or not. If used, what would you like the behaviour to be of the 2 below options:
1 - Translate it into a list with single item and log a warning message.
2 - Use default list and log warning message.

[shamardy]

A third option is to revert price_urls to price_url and make it accept either a list or a string, but it won't be very good in my opinion since we need to inform users about the change.

[onur-ozkan]

A third option is to revert price_urls to price_url and make it accept either a list or a string, but it won't be very good in my opinion since we need to inform users about the change.

I would support both at the same time with an enum.

e.g.,:

enum PriceSourceType {
    #[serde(rename = "price_url")]
    Singular(String),
    #[serde(rename = "price_urls")]
    Multiple(Vec<String>),
}

impl PriceSourceType {
    /// Always use this to get the data
    fn get_urls(&self) -> Vec<String> {
        match self {
            // TODO: deprecate price_url soon and inform the users
            PriceSourceType::Singular(t) => vec![t.clone()],
            PriceSourceType::Multiple(t) => t.clone(),
        }
    }
}

[shamardy]

Done

[smk762]

Thanks for the quick PR!
For the docs update, can you confirm the price_urls priority is sequential? e.g. the nth url wont be used unless all prior urls failed?

[shamardy]

For the docs update, can you confirm the price_urls priority is sequential? e.g. the nth url wont be used unless all prior urls failed?

Yep, it's sequential. I will also add a rotation logic (if the first one on the list fails we move it to the back of the list for subsequent requests).

[shamardy]

I will also add a rotation logic (if the first one on the list fails we move it to the back of the list for subsequent requests).

Done

[onur-ozkan]

LGTM

[borngraced]

Great work! just couple notes

[borngraced]

very minor

[borngraced]

 let cfg = running_state.trading_bot_cfg.clone();
    let rates_registry = match fetch_price_tickers(&mut running_state.price_urls).await {
        Ok(model) => model,
        Err(err) => {
            let nb_orders = cancel_pending_orders(ctx, &cfg).await;
            error!("error fetching price: {err:?} - cancel {nb_orders} orders");
            return;
        },
    };

[shamardy]

done

[borngraced]

🚀

[smk762]

Thanks!

• Confirmed backwards compatibility if using price_url
• Confirmed price url list progression if a failing url is detected.
• Confirmed error message in logs when progressing to next url after one fails.