Trade Aggregations endpoint #96
Conversation
| match &self.base_asset.0 { | ||
| AssetType::Native => { | ||
| asset_parameters.push(format!("base_asset_type=native")); | ||
| } | ||
| AssetType::Alphanumeric4(asset) => { | ||
| asset_parameters.push(format!("base_asset_type=credit_alphanum4")); | ||
| asset_parameters.push(format!("&base_asset_code={}", asset.asset_code)); | ||
| asset_parameters.push(format!("&base_asset_issuer={}", asset.asset_issuer)); | ||
| } | ||
| AssetType::Alphanumeric12(asset) => { | ||
| asset_parameters.push(format!("base_asset_type=credit_alphanum12")); | ||
| asset_parameters.push(format!("&base_asset_code={}", asset.asset_code)); | ||
| asset_parameters.push(format!("&base_asset_issuer={}", asset.asset_issuer)); | ||
| } | ||
| } | ||
|
|
||
| match &self.counter_asset.0 { | ||
| AssetType::Native => { | ||
| asset_parameters.push(format!("&counter_asset_type=native")); | ||
| } | ||
| AssetType::Alphanumeric4(asset) => { | ||
| asset_parameters.push(format!("&counter_asset_type=credit_alphanum4")); | ||
| asset_parameters.push(format!("&counter_asset_code={}", asset.asset_code)); | ||
| asset_parameters.push(format!("&counter_asset_issuer={}", asset.asset_issuer)); | ||
| } | ||
| AssetType::Alphanumeric12(asset) => { | ||
| asset_parameters.push(format!("&counter_asset_type=credit_alphanum12")); | ||
| asset_parameters.push(format!("&counter_asset_code={}", asset.asset_code)); | ||
| asset_parameters.push(format!("&counter_asset_issuer={}", asset.asset_issuer)); | ||
| } | ||
| } |
There was a problem hiding this comment.
Deze constructie heb ik eerder gezien. Er zit veel duplicate code bij.
Kun je m in lijn met deze implementatie brengen?
There was a problem hiding this comment.
Klopt, deze komt elders ook voor. Ik dacht dat we hier een aparte issue voor hadden aangemaakt (zodat ik dit in de maintenance-fase kan oppakken) maar deze staat er nog niet bij. Zal ik 'm nu meenemen, of in een nieuwe feature branch? Dan zou ik gelijk die andere implementatie kunnen refactoren.
There was a problem hiding this comment.
Ik zou m gelijk nu meepakken.
| /// // Use with HorizonClient::get_trade_aggregations | ||
| /// ``` | ||
| /// | ||
| #[derive(Clone, Debug, PartialEq, Default)] |
There was a problem hiding this comment.
Kunnen we hier de Pagination derive gebruiken? Dan hoef je ook niet de set_limit, set_order te implementeren. Dat regelt de derive voor je.
There was a problem hiding this comment.
Dat heb ik inderdaad overwogen, alleen ontbreekt cursor in deze request. Deze zou dan toegevoegd moeten worden, zonder dat deze gebruikt wordt. Is dat wenselijk?
There was a problem hiding this comment.
Interessant, want hoe indexed deze paginering nu zonder cursor?
There was a problem hiding this comment.
Dat kan ik nergens terugvinden in de documentatie. Misschien was een limiet en sorteervolgorde voldoende bij dit endpoint.
There was a problem hiding this comment.
Ik snap m, het gaat met name om start en end tijd, icm met de resolution.
In dat geval zou ik zeggen, laat m zoals ie nu is. De implementatie werkt anders dan de andere paginatie functies. DRY is een fabel ;-)
kevin-pease
force-pushed
the
55-trade-aggregations
branch
from
f1cd94a
to
0fb73a8
Compare
✨ What kind of change does this PR introduce? (Bug fix, feature, docs update...)
Adds the trade aggregations endpoint. Fixes #55.
💥 Does this PR introduce a breaking change?
No.
📝 Links to relevant issues/docs
Note: the
offsetfield is mentioned in the documentation, but isn't present in the Laboratory. See:https://developers.stellar.org/docs/data/horizon/api-reference/aggregations/trade-aggregations/list
🤔 Checklist before submitting