-
Notifications
You must be signed in to change notification settings - Fork 8
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
virtual-staking: Add tests for validator withdrawal and rebalance #132
Conversation
@maurolacy Could you take a look if these are good enough tests for jail/tombstone/removal for #123? https://github.com/osmosis-labs/mesh-security/pull/132/files#diff-55aab61e430421dc3035cc9d8bcf2263cbc08138462de265da9a4b0efe4c3038R558-R622 They currently fail, but that's expected. |
b6b0379
to
6ed7579
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM!
6ed7579
to
727b2b8
Compare
Rebased |
727b2b8
to
e691fb8
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Sorry for the delay in reviewing. These tests are great, thanks for adding them.
contract.quick_bond(deps.as_mut(), "val1", 5); | ||
contract | ||
.hit_epoch(deps.as_mut()) | ||
.assert_no_bonding() |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is this because max_cap
by default is zero? Please add a comment / rename the test.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes. Good catch - the max_cap = 0 should definitely be explicit.
contract.quick_bond(deps.as_mut(), "val2", 40); | ||
contract | ||
.hit_epoch(deps.as_mut()) | ||
.assert_bond(&[("val1", (1u128, &denom)), ("val2", (4u128, &denom))]) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is this because of rebalance, right? Better add a comment, and / or change the test name.
I would also add a test in which the max_cap
is enough.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
What would be a clearer test name here? bond_rebalance
?
simple_bond
is a test case in which there's enough bonded.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I added a test case that's like simple_bond
, but with 2 validators.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
bond_rebalance
is OK.
@maurolacy I updated test names/comments. Hopefully it's all clear at a glance now! |
No description provided.