tests(app): ✨ add integration test coverage for undelegation #4110
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
cratelyn
changed the title
cratelyn
commented
Mar 26, 2024
cratelyn
force-pushed
the
kate/mock-consensus-staking-claim-undelegation-tokens-pt-ii
branch
from
3f97710 to
9604f01
Compare
cratelyn
force-pushed
the
kate/mock-consensus-staking-claim-undelegation-tokens-pt-ii
branch
from
9604f01 to
3790595
Compare
i found myself rewriting `client.notes.values().filter(|_| ..)` repeatedly. this helps test cases grab notes of a particular asset type.
cratelyn
force-pushed
the
kate/mock-consensus-staking-claim-undelegation-tokens-pt-ii
branch
from
3790595 to
b083423
Compare
cratelyn
force-pushed
the
kate/mock-consensus-staking-claim-undelegation-tokens-pt-ii
branch
from
b083423 to
79f08dd
Compare
cratelyn
force-pushed
the
kate/mock-consensus-staking-claim-undelegation-tokens-pt-ii
branch
from
79f08dd to
05c307a
Compare
cratelyn
force-pushed
the
kate/mock-consensus-staking-claim-undelegation-tokens-pt-ii
branch
from
05c307a to
63cf0d5
Compare
cratelyn
changed the title
26 tasks
erwanor
approved these changes
Mar 27, 2024
@erwanor notes: > Just two ideas, it would be good to check: > - the rate at which delegation <> undelegation token gets converted > (by dividing the amounts and comparing it against the stated > validator rate) > > - the rate at which undelegation <> staking token gets converted (so > that later we can pull that logic to check that slashing events > effect that rate correctly) as a step in that direction, i've captured the amounts from each note, and the validator rate. we will investigate this more tomorrow. ``` [crates/core/app/tests/app_can_undelegate_from_a_validator.rs:379:9] staking_note_amount = 1000000000 [crates/core/app/tests/app_can_undelegate_from_a_validator.rs:380:9] staking_note_2_amount = 1000000000 [crates/core/app/tests/app_can_undelegate_from_a_validator.rs:381:9] delegate_note_amount = 1000000000 [crates/core/app/tests/app_can_undelegate_from_a_validator.rs:382:9] undelegate_note_amount = 1001000000000 [crates/core/app/tests/app_can_undelegate_from_a_validator.rs:384:9] staking_note_amount / delegate_note_amount = 1 [crates/core/app/tests/app_can_undelegate_from_a_validator.rs:385:9] delegate_note_amount / undelegate_note_amount = 0 [crates/core/app/tests/app_can_undelegate_from_a_validator.rs:386:9] staking_note_2_amount / staking_note_amount = 1 [crates/core/app/tests/app_can_undelegate_from_a_validator.rs:392:9] validator_rate = Some( RateData { identity_key: penumbravalid1lg6ede74e4jsrnee8cvf9d9xe5n2ps2na8pga8jvpcqp3kpe5yrsvla0pu, validator_reward_rate: 49974986, validator_exchange_rate: 300100244731, }, ) test app_can_undelegate_from_a_validator ... ok ```
#4110 (comment) use `compute_unbonding_height` instead of manually calculating the unbonding height.
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.
hdevalence
previously requested changes
Mar 28, 2024
Comment on lines
49
to
57
| 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<()> { |
There was a problem hiding this comment.
We should not structure the code this way.
It spreads the consensus critical part of the ActionHandler out into different places, rather than than having it in one place where it can be inspected locally.
And it leaks the internals of that implementation into a public interface, with methods that may or may not appear depending on compile settings, because the Undelegate type must be independent of the component implementation.
There was a problem hiding this comment.
...i'm going to mark this as resolved, now that this change has been reverted.
This reverts commit 232ee82. #4110 (review) #4110 (comment) > We should not structure the code this way. > > It spreads the consensus critical part of the ActionHandler out into > different places, rather than than having it in one place where it can > be inspected locally. > > And it leaks the internals of that implementation into a public > interface, with methods that may or may not appear depending on > compile settings, because the Undelegate type must be independent of > the component implementation.
cratelyn
force-pushed
the
kate/mock-consensus-staking-claim-undelegation-tokens-pt-ii
branch
from
7bcd43c to
b513ddb
Compare
cratelyn
dismissed
hdevalence’s stale review
changes requested were made/reverted in b513ddb
cratelyn
deleted the
kate/mock-consensus-staking-claim-undelegation-tokens-pt-ii
branch
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
this introduces additional test coverage for the staking component, examining the mock client's notes during delegation, the unbonding period, and after claiming an undelegation.
ActionPlan: From<UndelegateClaimPlan>#4115