feat: remove redis for exchange position tracking #398
Conversation
thevaibhav-dixit
force-pushed
the
remove-redis-wip
branch
4 times, most recently
from
d2ae2fe to
c8e8741
Compare
ledger/src/lib.rs
Outdated
| .exchange_position_account_balance(exchange_position_id) | ||
| .await? | ||
| .map(|b| b.settled()) | ||
| .unwrap_or(rust_decimal::Decimal::ZERO); |
There was a problem hiding this comment.
should we return an error here if the option is None or does the unwrap_or pattern works fine?
ledger/src/balances.rs
Outdated
| &self, | ||
| exchange_position_id: impl Into<LedgerAccountId> + std::fmt::Debug, | ||
| ) -> Result<Option<AccountBalance>, LedgerError> { | ||
| self.get_ledger_account_balance(HEDGING_JOURNAL_ID, exchange_position_id, self.usd) |
There was a problem hiding this comment.
can you rename it to EXCHANGE_POSITION_JOURNAL_ID
ledger/src/balances.rs
Outdated
| .await | ||
| } | ||
|
|
||
| pub async fn hedge_position_omnibus_account_balance( |
There was a problem hiding this comment.
consistent use of exchange_position
ledger/src/lib.rs
Outdated
| @@ -10,7 +10,7 @@ mod constants; | |||
| mod error; | |||
| mod templates; | |||
|
|
|||
| use constants::*; | |||
| pub use constants::*; | |||
There was a problem hiding this comment.
why are these public?
ledger/src/lib.rs
Outdated
| @@ -67,6 +72,84 @@ impl Ledger { | |||
| } | |||
| } | |||
|
|
|||
| #[instrument(name = "ledger.adjust_exchange_position", skip(self, tx))] | |||
| pub async fn adjust_exchange_position( | |||
There was a problem hiding this comment.
don't see why this is pub.
ledger/src/lib.rs
Outdated
| .exchange_position_account_balance(exchange_position_id) | ||
| .await? | ||
| .map(|b| b.settled()) | ||
| .unwrap_or(rust_decimal::Decimal::ZERO); |
ledger/src/lib.rs
Outdated
| .map(|b| b.settled()) | ||
| .unwrap_or(rust_decimal::Decimal::ZERO); | ||
| let diff = current_balance * CENTS_PER_USD + usd_cents_amount; | ||
| if diff < rust_decimal::Decimal::ZERO { |
There was a problem hiding this comment.
why not use rust_decimal::Decimal at the top?
| .map(|b| b.settled()) | ||
| .unwrap_or(Decimal::ZERO); | ||
|
|
||
| ledger |
There was a problem hiding this comment.
this should not be exposed. Expose adjust_okex_position like in the calling code in poll_okex
ledger/tests/ledger.rs
Outdated
| .unwrap() | ||
| .settled(); | ||
| assert_eq!( | ||
| balance_after_second_adjustment - balance_after_first_adjustment, |
There was a problem hiding this comment.
I don't understand this assert. You are executing the same template twice and only asserting the effect of the second application. Wouldn't it be better to apply each template once and assert balance_after_second_adjustment - initial_balance - that way both templates must have worked.
thevaibhav-dixit
force-pushed
the
remove-redis-wip
branch
from
91f1ce3 to
5fb1cbe
Compare
No description provided.