Skip to content

Commit

Permalink
refactor(staking): outlined undelegate invariant checks
Browse files Browse the repository at this point in the history
see: #4110 (comment)

emitting tracing events makes the body of `check_and_execute` more
verbose. in an effort to help maintain the pleasant quality of
`anyhow::ensure` listing invariants we care about, this explores
outlining that logic into helper methods of `Undelegate`.

this is in some respects a little "louder", but makes the
`check_and_execute` a succinct list of calls:

```rust
// # Invariant: the undelegation must have been prepared for the current epoch.
self.check_that_undelegate_was_prepared_for_current_epoch(&state)
    .await?;

// # Invariant: The unbonded amount must be correctly computed.
self.check_that_unbonded_amount_is_correctly_computed(&state)
    .await?;
```

NB: git doesn't handle this sort of code motion with grace. view this
diff via side-by-side view, if possible.
  • Loading branch information
cratelyn committed Mar 28, 2024
1 parent d948eae commit 232ee82
Showing 1 changed file with 74 additions and 39 deletions.
113 changes: 74 additions & 39 deletions crates/core/component/stake/src/component/action_handler/undelegate.rs
Original file line number Diff line number Diff line change
@@ -1,8 +1,9 @@
use anyhow::Result;
use anyhow::{bail, Result};
use async_trait::async_trait;
use cnidarium::StateWrite;
use penumbra_sct::component::clock::EpochRead;
use penumbra_shielded_pool::component::AssetRegistry;
use tracing::error;

use crate::{
component::action_handler::ActionHandler,
Expand All @@ -18,43 +19,86 @@ impl ActionHandler for Undelegate {
}

async fn check_and_execute<S: StateWrite>(&self, mut state: S) -> Result<()> {
// These checks all formerly happened in the `check_historical` method,
// if profiling shows that they cause a bottleneck we could (CAREFULLY)
// move some of them back.
let u = self;
/* ----- checks ------ */
// NB: These checks all formerly happened in the `check_historical` method, if profiling
// shows that they cause a bottleneck we could (CAREFULLY) move some of them back.

// # Invariant: the undelegation must have been prepared for the current epoch.
self.check_that_undelegate_was_prepared_for_current_epoch(&state)
.await?;

// # Invariant: The unbonded amount must be correctly computed.
self.check_that_unbonded_amount_is_correctly_computed(&state)
.await?;

/* ----- execution ------ */

// Register the undelegation's denom, so clients can look it up later.
state.register_denom(&self.unbonding_token().denom()).await;

tracing::debug!(?self, "queuing undelegation for next epoch");
state.push_undelegation(self.clone());

// Check that the undelegation was prepared for the current epoch.
// This let us provide a more helpful error message if an epoch boundary was crossed.
// And it ensures that the unbonding delay is enforced correctly.
state.record(event::undelegate(self));

Ok(())
}
}

/// Interfaces for enforcing [`ActionHandler`] invariants.
impl Undelegate {
/// Returns `Ok(())` if this delegation is from the current epoch.
///
/// This ensures that the unbonding delay is enforced correctly. Additionally, it helps us
/// provide a more helpful error message if an epoch boundary was crossed.
async fn check_that_undelegate_was_prepared_for_current_epoch(
&self,
state: &impl StateWrite,
) -> Result<()> {
let u = self;
let current_epoch = state.get_current_epoch().await?;
let prepared_for_current_epoch = u.from_epoch == current_epoch;
if !prepared_for_current_epoch {
tracing::error!(
from = ?u.from_epoch,
current = ?current_epoch,
"undelegation was prepared for a different epoch",

// Check that this delegation is from the current epoch.
if u.from_epoch != current_epoch {
error!(
?u.from_epoch,
?current_epoch,
"undelegation was prepared for a different epoch"
);
anyhow::bail!(
bail!(
"undelegation was prepared for epoch {} but the current epoch is {}",
u.from_epoch.index,
current_epoch.index
);
}

// For undelegations, we enforce correct computation (with rounding)
// of the *unbonded amount based on the delegation amount*, because
// users (should be) starting with the amount of delegation tokens they
// wish to undelegate, and computing the amount of unbonded stake
// they receive.
//
// The direction of the computation matters because the computation
// involves rounding, so while both
//
// (unbonded amount, rates) -> delegation amount
// (delegation amount, rates) -> unbonded amount
//
// should give approximately the same results, they may not give
// exactly the same results.
Ok(())
}

/// Returns `Ok(())` if this delegation has a correctly computed unbonding amount.
///
/// For undelegations, we enforce correct computation (with rounding) of the *unbonded
/// amount based on the delegation amount*, because users (should be) starting with the
/// amount of delegation tokens they wish to undelegate, and computing the amount of
/// unbonded stake they receive.
///
/// The direction of the computation matters because the computation involves rounding, so
/// while both...
///
/// ```
/// (unbonded amount, rates) -> delegation amount
/// (delegation amount, rates) -> unbonded amount
/// ```
///
/// ...should give approximately the same results, they may not give *exactly* the same
/// results.
async fn check_that_unbonded_amount_is_correctly_computed(
&self,
state: &impl StateWrite,
) -> Result<()> {
let u = self;

// Compute the expected unbonded amount for the validator.
let rate_data = state
.get_validator_rate(&u.validator_identity)
.await?
Expand All @@ -63,6 +107,7 @@ impl ActionHandler for Undelegate {
})?;
let expected_unbonded_amount = rate_data.unbonded_amount(u.delegation_amount);

// Check that this undelegation matches the expected amount.
if u.unbonded_amount != expected_unbonded_amount {
tracing::error!(
actual = %u.unbonded_amount,
Expand All @@ -76,16 +121,6 @@ impl ActionHandler for Undelegate {
);
}

/* ----- execution ------ */

// Register the undelegation's denom, so clients can look it up later.
state.register_denom(&self.unbonding_token().denom()).await;

tracing::debug!(?self, "queuing undelegation for next epoch");
state.push_undelegation(self.clone());

state.record(event::undelegate(self));

Ok(())
}
}

0 comments on commit 232ee82

Please sign in to comment.