-
Notifications
You must be signed in to change notification settings - Fork 12
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
Add slashing tests #109
Add slashing tests #109
Conversation
6adb6c5
to
86269c6
Compare
33dba3e
to
7b5810b
Compare
7b5810b
to
b839893
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.
Good progress. Some feedback on the tests
tests/e2e/slashing_test.go
Outdated
// and that the validator has been jailed | ||
validator1, found = x.ConsumerApp.StakingKeeper.GetValidator(ctx, myExtValidator1) | ||
require.True(t, validator1.IsJailed()) | ||
// FIXME: Slashing amount is off by ~5% |
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.
This is what I tried to explain in the call. It will always run out of sync as the slashing logic is very complex in the SDK and takes the power of the validator into account. Contracts don't have access to state like undelegates or rebalances which are slashed first.
Nevertheless you can pick one side as the leader sync it to the other side. Either the provider is in charge and applies a percentage on the delegated amounts or the consumer communicates the amount burned to the contract.
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.
This was caused by the jailValidator
test helper hard-coding the validator's power to 100
. Now fixed.
Slashing logic is somewhat complex, but not something we cannot capture / reproduce in contracts.
Undelegations are being slashed on the contracts already. Redelegations are not possible with mesh-security at this point. Rebalances should't be an issue; but I need to explore it further, and we need to test it.
So, it seems to me at this point, most of the differences are due to rounding logic.
Using the slash ratios looks simpler, is impervious to price conversions, and in line with the original design.
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.
There are also actors on the consumer chain that can re-/undelegate. This data is not available to contracts and affects the slashed amount of the validator.
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.
MS contracts are not concerned with those actors. We only slash the cross-delegators associated with the slashed validator.
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.
Very nice to have some real numbers here. I added some question about the max lien amount. I am not sure if I have the correct understanding how they are calculated.
The tests cover the slash event on the consumer side and the event handling on the provider side. It would be great to also have the rebalance executed for the full cycle to ensure that the numbers match as well. This can be done in a new PR though.
I would be curious to see a slash on the provider side executed. But this can also be a new PR.
// Check new collateral | ||
require.Equal(t, 190_000_000, providerCli.QueryVaultBalance()) | ||
// Check new max lien | ||
require.Equal(t, 190_000_000, providerCli.QueryMaxLien()) |
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.
Can you elaborate how the max lien is calculated? Now that the free amount is consumed, I was expecting the max lien to be 190_000_000 * 0.9 to take the local slashing factor into account
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.
In this scenario there was free collateral to be slashed.
The max lien is the max of all the liens, and as long as it remains below the collateral (free collateral >= 0), it doesn't need to change.
// Check collateral | ||
require.Equal(t, 200_000_000, providerCli.QueryVaultBalance()) | ||
// Check max lien | ||
require.Equal(t, 190_000_000, providerCli.QueryMaxLien()) |
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.
As below, i was expecting only 90% of the staked amount as max lien
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.
Same explanation as before. This check is pre-slashing, in fact. It's there so that we can see that the amounts don't change after slashing.
Take a look at https://github.com/osmosis-labs/mesh-security/blob/main/docs/provider/Vault.md#invariants and related, in case you're interested.
Indeed. Will do that in a follow-up test.
This would require the new version of the contracts (v0.9.0) which will be released soon. Let's release this version of the SDK, and then work on testing and releasing that. |
Slashing scenarios tests.
Checks that tests work at the vault's level.
TODO:
Scenario 4. (not supported) (Requires more than one Consumer)Scenario 5. (not supported) (Requires more than one Consumer)