Merge pull request #166 from osmosis-labs/fix/slashing-sequence

Fix/slashing sequence

Author: maurolacy

contracts/consumer/virtual-staking/src/contract.rs

@@ -880,7 +880,7 @@ mod tests {
             .hit_epoch(deps.as_mut())
             .assert_bond(&[]) // No bond msgs after jailing
             .assert_unbond(&[]) // No unbond msgs after jailing
-            .assert_rewards(&["val1", "val2"]); // But rewards are still being gathered
+            .assert_rewards(&["val2"]); // Rewards are not gathered anymore because of the removal
 
         // Check that the bonded amounts of val1 have been slashed for being offline (10%)
         // Val2 is unaffected.
@@ -946,7 +946,7 @@ mod tests {
             .hit_epoch(deps.as_mut())
             .assert_bond(&[("val1", (20u128, &denom))]) // Tombstoned validators can still bond
             .assert_unbond(&[]) // No unbond msgs after jailing
-            .assert_rewards(&["val1"]); // Rewards are still being gathered
+            .assert_rewards(&[]); // Rewards are not gathered anymore because of jailing implying removal
 
         // Check that the non-slashed amounts of val1 have been bonded
         let bonded = contract.bonded.load(deps.as_ref().storage).unwrap();
@@ -978,7 +978,7 @@ mod tests {
             .hit_epoch(deps.as_mut())
             .assert_bond(&[]) // No bond msgs after jailing
             .assert_unbond(&[("val1", (9u128, &denom))]) // Only unbond non-slashed amount
-            .assert_rewards(&["val1"]); // Rewards are still being gathered
+            .assert_rewards(&[]); // Rewards are not gathered anymore because of the removal
 
         // Check that the non-slashed amounts of val1 have been unbonded
         // FIXME: Remove / filter zero amounts
@@ -1424,8 +1424,17 @@ mod tests {
         }
 
         fn jail(&self, deps: DepsMut, val: &str) {
-            self.handle_valset_update(deps, &[], &[], &[], &[val.to_string()], &[], &[])
-                .unwrap();
+            // We sent a removal along with the jail, as this is what the blockchain does
+            self.handle_valset_update(
+                deps,
+                &[],
+                &[val.to_string()],
+                &[],
+                &[val.to_string()],
+                &[],
+                &[],
+            )
+            .unwrap();
         }
 
         fn unjail(&self, deps: DepsMut, val: &str) {

contracts/provider/external-staking/src/contract.rs

@@ -488,13 +488,6 @@ impl ExternalStakingContract<'_> {
             // Maintenance
             valopers.insert(valoper.clone());
         }
-        // Process removals. Non-existent validators will be ignored.
-        for valoper in removals {
-            self.val_set
-                .remove_validator(deps.storage, valoper, height, time)?;
-            // Maintenance
-            valopers.insert(valoper.clone());
-        }
         // Process jailings. Non-existent validators will be ignored.
         for valoper in jailed {
             // Check that the validator is active at height and slash it if that is the case
@@ -514,6 +507,17 @@ impl ExternalStakingContract<'_> {
             // Maintenance
             valopers.insert(valoper.clone());
         }
+        // Process removals. Non-existent validators will be ignored.
+        // Filter out jailed validators, as they are already removed from the active validator set
+        let rms: HashSet<_> = removals.iter().collect();
+        let j: HashSet<_> = jailed.iter().collect();
+        let rms: Vec<_> = rms.difference(&j).collect();
+        for valoper in rms {
+            self.val_set
+                .remove_validator(deps.storage, valoper, height, time)?;
+            // Maintenance
+            valopers.insert((*valoper).clone());
+        }
         // Process unjailings. Does nothing at the moment, as we don't have a way to know if we the
         // validator must go to the active or the unbonded state.