dex: redesign value circuit breaker #4086
Conversation
| // Note that since we credited the DEX for _all_ inflows, we need to debit the | ||
| // unfilled amounts as well as the filled amounts. |
There was a problem hiding this comment.
this would be good to cover in a test
erwanor
added
zellic-component-remediated
| } | ||
| } | ||
|
|
||
| impl<T: StateWrite + ?Sized> ValueCircuitBreaker for T {} | ||
|
|
||
| /* |
There was a problem hiding this comment.
What happens if the tests are uncommented?
There was a problem hiding this comment.
They don't compile, the tests need to be updated / rewritten, that's the remaining work on the PR that needs to be picked up.
| @@ -570,32 +569,6 @@ pub(crate) trait Inner: StateWrite { | |||
| "updating position assets' aggregate balances" | |||
There was a problem hiding this comment.
the update of the aggregate balance was for debugging the value circuit breaker; this tracing message and the net_change_* variables are likely no longer needed
| amount: Amount::from(0u64), | ||
| }) | ||
| /// Debits a deposit from the DEX. | ||
| async fn vcb_debit(&mut self, value: Value) -> Result<()> { |
There was a problem hiding this comment.
should we have some debug tracing messages here for the value circuit breaker credits/debits?
There was a problem hiding this comment.
Sure, or we could also / instead fire off an ABCI event, so that it's possible to build a dashboard of inflows and outflows.
| // Debit the DEX for the swap outflows. | ||
| // Note that since we credited the DEX for _all_ inflows, we need to debit the | ||
| // unfilled amounts as well as the filled amounts. | ||
| self.vcb_debit(Value { |
There was a problem hiding this comment.
is the idea that the debit will underflow and cause an error to be returned in the value circuit breaker triggered case? it might be good to explicitly call that out
|
This breaks a few tests, as expected because we moved the vb credits to the action handlers: I think we can fix them without too much effort by doing: let infinite_gm = Value {
asset_id: gm.id(),
amount: Amount::from(100000u128) * gm.unit_amount(),
};
let infinite_gn = Value {
asset_id: gn.id(),
amount: Amount::from(100000u128) * gn.unit_amount(),
};
let infinite_penumbra = Value {
asset_id: penumbra.id(),
amount: Amount::from(100000u128) * penumbra.unit_amount(),
};
state_tx.vcb_credit(infinite_gm).await?;
state_tx.vcb_credit(infinite_gn).await?;
state_tx.vcb_credit(infinite_penumbra).await?;We will be able to replace them with something much better but in the meantime it seems useful to keep them around as basic checks. |
hdevalence
force-pushed
the
redesign-dex-vcb
branch
from
c065dce to
61c59f7
Compare
Closes #4025 This commit sketches a new mechanism but does not fix the existing tests; it needs to be picked up and pushed over the finish line. (We should be testing this).
hdevalence
force-pushed
the
redesign-dex-vcb
branch
from
e8fcdab to
269a4d3
Compare
Closes #4025
This commit sketches a new mechanism but does not fix the existing tests; it needs to be picked up and pushed over the finish line. (We should be testing this).