All Trades endpoint

[kevin-pease]

✨ What kind of change does this PR introduce? (Bug fix, feature, docs update...)

Begins the implementation of the Trades endpoint group, starting with the All Trades endpoint.
Includes some minor test value updates in the offers endpoint group, in order for the tests to succeed.

💥 Does this PR introduce a breaking change?

No.

🤔 Checklist before submitting

• All projects build
• Follows style guide lines
• Relevant documentation was updated
• Rebased onto current main

[tluijken]

Is deze code nu duplicaat? De check lijkt hetzelfde, maar de implementatie verschilt licht. Wellicht bedoelde je bij de 2e check geen base_asset maar een counter_asset?

[kevin-pease]

Ah, ja, klopt helemaal wat je zegt.

[tluijken]

De code voor het ophalen van de QueryString parameters is redelijk duplicaat, met uitzondering can de counter/base prefix. Dit kun je weer samenvoegen in 1 functie. Bijvoorbeeld zoiets:

impl Request for AllTradesRequest {
    fn add_asset_query(&mut self, query: &mut Vec<String>, asset: &Asset, asset_prefix: &str) {
        let asset_type = match &asset.0 {
            AssetType::Native => "native".to_string(),
            AssetType::Alphanumeric4(asset_data) | AssetType::Alphanumeric12(asset_data) => {
                let asset_type_label = if matches!(asset.0, AssetType::Alphanumeric4(_)) {
                    "credit_alphanum4"
                } else {
                    "credit_alphanum12"
                };
                query.push(format!("&{}_asset_code={}", asset_prefix, asset_data.asset_code));
                query.push(format!("&{}_asset_issuer={}", asset_prefix, asset_data.asset_issuer));
                asset_type_label.to_string()
            }
        };
        query.push(format!("{}_asset_type={}", asset_prefix, asset_type));
    }

    fn get_query_parameters(&self) -> String {
        let mut query = Vec::new();
        if let Some(base_asset) = &self.base_asset {
            self.add_asset_query(&mut query, base_asset, "base");
        }
        if let Some(counter_asset) = &self.counter_asset {
            self.add_asset_query(&mut query, counter_asset, "counter");
        }
        query.join("&").trim_start_matches('&').to_string()
    }

    fn build_url(&self, base_url: &str) -> String {
        format!("{}/{}?{}", base_url, super::TRADES_PATH, self.get_query_parameters())
    }
}

[kevin-pease]

Dit had ik inderdaad ook gezien. Het gebeurt ook elders in de code, dus misschien kunnen we een stapje verder gaan door er een algemeen iets van te maken?

[tluijken]

Dat klinkt als een goed idee :D

[tluijken]

Gaat dit goed met de andere preludes?
Neem bijv AccountResponseLinks. Door een dedicated naam te gebruiken weten we zeker dat we niet meerdere public structs hebben met dezelfde naam (wat via een prelude voor conflicten kan zorgen).

[kevin-pease]

Tot nu toe gaat het goed, maar ik kan me voorstellen dat het inderdaad problemen gaat opleveren; ik ga het aanpassen. Misschien is het goed om te kijken of de andere endpoints ook van deze conventie gebuik maken. 🙂

[tluijken]

Wellicht nog wel mooi om je voorstel mee te nemen om die ene match lus voor base_asset en counter_asset generiek te maken als die elders gebruikt wordt. Mag ook buiten deze PR.

[tluijken]

Vergeet niet deze branch te rebasen op develop en de conflicten even te fixen.